| Message ID | 20240120152518.13006-1-erick.archer@gmx.com |
|---|---|
| State | Accepted |
| Commit | ae1d892d518af5c092f2b1f8e6921996c6a95cb3 |
| Headers | show |
| Series | bus: mhi: ep: Use kcalloc() instead of kzalloc() | expand |
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
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