linux-hardening - Re: [PATCH 2/2] mfd: omap-usb-tll: use struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5bfebb11-eee1-4c1e-bb2a-7c3b27d2af7b@gmail.com>
Date: Wed, 26 Jun 2024 19:26:42 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Tony Lindgren <tony@...mide.com>, Kees Cook <kees@...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 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@...il.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
>>
>