Re: [PATCH] bus: mhi: ep: Use kcalloc() instead of kzalloc()

[Dan Carpenter]

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