[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9e654a02-4a6a-47ef-bef6-263866a4941f@gmail.com>
Date: Wed, 26 Jun 2024 21:03:27 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Kees Cook <kees@...nel.org>
Cc: Tony Lindgren <tony@...mide.com>, Lee Jones <lee@...nel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>, linux-omap@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
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@...il.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@...nel.org>
>
> -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
>>
>
Powered by blists - more mailing lists