Re: [PATCH 2/4] mm/damon/sysfs: check contexts->nr before clear_schemes_tried_regions

[Josh Law]

On 20 March 2026 14:47:40 GMT, SeongJae Park <sj@xxxxxxxxxx> wrote:
>On Fri, 20 Mar 2026 07:06:48 +0000 Josh Law <objecting@xxxxxxxxxxxxx> wrote:
>
>> 
>> 
>> On 20 March 2026 02:13:17 GMT, SeongJae Park <sj@xxxxxxxxxx> wrote:
>> >On Thu, 19 Mar 2026 15:57:40 +0000 Josh Law <objecting@xxxxxxxxxxxxx> wrote:
>> >
>> >> The CLEAR_SCHEMES_TRIED_REGIONS command accesses contexts_arr[0]
>> >> without verifying nr_contexts >= 1, causing a NULL pointer dereference
>> >> when no context is configured. Add the missing check.
>> >
>> >Nice catch, thank you!
>> >
>> >Privileged users can trigger this using DAMON sysfs interface.  E.g.,
>> >
>> >    # cd /sys/kernel/mm/damon/admin/kdamonds/
>> >    # echo 1 > nr_kdamonds
>> >    # echo clear_schemes_tried_regions > state
>> >    killed
>> >    # dmesg
>> >    [...]
>> >    [63541.377604] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> >    [...]
>> >
>> >Privileged users can do anything even worse than this, but they might also do
>> >this by a mistake.
>> >
>> >So this deserves Fixes: and Cc stable.
>> >
>> >>
>> >> Signed-off-by: Josh Law <objecting@xxxxxxxxxxxxx>
>> >> ---
>> >>  mm/damon/sysfs.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
>> >> index b573b9d60784..36ad2e8956c9 100644
>> >> --- a/mm/damon/sysfs.c
>> >> +++ b/mm/damon/sysfs.c
>> >> @@ -1769,6 +1769,8 @@ static int damon_sysfs_handle_cmd(enum damon_sysfs_cmd cmd,
>> >>       case DAMON_SYSFS_CMD_UPDATE_SCHEMES_TRIED_REGIONS:
>> >>               return damon_sysfs_update_schemes_tried_regions(kdamond, false);
>> >>       case DAMON_SYSFS_CMD_CLEAR_SCHEMES_TRIED_REGIONS:
>> >> +             if (kdamond->contexts->nr != 1)
>> >> +                     return -EINVAL;
>> >>               return damon_sysfs_schemes_clear_regions(
>> >>                       kdamond->contexts->contexts_arr[0]->schemes);
>> >>       case DAMON_SYSFS_CMD_UPDATE_SCHEMES_EFFECTIVE_QUOTAS:
>> >> --
>> >> 2.34.1
>> >
>> >So this patch looks good as an individual fix for the individual bug, but...
>> >
>> >Sashiko commented.
>> >
>> ># review url: https://sashiko.dev/#/patchset/20260319155742.186627-3-objecting@xxxxxxxxxxxxx
>> >
>> >: Does this missing check also affect other manual commands?
>> >:
>> >: If a user writes UPDATE_SCHEMES_STATS, UPDATE_SCHEMES_EFFECTIVE_QUOTAS,
>> >: or UPDATE_TUNED_INTERVALS to the state file after setting nr_contexts
>> >: to 0, damon_sysfs_handle_cmd() queues the corresponding callback via
>> >: damon_sysfs_damon_call().
>> >:
>> >: When the kdamond thread executes the callback, it appears functions like
>> >: damon_sysfs_upd_schemes_stats() access contexts_arr[0] without verifying
>> >: contexts->nr:
>> >:
>> >: static int damon_sysfs_upd_schemes_stats(void *data)
>> >: {
>> >:         struct damon_sysfs_kdamond *kdamond = data;
>> >:         struct damon_ctx *ctx = kdamond->damon_ctx;
>> >:
>> >:         damon_sysfs_schemes_update_stats(
>> >:                         kdamond->contexts->contexts_arr[0]->schemes, ctx);
>> >:         return 0;
>> >: }
>> >:
>> >: Could this result in a similar NULL pointer dereference if these commands
>> >: are triggered while no context is configured?
>> >
>> >Sashiko is correct.  Privileged users can trigger the issues like below.
>> >
>> ># damo start
>> ># cd /sys/kernel/mm/damon/admin/kdamonds/0
>> ># echo 0 > contexts/nr_contexts
>> ># echo update_schemes_stats > state
>> ># echo update_schemes_effective_quotas > state
>> ># echo update_tuned_intervals > state
>> >
>> >Not necessarily blocker of this patch, but seems all the issues are in a same
>> >category.  The third patch of this series is also fixing one of the category
>> >bugs.  How about fixing all at once by checking kdamond->contexts->nr at the
>> >beginning of damon_sysfs_handle_cmd(), like below?
>> >
>> >--- a/mm/damon/sysfs.c
>> >+++ b/mm/damon/sysfs.c
>> >@@ -2404,6 +2404,9 @@ static int damon_sysfs_update_schemes_tried_regions(
>> > static int damon_sysfs_handle_cmd(enum damon_sysfs_cmd cmd,
>> >                struct damon_sysfs_kdamond *kdamond)
>> > {
>> >+       if (cmd != DAMON_SYSFS_CMD_OFF && kdamond->contexts->nr != 1)
>> >+               return -EINVAL;
>> >+
>> >        switch (cmd) {
>> >        case DAMON_SYSFS_CMD_ON:
>> >                return damon_sysfs_turn_damon_on(kdamond);
>> >
>> >If we pick this, Fixes: would be deserve to the oldest buggy commit that
>> >introduced the first bug of this category.  It is indeed quite old.
>> >
>> >Fixes: 0ac32b8affb5 ("mm/damon/sysfs: support DAMOS stats")
>> >Cc: <stable@xxxxxxxxxxxxxxx> # 5.18.x
>> >
>> >
>> >Thanks,
>> >SJ
>> 
>> 
>> 
>> Hello, did you give Reviewed by you? Or not..
>
>Are you meaning Reviewed-by: tag?  If so, no, not yet.  I want to get your
>answer to above question first.  Could you please answer?
>
>
>Thanks,
>SJ
>
>[...]


Well, two is in the same catagory. But seperate fixes may be best.  Because patch 3 dont call that function, so it may be screwy, i mean, if you want me to. Ill guard it. But its a bit on the hacky side


V/R


Josh Law