Messages in this thread | | | Subject | Re: [PATCH] nbd: pass nbd_sock to nbd_read_reply() instead of index | From | Yu Kuai <> | Date | Thu, 28 Sep 2023 17:40:42 +0800 |
| |
Hi,
在 2023/09/28 17:24, Ming Lei 写道: > On Thu, Sep 28, 2023 at 05:06:40PM +0800, Yu Kuai wrote: >> Hi, >> >> 在 2023/09/28 16:57, Ming Lei 写道: >>> On Thu, Sep 28, 2023 at 04:55:03PM +0800, Yu Kuai wrote: >>>> Hi, >>>> >>>> 在 2023/09/28 15:40, Ming Lei 写道: >>>>> On Thu, Sep 28, 2023 at 02:03:28PM +0800, Yu Kuai wrote: >>>>>> Hi, >>>>>> >>>>>> 在 2023/09/28 12:05, Ming Lei 写道: >>>>>>> On Mon, Sep 11, 2023 at 10:33:08AM +0800, linan666@huaweicloud.com wrote: >>>>>>>> From: Li Nan <linan122@huawei.com> >>>>>>>> >>>>>>>> If a socket is processing ioctl 'NBD_SET_SOCK', config->socks might be >>>>>>>> krealloc in nbd_add_socket(), and a garbage request is received now, a UAF >>>>>>>> may occurs. >>>>>>>> >>>>>>>> T1 >>>>>>>> nbd_ioctl >>>>>>>> __nbd_ioctl >>>>>>>> nbd_add_socket >>>>>>>> blk_mq_freeze_queue >>>>>>>> T2 >>>>>>>> recv_work >>>>>>>> nbd_read_reply >>>>>>>> sock_xmit >>>>>>>> krealloc config->socks >>>>>>>> def config->socks >>>>>>>> >>>>>>>> Pass nbd_sock to nbd_read_reply(). And introduce a new function >>>>>>>> sock_xmit_recv(), which differs from sock_xmit only in the way it get >>>>>>>> socket. >>>>>>>> >>>>>>> >>>>>>> I am wondering why not grab queue usage counter before calling nbd_read_reply() >>>>>>> for avoiding such issue, something like the following change: >>>>>>> >>>>>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >>>>>>> index df1cd0f718b8..09215b605b12 100644 >>>>>>> --- a/drivers/block/nbd.c >>>>>>> +++ b/drivers/block/nbd.c >>>>>>> @@ -837,9 +837,6 @@ static void recv_work(struct work_struct *work) >>>>>>> while (1) { >>>>>>> struct nbd_reply reply; >>>>>>> - if (nbd_read_reply(nbd, args->index, &reply)) >>>>>>> - break; >>>>>>> - >>>>>>> /* >>>>>>> * Grab .q_usage_counter so request pool won't go away, then no >>>>>>> * request use-after-free is possible during nbd_handle_reply(). >>>>>>> @@ -852,6 +849,9 @@ static void recv_work(struct work_struct *work) >>>>>>> break; >>>>>>> } >>>>>> >>>>>> This break how nbd works, if there is no reply yet, recv_work() will >>>>>> wait for reply in: >>>>>> >>>>>> nbd_read_reply >>>>>> sock_xmit >>>>>> sock_recvmsg >>>>>> >>>>>> After this change, recv_work() will just return if there is no io. >>>>> >>>>> OK, got it, thanks for the input. >>>>> >>>>> But I feel it isn't necessary & fragile to store one extra reference of nsock in >>>>> `recv_thread_args`. >>>>> >>>>> Just run a quick look, the only potential UAF on config->socks should be recv_work(), >>>>> so you can retrieve the `nsock` reference at the entry of recv_work(), >>>> >>>> I don't understand what you mean retrieve the 'nsock', is following what >>>> you expected? >>>> >>>> blk_queue_enter() -> prevent concurrent with nbd_add_socket >>>> nsock = config->socks[args->index] >>>> blk_queue_exit() >>> >>> Yeah, turns out you do understand, :-) >> >> Ok, I was not sure about this blk_queue_enter(). By the way, this > > blk_queue_enter() isn't exported, but you can grab ->config_lock > for getting the `nsock`. > >> remind me of what you did to fix uaf of access queue->mq_hctx[] by >> convert the array to xarray. >> >> >> Maybe it's better to covert config->socks[] to xarray to fix this uaf as >> well? > > ->socks[idx] is needed in nbd fast path, so xarray may not be one good idea > since xarray_load() introduces extra load, especially ->socks[] uaf > should exist in recv_work() very likely. For other cases, the active > block request holds queue usage counter.
Thanks for the explanation, grab 'config_lock' to get 'nsock' in the begining sounds good to me.
Kuai
> > > Thanks, > Ming > > . >
| |