[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20250905013940.94255-1-sj@kernel.org>
Date: Thu, 4 Sep 2025 18:39:40 -0700
From: SeongJae Park <sj@...nel.org>
To: Stanislav Fort <stanislav.fort@...le.com>
Cc: SeongJae Park <sj@...nel.org>,
damon@...ts.linux.dev,
linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
Stanislav Fort <disclosure@...le.com>
Subject: Re: [PATCH] mm/damon/sysfs: fix use-after-free in state_show()
On Thu, 4 Sep 2025 20:55:49 +0300 Stanislav Fort <stanislav.fort@...le.com> wrote:
> state_show() currently reads kdamond->damon_ctx without holding
> damon_sysfs_lock. This creates a use-after-free race condition:
>
> CPU 0 CPU 1
> ----- -----
> state_show() damon_sysfs_turn_damon_on()
> ctx = kdamond->damon_ctx;
> mutex_lock(&damon_sysfs_lock);
> damon_destroy_ctx(kdamond->damon_ctx);
> kdamond->damon_ctx = NULL;
> mutex_unlock(&damon_sysfs_lock);
> damon_is_running(ctx); /* ctx is freed */
> mutex_lock(&ctx->kdamond_lock); /* UAF */
>
> The race can occur with other functions that free or replace the context
> while holding damon_sysfs_lock, such as damon_sysfs_kdamonds_rm_dirs()
> and damon_sysfs_kdamond_release().
>
> Fix this by acquiring damon_sysfs_lock before accessing the context,
> mirroring the locking pattern used in pid_show().
>
> This vulnerability was present when state_show() was first introduced to
> access kdamond->damon_ctx.
Nice catch, thank you!
checkpatch.pl complains as below, though:
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#34:
Reported-by: Stanislav Fort <disclosure@...le.com>
Signed-off-by: Stanislav Fort <disclosure@...le.com>
ERROR: patch seems to be corrupt (line wrapped?)
#77: FILE: mm/damon/sysfs.c:1279:
2.34.1
WARNING: From:/Signed-off-by: email address mismatch: 'From: Stanislav Fort <stanislav.fort@...le.com>' != 'Signed-off-by: Stanislav Fort <disclosure@...le.com>'
I know the reporting was made in a non-public mailing list. Could you please
add a context though, e.g.,
Closes: N/A # non-publicly reported
The second and third ones should be properly fixed.
>
> Fixes: a61ea561c871 ("mm/damon/sysfs: link DAMON for virtual address spaces monitoring")
> Reported-by: Stanislav Fort <disclosure@...le.com>
> Signed-off-by: Stanislav Fort <disclosure@...le.com>
> ---
> mm/damon/sysfs.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 1234567..abcdef0 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1258,17 +1258,24 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> struct damon_sysfs_kdamond *kdamond = container_of(kobj,
> struct damon_sysfs_kdamond, kobj);
> - struct damon_ctx *ctx = kdamond->damon_ctx;
> - bool running;
> + struct damon_ctx *ctx;
> + bool running = false;
> +
> + if (!mutex_trylock(&damon_sysfs_lock))
> + return -EBUSY;
> +
> + ctx = kdamond->damon_ctx;
> + if (ctx)
> + running = damon_is_running(ctx);
>
> - if (!ctx)
> - running = false;
> - else
> - running = damon_is_running(ctx);
> + mutex_unlock(&damon_sysfs_lock);
>
> return sysfs_emit(buf, "%s\n", running ?
> damon_sysfs_cmd_strs[DAMON_SYSFS_CMD_ON] :
> damon_sysfs_cmd_strs[DAMON_SYSFS_CMD_OFF]);
> }
Other than the checkpatch issue, the change looks good to me.
Thanks,
SJ
[...]
Powered by blists - more mailing lists