On Mon, Jan 22, 2024 at 10:15:20AM +0300, Dan Carpenter wrote: > This code does not have an integer overflow, but it might have a > different memory corruption bug. > > On Sat, Jan 20, 2024 at 04:25:18PM +0100, Erick Archer wrote: > > As noted in the "Deprecated Interfaces, Language Features, Attributes, > > and Conventions" documentation [1], size calculations (especially > > multiplication) should not be performed in memory allocator (or similar) > > function arguments due to the risk of them overflowing. This could lead > > to values wrapping around and a smaller allocation being made than the > > caller was expecting. Using those allocations could lead to linear > > overflows of heap memory and other misbehaviors. > > > > So, use the purpose specific kcalloc() function instead of the argument > > count * size in the kzalloc() function. > > > > This one is more complicated to analyze. I have built a Smatch cross > function database so it's easy for me and I will help you. > > $ smbd.py where mhi_ep_cntrl event_rings > drivers/pci/endpoint/functions/pci-epf-mhi.c | pci_epf_mhi_probe | (struct mhi_ep_cntrl)->event_rings | 0 > drivers/bus/mhi/ep/main.c | mhi_ep_irq | (struct mhi_ep_cntrl)->event_rings | min-max > drivers/bus/mhi/ep/mmio.c | mhi_ep_mmio_init | (struct mhi_ep_cntrl)->event_rings | 0-255 > drivers/bus/mhi/ep/mmio.c | mhi_ep_mmio_update_ner | (struct mhi_ep_cntrl)->event_rings | 0-255 > > The other way to figure this stuff out would be to do: > > $ grep -Rn "event_rings = " drivers/bus/mhi/ep/ > drivers/bus/mhi/ep/mmio.c:260: mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); > drivers/bus/mhi/ep/mmio.c:261: mhi_cntrl->hw_event_rings = FIELD_GET(MHICFG_NHWER_MASK, regval); > drivers/bus/mhi/ep/mmio.c:271: mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); > drivers/bus/mhi/ep/mmio.c:272: mhi_cntrl->hw_event_rings = FIELD_GET(MHICFG_NHWER_MASK, regval); > > That means that this multiplication can never overflow so the patch > has no effect on runtime. The patch is still useful because we don't > want every single person to have to do this analysis. The kcalloc() > function is just safer and more obviously correct. > Agree. > It's a bit concerning that ->event_rings is set multiple times, but only > allocated one time. It's either unnecessary or there is a potential > memory corruption bug. If it's really necessary then there should be a > check that the new size is <= the size of the original buffer that we > allocated. Agree, the dual assignment could be avoided. I added it initially to have all the memory allocations in one place, and also there is a guarantee from the spec that the MHICFG_NER_MASK will always be initialized to hw max value. But looking at it again, it seems redundant. So I will drop the assignment from mhi_ep_mmio_init(). Thanks for spotting! - Mani -- மணிவண்ணன் சதாசிவம்
