Hi Dan,
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.
I don't see this possible memory corruption bug. More info below.
> 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.
Ok, I will send a v2 patch with more info in the commit message.
> 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.
The ->event_rings is set twice. In the mhi_ep_mmio_init function and in
the mhi_ep_mmio_update_ner function.
void mhi_ep_mmio_init(struct mhi_ep_cntrl *mhi_cntrl)
{
[...]
mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval);
[...]
}
void mhi_ep_mmio_update_ner(struct mhi_ep_cntrl *mhi_cntrl)
{
[...]
mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval);
[...]
}
But ->event_rings does not need to be allocated because the type is a u32.
struct mhi_ep_cntrl {
[...]
u32 event_rings;
[...]
};
So, I don't know what you are trying to explain to me. I'm sorry.
> 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.
Thanks a lot for the advices.
Regards,
Erick
> regards,
> dan carpenter
