linux-kernel - Re: [PATCH v2] perf/ring_buffer: Fix exposing a temporarily decreased data

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190517084712.GL2623@hirez.programming.kicks-ass.net>
Date:   Fri, 17 May 2019 10:47:12 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Yabin Cui <yabinc@...gle.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] perf/ring_buffer: Fix exposing a temporarily
 decreased data_head.

On Thu, May 16, 2019 at 11:40:10AM -0700, Yabin Cui wrote:
> In perf_output_put_handle(), an IRQ/NMI can happen in below location and
> write records to the same ring buffer:
> 	...
> 	local_dec_and_test(&rb->nest)
> 	...                          <-- an IRQ/NMI can happen here
> 	rb->user_page->data_head = head;
> 	...
> 
> In this case, a value A is written to data_head in the IRQ, then a value
> B is written to data_head after the IRQ. And A > B. As a result,
> data_head is temporarily decreased from A to B. And a reader may see
> data_head < data_tail if it read the buffer frequently enough, which
> creates unexpected behaviors.
> 
> This can be fixed by moving dec(&rb->nest) to after updating data_head,
> which prevents the IRQ/NMI above from updating data_head.
> 
> Signed-off-by: Yabin Cui <yabinc@...gle.com>
> ---
> 
> v1 -> v2: change rb->nest from local_t to unsigned int, and add barriers.
> 
> ---
>  kernel/events/internal.h    |  2 +-
>  kernel/events/ring_buffer.c | 24 ++++++++++++++++++------
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
> index 79c47076700a..0a8c003b9bcf 100644
> --- a/kernel/events/internal.h
> +++ b/kernel/events/internal.h
> @@ -24,7 +24,7 @@ struct ring_buffer {
>  	atomic_t			poll;		/* POLL_ for wakeups */
>  
>  	local_t				head;		/* write position    */
> -	local_t				nest;		/* nested writers    */
> +	unsigned int			nest;		/* nested writers    */
>  	local_t				events;		/* event limit       */
>  	local_t				wakeup;		/* wakeup stamp      */
>  	local_t				lost;		/* nr records lost   */
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 674b35383491..c677beb01fb1 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -38,7 +38,8 @@ static void perf_output_get_handle(struct perf_output_handle *handle)
>  	struct ring_buffer *rb = handle->rb;
>  
>  	preempt_disable();
> -	local_inc(&rb->nest);
> +	rb->nest++;
> +	barrier();

Urgh; almost but not quite. You just lost the 'volatile' qualifier and
now the compiler can mess things up for you.

>  	handle->wakeup = local_read(&rb->wakeup);
>  }

What I'm going to do is split this into two patches, one fixes the
problem and marked for backport, and one changing away from local_t.