Messages in this thread | | | Subject | Re: [PATCH -next] nbd: get config_lock before sock_shutdown | From | Yu Kuai <> | Date | Thu, 28 Sep 2023 14:04:36 +0800 |
| |
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
>
| |