lkml.org 
[lkml]   [2023]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH -next] nbd: get config_lock before sock_shutdown
From

在 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.

\
 
 \ /
  Last update: 2023-10-30 09:50    [W:3.809 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site