Messages in this thread | | | Date | Mon, 30 Oct 2023 16:49:50 +0800 | Subject | Re: [PATCH -next] nbd: get config_lock before sock_shutdown | From | zhongjinghua <> |
| |
在 2023/9/28 14:04, Yu Kuai 写道: > Hi, > > 在 2023/08/01 8:27, Jens Axboe 写道: >> 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@huaweicloud.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. > > Jinghua, > > Please add comment as Jens suggested, and resend this patch. > > Thanks, > Kuai > >> OK.
Later I'll send out,
Thanks to Jens for the advice.
| |