| Message ID | 20240120152518.13006-1-erick.archer@gmx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bus: mhi: ep: Use kcalloc() instead of kzalloc() | expand |
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
On 1/20/24 09:25, 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. > > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/162 > Signed-off-by: Erick Archer <erick.archer@gmx.com> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks!
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
On Sun, Jan 28, 2024 at 11:29:33AM +0100, Erick Archer wrote: > > 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. > It's not about the type. The event_rings struct member is the number of elements in the mhi_cntrl->mhi_event array. However, we ->event_rings without re-allocating mhi_cntrl->mhi_event so those are not in sync any more. So since we don't know the number of elements in the mhi_cntrl->mhi_event array leading to memory corruption. > 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); > [...] > } These ->event_rings assignments look exactly the same. It depends on regval. So possibly one could be deleted. regards, dan carpenter
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
Hi Dan, On Mon, Jan 29, 2024 at 08:20:26AM +0300, Dan Carpenter wrote: > On Sun, Jan 28, 2024 at 11:29:33AM +0100, Erick Archer wrote: > > > 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. > > > > It's not about the type. > > The event_rings struct member is the number of elements in the > mhi_cntrl->mhi_event array. However, we ->event_rings without > re-allocating mhi_cntrl->mhi_event so those are not in sync any more. > So since we don't know the number of elements in the mhi_cntrl->mhi_event > array leading to memory corruption. Thanks for this clarification. Now I understand what you are explaining to me. Regards, Erick
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index 65fc1d738bec..8d7a4102bdb7 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -1149,8 +1149,9 @@ int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) mhi_ep_mmio_mask_interrupts(mhi_cntrl); mhi_ep_mmio_init(mhi_cntrl); - mhi_cntrl->mhi_event = kzalloc(mhi_cntrl->event_rings * (sizeof(*mhi_cntrl->mhi_event)), - GFP_KERNEL); + mhi_cntrl->mhi_event = kcalloc(mhi_cntrl->event_rings, + sizeof(*mhi_cntrl->mhi_event), + GFP_KERNEL); if (!mhi_cntrl->mhi_event) return -ENOMEM;
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. Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/162 Signed-off-by: Erick Archer <erick.archer@gmx.com> --- drivers/bus/mhi/ep/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.25.1