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. 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. I work in static analysis and I understand the struggle of trying to understand code to see if static checker warnings are a real bug or not. I'm not going to insist that you figure everything out, but I am asking that you at least try. If after spending ten minutes reading the code you can't figure it out, then it's fine to write something like, "I don't know whether this multiply can really overflow or not, but let's make it safer by using kcalloc()." You can put that sort of "I don't know information" under the --- cut off line inf you want. regards, dan carpenter
