Re: [PATCH] mm/damon/sysfs-schemes: fix use-after-free on memcg_path and goal path

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The sysfs show functions `memcg_path_show()` and `path_show()` read string pointers without holding `damon_sysfs_lock`, leaving a Use-After-Free vulnerability.
--

commit a381c30634b7e486fce5448c7aae9e23da7aecf9
Author: Junxi Qian <qjx1298677004@gmail.com>

mm/damon/sysfs-schemes: fix use-after-free on memcg_path and goal path

This commit fixes a use-after-free race between the kdamond thread and
sysfs store operations. It does this by acquiring damon_sysfs_lock while
swapping the memcg_path and path string pointers, and freeing the old
pointers outside the lock.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 5186966dafb35..005cfecb11c5d 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
[ ... ]
> @@ -543,15 +543,20 @@ static ssize_t memcg_path_store(struct kobject *kobj,
[ ... ]
>  	strscpy(path, buf, count + 1);
> -	kfree(filter->memcg_path);
> +
> +	mutex_lock(&damon_sysfs_lock);
> +	old = filter->memcg_path;
>  	filter->memcg_path = path;
> +	mutex_unlock(&damon_sysfs_lock);
> +
> +	kfree(old);

Could this explicit free race with the lockless read in memcg_path_show()?

The VFS and sysfs core layers do not serialize concurrent show and store
operations for the same attribute. If one thread calls memcg_path_show(),
it evaluates the pointer without holding damon_sysfs_lock:

static ssize_t memcg_path_show(...)
{
        ...
        return sysfs_emit(buf, "%s\n",
                          filter->memcg_path ? filter->memcg_path : "");
}

Can a concurrent store operation swap the pointer and kfree() the old one
just before sysfs_emit() dereferences it? Does memcg_path_show() also need
to acquire damon_sysfs_lock?

[ ... ]
> @@ -1196,15 +1201,20 @@ static ssize_t path_store(struct kobject *kobj,
[ ... ]
>  	strscpy(path, buf, count + 1);
> -	kfree(goal->path);
> +
> +	mutex_lock(&damon_sysfs_lock);
> +	old = goal->path;
>  	goal->path = path;
> +	mutex_unlock(&damon_sysfs_lock);
> +
> +	kfree(old);

Similarly, does path_show() have the same vulnerability?

static ssize_t path_show(...)
{
        ...
        return sysfs_emit(buf, "%s\n", goal->path ? goal->path : "");
}

Since goal->path is accessed locklessly here, can a concurrent path_store()
free the string right before path_show() reads it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420085332.178473-1-qjx1298677004@gmail.com?part=1