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

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

[SeongJae Park]

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

[...]

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

[Dan Carpenter]

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()

[Dan Carpenter]

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()

[SeongJae Park]

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()

[Christophe JAILLET]

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()

[Dan Carpenter]

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