linux-kernel - Re: [PATCH v1] perf test: Fix maps use after put

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZEEgwKYMaA9iTgFA@kernel.org>
Date:   Thu, 20 Apr 2023 08:23:44 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf test: Fix maps use after put

Em Wed, Apr 19, 2023 at 08:04:30PM -0700, Ian Rogers escreveu:
> Fix a use after put reference count issue. maps is copied from leader,
> but the leader is put on line 79 and then maps is used to read the
> reference count below - so a use after put, with the put of maps
> happening within thread__put. Fix by reversing the order of puts so
> that the leader is put last.
> 
> To explain the reference count checker, I wrote this up as a little
> example here:
> https://perf.wiki.kernel.org/index.php/Reference_Count_Checking
> 
> Note, the bug was introduced by the committer and wasn't present in
> the original reference count patch set.

Yes, the bug predated your patch and is detected by the reference count
checking you contributed. 

This was just part of splitting up your series into smaller chunks, in
this case either we fix the problem detected while developing this
reference counting infrastructure or fix it later after the merged
infrastructure, when built with EXTRA_CFLAGS="-DREFCNT_CHECKING=1"
detects it when running 'perf test'.

Thanks for providing the separate patch fixing it.

Applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>  tools/perf/tests/thread-maps-share.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
> index 75ce8aedfc78..858e725318a9 100644
> --- a/tools/perf/tests/thread-maps-share.c
> +++ b/tools/perf/tests/thread-maps-share.c
> @@ -76,16 +76,16 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s
>  	TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps));
>  
>  	/* release thread group */
> -	thread__put(leader);
> +	thread__put(t3);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3);
>  
> -	thread__put(t1);
> +	thread__put(t2);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2);
>  
> -	thread__put(t2);
> +	thread__put(t1);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1);
>  
> -	thread__put(t3);
> +	thread__put(leader);
>  
>  	/* release other group  */
>  	thread__put(other_leader);
> -- 
> 2.40.0.634.g4ca3ef3211-goog
> 

-- 

- Arnaldo