On 26/06/2024 20:43, Kees Cook wrote: > On Thu, Jun 20, 2024 at 11:22:34PM +0200, Javier Carrasco wrote: >> Use the struct_size macro to calculate the size of the tll, which >> includes a trailing flexible array. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> >> >> --- > > I would actually include this entire bit below in the main commit log. > It's the core of the "why" for this patch. > >> The memory allocation used to be carried out in two steps: >> >> tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); >> tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch, >> GFP_KERNEL); >> >> Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") >> turned that into the current allocation: >> >> tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]), >> GFP_KERNEL); >> >> That has surprised me at first glance because I would have expected >> sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not >> being equivalent to 'sizeof(struct clk *) * nch'. >> >> I might be missing/misunderstanding something here because the commit >> is not new, and the error should be noticeable. Moreover, I don't have >> real hardware to test it. Hence why I didn't mark this patch as a fix. >> >> I would be pleased to get feedback about this (why it is right as it is, >> or if that is actually a bug). > > Yeah, I would include: > > Fixes: commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") > > Because that was a clear mistake. I suspect they were intending to do > this, which I've seen as a code pattern from time to time: > > devm_kzalloc(dev, offsetof(typeof(*tll), ch_clk[nch])); > > But as Jann points out, "nch" is so small: > > drivers/mfd/omap-usb-tll.c:81:#define OMAP_REV2_TLL_CHANNEL_COUNT 2 > drivers/mfd/omap-usb-tll.c:82:#define OMAP_TLL_CHANNEL_COUNT 3 > drivers/mfd/omap-usb-tll.c:220: nch = OMAP_TLL_CHANNEL_COUNT; > drivers/mfd/omap-usb-tll.c:224: nch = OMAP_REV2_TLL_CHANNEL_COUNT; > > struct usbtll_omap { > void __iomem *base; > int nch; /* num. of channels */ > struct clk *ch_clk[]; /* must be the last member */ > }; > > That this allocation was asking for 4 + 4 + 4 * 1 (12) instead of: > 4 + 4 + 4 * OMAP_TLL_CHANNEL_COUNT (20) > or > 4 + 4 + 4 * OMAP_REV2_TLL_CHANNEL_COUNT (16) > > the latter would have ended up in the same kmalloc bucket (12 would be > rounded up to 16), but with the ARM alignment issue, the minimum bucket > size would effectively be tied to CONFIG_ARM_L1_CACHE_SHIFT, which could > be as low as 5: 32 bytes minimum, so no bug to be had in the real > world. > > Reviewed-by: Kees Cook <kees@xxxxxxxxxx> > > -Kees > Thanks for the accurate clarification. That explains indeed why the bug went unnoticed. A few more channels or members in the usbtll_omap structure would have triggered some alarms. I will address your comments for v2. >> --- >> drivers/mfd/omap-usb-tll.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c >> index a091e5b0f21d..5f25ac514ff2 100644 >> --- a/drivers/mfd/omap-usb-tll.c >> +++ b/drivers/mfd/omap-usb-tll.c >> @@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev) >> break; >> } >> >> - tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]), >> - GFP_KERNEL); >> + tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL); >> if (!tll) { >> pm_runtime_put_sync(dev); >> pm_runtime_disable(dev); >> >> -- >> 2.40.1 >> >