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

[Dan Carpenter <dan.carpenter@linaro.org>]

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