linux-kernel - Re: [PATCH] bcache: fix NULL pointer reference in cached_dev_detach

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: <5f6cc0b9-50d0-de90-3a52-09abb959b991@wangsu.com>
Date:   Wed, 10 Nov 2021 14:15:26 +0800
From:   Lin Feng <linf@...gsu.com>
To:     colyli@...e.de, kent.overstreet@...il.com
Cc:     linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bcache: fix NULL pointer reference in
 cached_dev_detach_finish

Hi Coly,

Since previous patch(faulted) has been merged into upstream, this issue
has to be fix ASAP, else kernel will crash once detach runs, I'm so sorry
for my mistake!

Thanks,
LinFeng

On 11/9/21 12:13, Lin Feng wrote:
> Commit 0259d4498ba484("bcache: move calc_cached_dev_sectors to proper
> place on backing device detach") tries to fix calc_cached_dev_sectors
> when bcache device detaches, but now we have:
> 
> cached_dev_detach_finish
>      ...
>      bcache_device_detach(&dc->disk);
>          ...
>          closure_put(&d->c->caching);
>          d->c = NULL; [*explicitly set dc->disk.c to NULL*]
>      list_move(&dc->list, &uncached_devices);
>      calc_cached_dev_sectors(dc->disk.c); [*passing a NULL pointer*]
>      ...
> 
> Upper codeflows shows how bug happens, this patch fix the problem by
> caching dc->disk.c beforehand, and cache_set won't be freed under us
> because c->caching closure at least holds a reference count and closure
> callback __cache_set_unregister only being called by bch_cache_set_stop
> which using closure_queue(&c->caching), that means c->caching closure
> callback for destroying cache_set won't be trigger by previous
> closure_put(&d->c->caching).
> So at this stage(while cached_dev_detach_finish is calling) it's safe to
> access cache_set dc->disk.c.
> 
> Fixes: 0259d4498ba484("bcache: move calc_cached_dev_sectors to proper place on backing device detach")
> Signed-off-by: Lin Feng <linf@...gsu.com>
> ---
>   drivers/md/bcache/super.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 4a9a65dff95e..3d9bc7cd27f8 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1139,6 +1139,7 @@ static void cancel_writeback_rate_update_dwork(struct cached_dev *dc)
>   static void cached_dev_detach_finish(struct work_struct *w)
>   {
>   	struct cached_dev *dc = container_of(w, struct cached_dev, detach);
> +	struct cache_set *c = dc->disk.c;
>   
>   	BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags));
>   	BUG_ON(refcount_read(&dc->count));
> @@ -1156,7 +1157,7 @@ static void cached_dev_detach_finish(struct work_struct *w)
>   
>   	bcache_device_detach(&dc->disk);
>   	list_move(&dc->list, &uncached_devices);
> -	calc_cached_dev_sectors(dc->disk.c);
> +	calc_cached_dev_sectors(c);
>   
>   	clear_bit(BCACHE_DEV_DETACHING, &dc->disk.flags);
>   	clear_bit(BCACHE_DEV_UNLINK_DONE, &dc->disk.flags);
>