* Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
2024-06-20 21:22 ` [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll Javier Carrasco
@ 2024-06-26 15:26 ` Lee Jones
2024-06-26 17:26 ` Javier Carrasco
2024-06-26 17:57 ` Jann Horn
2024-06-26 18:43 ` Kees Cook
2 siblings, 1 reply; 9+ messages in thread
From: Lee Jones @ 2024-06-26 15:26 UTC (permalink / raw)
To: Javier Carrasco
Cc: Tony Lindgren, Kees Cook, Gustavo A. R. Silva, linux-omap,
linux-kernel, linux-hardening
On Thu, 20 Jun 2024, 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>
>
> ---
> 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).
You don't need this H/W to test this our for yourself.
Mock-up the structs in a user-space C-program and print out the sizes.
Please report them all to justify the patch.
> ---
> 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
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
2024-06-26 15:26 ` Lee Jones
@ 2024-06-26 17:26 ` Javier Carrasco
0 siblings, 0 replies; 9+ messages in thread
From: Javier Carrasco @ 2024-06-26 17:26 UTC (permalink / raw)
To: Lee Jones
Cc: Tony Lindgren, Kees Cook, Gustavo A. R. Silva, linux-omap,
linux-kernel, linux-hardening
On 26/06/2024 17:26, Lee Jones wrote:
> On Thu, 20 Jun 2024, 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>
>>
>> ---
>> 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).
>
> You don't need this H/W to test this our for yourself.
>
> Mock-up the structs in a user-space C-program and print out the sizes.
>
> Please report them all to justify the patch.
>
Values obviously depend on the architecture, but in general:
1.- Before commit 16c2004d9e4d:
1.1. tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL);
-> sizeof(struct usbtll_omap) = N
1.2 tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
GFP_KERNEL);
-> sizeof(struct clk *) * tll->nch = M * nch
Total = N + M * nch,
where M is the size of a single pointer.
2.- After commit 16c2004d9e4d:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
GFP_KERNEL);
-> sizeof(*tll) = N
-> sizeof(tll->ch_clk[nch]) = sizeof(struct clk *) = M
Total = N + M
Therefore, it only allocates memory for a single pointer.
3.- struct_size (this patch):
sizeof(*tll) + nch * sizeof(struct clk *) = N + nch * M
What I meant with not having real hardware is that I could not replicate
the whole code with all their structures to get exact sizes, which don't
leave room for discussion or misunderstandings.
Best regards,
Javier Carrasco
>> ---
>> 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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
2024-06-20 21:22 ` [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll Javier Carrasco
2024-06-26 15:26 ` Lee Jones
@ 2024-06-26 17:57 ` Jann Horn
2024-06-26 18:43 ` Kees Cook
2 siblings, 0 replies; 9+ messages in thread
From: Jann Horn @ 2024-06-26 17:57 UTC (permalink / raw)
To: Javier Carrasco, Markus Elfring, Ladislav Michl, Roger Quadros,
Lee Jones
Cc: Tony Lindgren, Kees Cook, Gustavo A. R. Silva, linux-omap,
linux-kernel, linux-hardening
On Thu, Jun 20, 2024 at 11:23 PM Javier Carrasco
<javier.carrasco.cruz@gmail.com> 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>
>
> ---
> 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).
I might be a good idea to ask the people who wrote / accepted the
patch that seems to have introduced the bug? I've CC'ed them.
The kernel slab allocator enforces a minimum alignment of
KMALLOC_MIN_SIZE, which on MIPS seems to be based on the size of L1
cache lines or something like that? This allocator alignment might be
enough to prevent the bug from actually causing an observable effect
in many configurations - that could explain why it went undetected,
assuming that nobody tested this in a KASAN build.
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
2024-06-20 21:22 ` [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll Javier Carrasco
2024-06-26 15:26 ` Lee Jones
2024-06-26 17:57 ` Jann Horn
@ 2024-06-26 18:43 ` Kees Cook
2024-06-26 19:03 ` Javier Carrasco
2 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2024-06-26 18:43 UTC (permalink / raw)
To: Javier Carrasco
Cc: Tony Lindgren, Lee Jones, Gustavo A. R. Silva, linux-omap,
linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll
2024-06-26 18:43 ` Kees Cook
@ 2024-06-26 19:03 ` Javier Carrasco
0 siblings, 0 replies; 9+ messages in thread
From: Javier Carrasco @ 2024-06-26 19:03 UTC (permalink / raw)
To: Kees Cook
Cc: Tony Lindgren, Lee Jones, Gustavo A. R. Silva, linux-omap,
linux-kernel, linux-hardening
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@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
>
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread