linux-kernel - Re: [PATCH] libsubcmd: Fix null intersection case in exclude

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUs8jD_qLJGjLBiVheu6JzynmmYSmfr_9PgKmxK4qEwVg@mail.gmail.com>
Date: Sun, 7 Dec 2025 14:16:13 -0800
From: Ian Rogers <irogers@...gle.com>
To: Sri Jayaramappa <sjayaram@...mai.com>, Guilherme Amadio <amadio@...too.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Joshua Hunt <johunt@...mai.com>
Subject: Re: [PATCH] libsubcmd: Fix null intersection case in exclude_cmds()

On Tue, Dec 2, 2025 at 1:40 PM Sri Jayaramappa <sjayaram@...mai.com> wrote:
>
> When there is no exclusion occurring from the cmds list - for example -
> cmds contains ["read-vdso32"] and excludes contains ["archive"] - the
> main loop completes with ci == cj == 0. In the original code the loop
> processing the remaining elements in the list was conditional:
>
>     if (ci != cj) { ...}
>
> So we end up in the assertion loop since ci < cmds->cnt and we
> incorrectly try to assert the list elements to be NULL and fail with
> the following error
>
>    help.c:104: exclude_cmds: Assertion `cmds->names[ci] == NULL' failed.
>
> Fix this by moving the if (ci != cj) check inside of a broader loop.
> If ci != cj, left shift the list elements, as before, and then
> unconditionally advance the ci and cj indicies which also covers the
> ci == cj case.
>
> Fixes: 1fdf938168c4d26f ("perf tools: Fix use-after-free in help_unknown_cmd()")
>
> Signed-off-by: Sri Jayaramappa <sjayaram@...mai.com>

Thanks Sri! Guilherme reported a related issue and I think your fix covers it:
https://lore.kernel.org/lkml/aTXQw9dtP5df7LmP@gentoo.org/

I came up with the following test based on your commit message, could
you validate it covers the case for your fix:
```
diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c
index 2280b4c0e5e7..9da96a16fd20 100644
--- a/tools/perf/tests/subcmd-help.c
+++ b/tools/perf/tests/subcmd-help.c
@@ -95,10 +95,36 @@ static int test__exclude_cmdnames(struct
test_suite *test __maybe_unused,
       return TEST_OK;
}

+static int test__exclude_cmdnames_no_overlap(struct test_suite *test
__maybe_unused,
+                                            int subtest __maybe_unused)
+{
+       struct cmdnames cmds1 = {};
+       struct cmdnames cmds2 = {};
+
+       add_cmdname(&cmds1, "read-vdso32", 11);
+       add_cmdname(&cmds2, "archive", 7);
+
+       TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 1);
+       TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 1);
+
+       exclude_cmds(&cmds1, &cmds2);
+
+       TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 1);
+       TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 1);
+
+       TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1,
"read-vdso32") == 1);
+       TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "archive") == 0);
+
+       clean_cmdnames(&cmds1);
+       clean_cmdnames(&cmds2);
+       return TEST_OK;
+}
+
static struct test_case tests__subcmd_help[] = {
       TEST_CASE("Load subcmd names", load_cmdnames),
       TEST_CASE("Uniquify subcmd names", uniq_cmdnames),
       TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames),
+       TEST_CASE("Exclude disjoint subcmd names", exclude_cmdnames_no_overlap),
       {       .name = NULL, }
};
```
If you build perf you can run the test like:
```
$ perf test -v help
 68: libsubcmd help tests                                            :
 68.1: Load subcmd names                                             : Ok
 68.2: Uniquify subcmd names                                         : Ok
 68.3: Exclude duplicate subcmd names                                : Ok
 68.4: Exclude disjoint subcmd names                                 : Ok
```
Perhaps you can think of other values for the test so we don't run
into more issues.

Thanks,
Ian

> ---
>  tools/lib/subcmd/help.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c
> index ddaeb4eb3e24..db94aa685b73 100644
> --- a/tools/lib/subcmd/help.c
> +++ b/tools/lib/subcmd/help.c
> @@ -97,11 +97,13 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
>                         ei++;
>                 }
>         }
> -       if (ci != cj) {
> -               while (ci < cmds->cnt) {
> -                       cmds->names[cj++] = cmds->names[ci];
> -                       cmds->names[ci++] = NULL;
> +       while (ci < cmds->cnt) {
> +               if (ci != cj) {
> +                       cmds->names[cj] = cmds->names[ci];
> +                       cmds->names[ci] = NULL;
>                 }
> +               ci++;
> +               cj++;
>         }
>         for (ci = cj; ci < cmds->cnt; ci++)
>                 assert(cmds->names[ci] == NULL);
> --
> 2.34.1
>