linux-kernel - Re: [PATCH] mm/damon/sysfs-schemes: using kmalloc_array() and size

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 for Android: free password hash cracker in your pocket

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

Message-ID: <21407408-78e4-48eb-8296-fcddc702ae25@nfschina.com>
Date: Wed, 23 Apr 2025 10:04:56 +0800
From: Su Hui <suhui@...china.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 SeongJae Park <sj@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>
Cc: Su Hui <suhui@...china.com>, akpm@...ux-foundation.org,
 damon@...ts.linux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] mm/damon/sysfs-schemes: using kmalloc_array() and
 size_add()

On 2025/4/23 02:50, Christophe JAILLET wrote:
> Le 22/04/2025 à 20:23, SeongJae Park a écrit :
>> On Tue, 22 Apr 2025 13:44:39 +0300 Dan Carpenter 
>> <dan.carpenter@...aro.org> wrote:
>>
>>> On Tue, Apr 22, 2025 at 01:38:05PM +0300, Dan Carpenter wrote:
>>>> On Mon, Apr 21, 2025 at 02:24:24PM +0800, Su Hui wrote:
>>>>> It's safer to using kmalloc_array() and size_add() because it can
>>>>> prevent possible overflow problem.
>>>>>
>>>>> Signed-off-by: Su Hui <suhui@...china.com>
>> [...]
>>>>> --- a/mm/damon/sysfs-schemes.c
>>>>> +++ b/mm/damon/sysfs-schemes.c
>>>>> @@ -465,7 +465,8 @@ static ssize_t memcg_path_store(struct kobject 
>>>>> *kobj,
>>>>>   {
>>>>>       struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>>>>>               struct damon_sysfs_scheme_filter, kobj);
>>>>> -    char *path = kmalloc(sizeof(*path) * (count + 1), GFP_KERNEL);
>>>>> +    char *path = kmalloc_array(size_add(count, 1), sizeof(*path),
>>>>> +                   GFP_KERNEL);
>>>>
>>>> Count is clamped in rw_verify_area().
>>>>
>>>> Smatch does a kind of ugly hack to handle rw_verify_area() which is 
>>>> that
>>>> it says neither the count nor the pos can be more than 1G. And 
>>>> obviously
>>>> files which are larger than 2GB exist but pretending they don't 
>>>> silences
>>>> all these integer overflow warnings.
>>>>
>>>
>>> Actually rw_verify_area() ensures that "pos + count" can't 
>>> overflow.  But
>>> here we are multiplying.  Fortunately, we are multiplying by 1 so 
>>> that's
>>> safe and also count can't be larger than PAGE_SIZE here which is 
>>> safe as
>>> well.
>>
>> Thank you for adding these details, Dan.  I understand the size_add() 
>> change
>> can make warnings slience, though it is not really fixing a real 
>> bug.  So I
>> believe there is no action item to make a change to this patch. Maybe 
>> making
>> the commit message more clarified can be helpful, though?
>>
>> Please let me know if I'm misunderstanding your point and/or you want 
>> some
>> changes.
>
> As sizeof(*path) = 1, maybe, just change it to:
>     char *path = kmalloc(count + 1, GFP_KERNEL);
Maybe nothing should change?
Thanks for Dan's explanation again.
I send this patch because  it is mentioned in 
Documentation/process/deprecated.rst that:

"
Dynamic size calculations (especially multiplication) should not be
performed in memory allocator (or similar) function arguments due to the
risk of them overflowing.
"

Although in this case, this dynamic size calculations is  safe.
But it's also fine for me to change this patch or discard this patch 
because there is no
bug really fixed.

Su Hui