NAK: [PATCH 1/2] blktrace: Protect q->blk_trace with RCU

[Andrea Righi]

On Tue, Mar 31, 2020 at 08:44:43AM -0400, Benjamin M Romer wrote:
> From: Jan Kara <jack at suse.cz>
> 
> KASAN is reporting that __blk_add_trace() has a use-after-free issue
> when accessing q->blk_trace. Indeed the switching of block tracing (and
> thus eventual freeing of q->blk_trace) is completely unsynchronized with
> the currently running tracing and thus it can happen that the blk_trace
> structure is being freed just while __blk_add_trace() works on it.
> Protect accesses to q->blk_trace by RCU during tracing and make sure we
> wait for the end of RCU grace period when shutting down tracing. Luckily
> that is rare enough event that we can afford that. Note that postponing
> the freeing of blk_trace to an RCU callback should better be avoided as
> it could have unexpected user visible side-effects as debugfs files
> would be still existing for a short while block tracing has been shut
> down.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=205711
> CC: stable at vger.kernel.org
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> Reviewed-by: Ming Lei <ming.lei at redhat.com>
> Tested-by: Ming Lei <ming.lei at redhat.com>
> Reviewed-by: Bart Van Assche <bvanassche at acm.org>
> Reported-by: Tristan Madani <tristmd at gmail.com>
> Signed-off-by: Jan Kara <jack at suse.cz>
> Signed-off-by: Jens Axboe <axboe at kernel.dk>
> 
> CVE-2019-19768
> (cherry picked from commit c780e86dd48ef6467a1146cf7d0fe1e05a635039)
> [ ben_r: adjusted patch to apply to ubuntu ]

Maybe we should use "backported from commit <sha1>"?

>  static void blk_add_trace_rq_abort(void *ignore,
> @@ -785,13 +794,19 @@ static void blk_add_trace_rq_complete(void *ignore,
>  static void blk_add_trace_bio(struct request_queue *q, struct bio *bio,
>  			      u32 what, int error)
>  {
> -	struct blk_trace *bt = q->blk_trace;
> +	struct blk_trace *bt;
>  
>  	if (likely(!bt))

I think there's an error here. This "if" line should be removed,
otherwise we're just checking random data in the stack.

> +	rcu_read_lock();
> +	bt = rcu_dereference(q->blk_trace);
> +	if (likely(!bt)) {
> +		rcu_read_unlock();
>  		return;
> +	}
>  
>  	__blk_add_trace(bt, bio->bi_iter.bi_sector, bio->bi_iter.bi_size,
>  			bio->bi_rw, what, error, 0, NULL);
> +	rcu_read_unlock();
>  }

Thanks,
-Andrea