* [PATCH -next] nbd: get config_lock before sock_shutdown
@ 2023-07-07 6:22 Zhong Jinghua
2023-07-28 7:10 ` Yu Kuai
2023-08-01 0:27 ` Jens Axboe
0 siblings, 2 replies; 5+ messages in thread
From: Zhong Jinghua @ 2023-07-07 6:22 UTC (permalink / raw)
To: josef, axboe
Cc: linux-block, nbd, linux-kernel, zhongjinghua, yi.zhang, yukuai3
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);
nbd_bdev_reset(nbd);
/* user requested, ignore socket errors */
if (test_bit(NBD_RT_DISCONNECT_REQUESTED, &config->runtime_flags))
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH -next] nbd: get config_lock before sock_shutdown
2023-07-07 6:22 [PATCH -next] nbd: get config_lock before sock_shutdown Zhong Jinghua
@ 2023-07-28 7:10 ` Yu Kuai
2023-08-01 0:27 ` Jens Axboe
1 sibling, 0 replies; 5+ messages in thread
From: Yu Kuai @ 2023-07-28 7:10 UTC (permalink / raw)
To: Zhong Jinghua, josef, axboe
Cc: linux-block, nbd, linux-kernel, yi.zhang, yukuai (C)
在 2023/07/07 14:22, Zhong Jinghua 写道:
> 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.
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
>
> 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);
> nbd_bdev_reset(nbd);
> /* user requested, ignore socket errors */
> if (test_bit(NBD_RT_DISCONNECT_REQUESTED, &config->runtime_flags))
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH -next] nbd: get config_lock before sock_shutdown
2023-07-07 6:22 [PATCH -next] nbd: get config_lock before sock_shutdown Zhong Jinghua
2023-07-28 7:10 ` Yu Kuai
@ 2023-08-01 0:27 ` Jens Axboe
2023-09-28 6:04 ` Yu Kuai
1 sibling, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2023-08-01 0:27 UTC (permalink / raw)
To: Zhong Jinghua, josef; +Cc: linux-block, nbd, linux-kernel, yi.zhang, yukuai3
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH -next] nbd: get config_lock before sock_shutdown
2023-08-01 0:27 ` Jens Axboe
@ 2023-09-28 6:04 ` Yu Kuai
2023-10-30 8:49 ` zhongjinghua
0 siblings, 1 reply; 5+ messages in thread
From: Yu Kuai @ 2023-09-28 6:04 UTC (permalink / raw)
To: Jens Axboe, Zhong Jinghua, josef
Cc: linux-block, nbd, linux-kernel, yi.zhang, yukuai (C)
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH -next] nbd: get config_lock before sock_shutdown
2023-09-28 6:04 ` Yu Kuai
@ 2023-10-30 8:49 ` zhongjinghua
0 siblings, 0 replies; 5+ messages in thread
From: zhongjinghua @ 2023-10-30 8:49 UTC (permalink / raw)
To: Yu Kuai, Jens Axboe, josef
Cc: linux-block, nbd, linux-kernel, yi.zhang, yukuai (C)
在 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-30 8:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-07 6:22 [PATCH -next] nbd: get config_lock before sock_shutdown Zhong Jinghua
2023-07-28 7:10 ` Yu Kuai
2023-08-01 0:27 ` Jens Axboe
2023-09-28 6:04 ` Yu Kuai
2023-10-30 8:49 ` zhongjinghua
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).