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] [day] [month] [year] [list]

Date:   Mon, 30 Oct 2023 16:49:50 +0800
From:   zhongjinghua <zhongjinghua@...weicloud.com>
To:     Yu Kuai <yukuai1@...weicloud.com>, Jens Axboe <axboe@...nel.dk>,
        josef@...icpanda.com
Cc:     linux-block@...r.kernel.org, nbd@...er.debian.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next] nbd: get config_lock before sock_shutdown


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