mm/damon/sysfs-schemes: using kmalloc_array() and size_add()

[PATCH] mm/damon/sysfs-schemes: using kmalloc_array() and size_add()

Posted by Su Hui 7 months, 3 weeks ago

It's safer to using kmalloc_array() and size_add() because it can
prevent possible overflow problem.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 mm/damon/sysfs-schemes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 23b562df0839..79220aba436f 100644
--- 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);
 
 	if (!path)
 		return -ENOMEM;
@@ -2035,7 +2036,7 @@ static int damon_sysfs_memcg_path_to_id(char *memcg_path, unsigned short *id)
 	if (!memcg_path)
 		return -EINVAL;
 
-	path = kmalloc(sizeof(*path) * PATH_MAX, GFP_KERNEL);
+	path = kmalloc_array(PATH_MAX, sizeof(*path), GFP_KERNEL);
 	if (!path)
 		return -ENOMEM;
 
-- 
2.30.2

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

Posted by Dan Carpenter 7 months, 3 weeks ago

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@nfschina.com>
> ---
>  mm/damon/sysfs-schemes.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 23b562df0839..79220aba436f 100644
> --- 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.

>  
>  	if (!path)
>  		return -ENOMEM;
> @@ -2035,7 +2036,7 @@ static int damon_sysfs_memcg_path_to_id(char *memcg_path, unsigned short *id)
>  	if (!memcg_path)
>  		return -EINVAL;
>  
> -	path = kmalloc(sizeof(*path) * PATH_MAX, GFP_KERNEL);
> +	path = kmalloc_array(PATH_MAX, sizeof(*path), GFP_KERNEL);

If we boost PATH_MAX to that high then we're going to run into
all sorts of other issues first.

regards,
dan carpenter

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

Posted by Dan Carpenter 7 months, 3 weeks ago

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@nfschina.com>
> > ---
> >  mm/damon/sysfs-schemes.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > index 23b562df0839..79220aba436f 100644
> > --- 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.

regards,
dan carpenter

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

Posted by SeongJae Park 7 months, 3 weeks ago

On Tue, 22 Apr 2025 13:44:39 +0300 Dan Carpenter <dan.carpenter@linaro.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@nfschina.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.


Thanks,
SJ

> 
> regards,
> dan carpenter

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

Posted by Christophe JAILLET 7 months, 3 weeks ago

Le 22/04/2025 à 20:23, SeongJae Park a écrit :
> On Tue, 22 Apr 2025 13:44:39 +0300 Dan Carpenter <dan.carpenter@linaro.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@nfschina.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);

CJ
> 
> 
> Thanks,
> SJ
> 
>>
>> regards,
>> dan carpenter
> 
>

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

Posted by Su Hui 7 months, 3 weeks ago

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@linaro.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@nfschina.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

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

Posted by Dan Carpenter 7 months, 3 weeks ago

On Wed, Apr 23, 2025 at 10:04:56AM +0800, Su Hui wrote:
> 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@linaro.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@nfschina.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?

Yeah.  No need to change.  Sysfs buffers are always a page size and
count is <= PAGE_SIZE.  Generally, it's one of the pieces of trivia
that people should know.  That's how sysfs_emit() works.

regards,
dan carpenter

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

Posted by SeongJae Park 7 months, 3 weeks ago

On Mon, 21 Apr 2025 14:24:24 +0800 Su Hui <suhui@nfschina.com> wrote:

> It's safer to using kmalloc_array() and size_add() because it can
> prevent possible overflow problem.

Nice finding, thank you!

> 
> Signed-off-by: Su Hui <suhui@nfschina.com>

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]