linux-kernel - Re: [PATCH -next] nbd: get config_lock before sock

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: <1b67a9dd-c28a-661a-3a46-dab509d4c34e@kernel.dk>
Date:   Mon, 31 Jul 2023 18:27:57 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Zhong Jinghua <zhongjinghua@...weicloud.com>, josef@...icpanda.com
Cc:     linux-block@...r.kernel.org, nbd@...er.debian.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yukuai3@...wei.com
Subject: Re: [PATCH -next] nbd: get config_lock before sock_shutdown

On 7/7/23 12:22?AM, Zhong Jinghua wrote:
> Config->socks in sock_shutdown may trigger a UAF problem.
> The reason is that sock_shutdown does not hold the config_lock,
> so that nbd_ioctl can release config->socks at this time.
> 
> T0: NBD_SET_SOCK
> T1: NBD_DO_IT
> 
> T0						T1
> 
> nbd_ioctl
>   mutex_lock(&nbd->config_lock)
>   // get lock
>   __nbd_ioctl
>     nbd_start_device_ioctl
>       nbd_start_device
>        mutex_unlock(&nbd->config_lock)
>          // relase lock
>          wait_event_interruptible
>          (kill, enter sock_shutdown)
>          sock_shutdown
> 					nbd_ioctl
> 					  mutex_lock(&nbd->config_lock)
> 					  // get lock
> 					  __nbd_ioctl
> 					    nbd_add_socket
> 					      krealloc
> 						kfree(p)
> 					        //config->socks is NULL
>            nbd_sock *nsock = config->socks // error
> 
> Fix it by moving config_lock up before sock_shutdown.
> 
> Signed-off-by: Zhong Jinghua <zhongjinghua@...weicloud.com>
> ---
>  drivers/block/nbd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index c410cf29fb0c..accbe99ebb7e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1428,13 +1428,18 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd)
>  	mutex_unlock(&nbd->config_lock);
>  	ret = wait_event_interruptible(config->recv_wq,
>  					 atomic_read(&config->recv_threads) == 0);
> +
> +	/*
> +	 * recv_work in flush_workqueue will not get this lock, because nbd_open
> +	 * will hold nbd->config_refs
> +	 */
> +	mutex_lock(&nbd->config_lock);
>  	if (ret) {
>  		sock_shutdown(nbd);
>  		nbd_clear_que(nbd);
>  	}
>  
>  	flush_workqueue(nbd->recv_workq);
> -	mutex_lock(&nbd->config_lock);

Feels pretty iffy to hold config_lock over the flush. If anything off
recv_work() ever grabs it, we'd be stuck. Your comment assumes that the
only case this will currently happen is if we drop the last ref, or at
least that's the case that'd do it even if you don't mention it
explicitly.

Maybe this is all fine, but recv_work() should have a comment matching
this one, and this comment should be more descriptive as well.

-- 
Jens Axboe