Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

[Kees Cook <kees@kernel.org>]

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@gmail.com>
> 
> ---

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@kernel.org>

-Kees

> ---
>  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
> 

-- 
Kees Cook