Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <ab998dda-80ba-7d8b-0cae-36665826deb5@huaweicloud.com>
Date:   Mon, 30 Oct 2023 16:49:50 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH -next] nbd: get config_lock before sock_shutdown
To:     Yu Kuai <yukuai1@huaweicloud.com>, Jens Axboe <axboe@kernel.dk>,
        josef@toxicpanda.com
Cc:     linux-block@vger.kernel.org, nbd@other.debian.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20230707062256.1271948-1-zhongjinghua@huaweicloud.com>
 <1b67a9dd-c28a-661a-3a46-dab509d4c34e@kernel.dk>
 <ae49487b-9aa8-fe37-792b-676f7e70b23b@huaweicloud.com>
From:   zhongjinghua <zhongjinghua@huaweicloud.com>
In-Reply-To: <ae49487b-9aa8-fe37-792b-676f7e70b23b@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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