* [PATCH 1/2] nbd: fix use-after-freed crash for nbd->recv_workq
2020-10-20 8:16 [PATCH 0/2] nbd: fix use-after-freed and double lock bugs xiubli
@ 2020-10-20 8:16 ` xiubli
2020-10-20 8:16 ` [PATCH 2/2] nbd: fix double lock for nbd->config_lock xiubli
1 sibling, 0 replies; 3+ messages in thread
From: xiubli @ 2020-10-20 8:16 UTC (permalink / raw)
To: josef, axboe; +Cc: nbd, linux-block, jdillama, mgolub, Xiubo Li
From: Xiubo Li <xiubli@redhat.com>
The crash call trace:
<6>[ 1012.319386] block nbd1: NBD_DISCONNECT
<1>[ 1012.319437] BUG: kernel NULL pointer dereference, address: 0000000000000020
<1>[ 1012.319439] #PF: supervisor write access in kernel mode
<1>[ 1012.319441] #PF: error_code(0x0002) - not-present page
<6>[ 1012.319442] PGD 0 P4D 0
<4>[ 1012.319448] Oops: 0002 [#1] SMP NOPTI
<4>[ 1012.319454] CPU: 9 PID: 25111 Comm: rbd-nbd Tainted: G E 5.9.0+ #6
[...]
<4>[ 1012.319505] PKRU: 55555554
<4>[ 1012.319506] Call Trace:
<4>[ 1012.319560] flush_workqueue+0x81/0x440
<4>[ 1012.319598] nbd_disconnect_and_put+0x50/0x70 [nbd]
<4>[ 1012.319607] nbd_genl_disconnect+0xc7/0x170 [nbd]
<4>[ 1012.319627] genl_rcv_msg+0x1dd/0x2f9
<4>[ 1012.319642] ? genl_start+0x140/0x140
<4>[ 1012.319644] netlink_rcv_skb+0x49/0x110
<4>[ 1012.319649] genl_rcv+0x24/0x40
<4>[ 1012.319651] netlink_unicast+0x1a5/0x280
<4>[ 1012.319653] netlink_sendmsg+0x23d/0x470
<4>[ 1012.319667] sock_sendmsg+0x5b/0x60
<4>[ 1012.319676] ____sys_sendmsg+0x1ef/0x260
<4>[ 1012.319679] ? copy_msghdr_from_user+0x5c/0x90
<4>[ 1012.319680] ? ____sys_recvmsg+0xa5/0x180
<4>[ 1012.319682] ___sys_sendmsg+0x7c/0xc0
<4>[ 1012.319683] ? copy_msghdr_from_user+0x5c/0x90
<4>[ 1012.319685] ? ___sys_recvmsg+0x89/0xc0
<4>[ 1012.319692] ? __wake_up_common_lock+0x87/0xc0
<4>[ 1012.319715] ? __check_object_size+0x46/0x173
<4>[ 1012.319727] ? _copy_to_user+0x22/0x30
<4>[ 1012.319729] ? move_addr_to_user+0xc3/0x100
<4>[ 1012.319731] __sys_sendmsg+0x57/0xa0
<4>[ 1012.319744] do_syscall_64+0x33/0x40
<4>[ 1012.319760] entry_SYSCALL_64_after_hwframe+0x44/0xa9
<4>[ 1012.319780] RIP: 0033:0x7f5baa8e3ad5
In case the reference of nbd->config reached zero and trying to
free the related resource, including the nbd->recv_workq, if another
process send one DISCONENCT cmd by using the netlink, it will be
potentially crash like this.
This will the nbd->recv_workq under the nbd->config_lock.
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
drivers/block/nbd.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index edf8b632e3d2..3d3f4255e495 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1327,9 +1327,10 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd, struct block_device *b
atomic_read(&config->recv_threads) == 0);
if (ret)
sock_shutdown(nbd);
- flush_workqueue(nbd->recv_workq);
mutex_lock(&nbd->config_lock);
+ flush_workqueue(nbd->recv_workq);
+
nbd_bdev_reset(bdev);
/* user requested, ignore socket errors */
if (test_bit(NBD_RT_DISCONNECT_REQUESTED, &config->runtime_flags))
@@ -1339,6 +1340,18 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd, struct block_device *b
return ret;
}
+static void nbd_config_clear_rt_ref_and_put(struct nbd_device *nbd)
+{
+ lockdep_assert_held(&nbd->config_lock);
+
+ if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF,
+ &nbd->config->runtime_flags)) {
+ mutex_unlock(&nbd->config_lock);
+ nbd_config_put(nbd);
+ mutex_lock(&nbd->config_lock);
+ }
+}
+
static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
struct block_device *bdev)
{
@@ -2006,16 +2019,15 @@ static void nbd_disconnect_and_put(struct nbd_device *nbd)
mutex_lock(&nbd->config_lock);
nbd_disconnect(nbd);
nbd_clear_sock(nbd);
- mutex_unlock(&nbd->config_lock);
+
/*
* Make sure recv thread has finished, so it does not drop the last
* config ref and try to destroy the workqueue from inside the work
* queue.
*/
flush_workqueue(nbd->recv_workq);
- if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF,
- &nbd->config->runtime_flags))
- nbd_config_put(nbd);
+ nbd_config_clear_rt_ref_and_put(nbd);
+ mutex_unlock(&nbd->config_lock);
}
static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
--
2.18.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] nbd: fix double lock for nbd->config_lock
2020-10-20 8:16 [PATCH 0/2] nbd: fix use-after-freed and double lock bugs xiubli
2020-10-20 8:16 ` [PATCH 1/2] nbd: fix use-after-freed crash for nbd->recv_workq xiubli
@ 2020-10-20 8:16 ` xiubli
1 sibling, 0 replies; 3+ messages in thread
From: xiubli @ 2020-10-20 8:16 UTC (permalink / raw)
To: josef, axboe; +Cc: nbd, linux-block, jdillama, mgolub, Xiubo Li
From: Xiubo Li <xiubli@redhat.com>
When NBD_CLEAR_SOCK with ioctl method, it will double lock the
config_lock.
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
drivers/block/nbd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 3d3f4255e495..b21abf134e34 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1358,9 +1358,7 @@ static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
sock_shutdown(nbd);
__invalidate_device(bdev, true);
nbd_bdev_reset(bdev);
- if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF,
- &nbd->config->runtime_flags))
- nbd_config_put(nbd);
+ nbd_config_clear_rt_ref_and_put(nbd);
}
static bool nbd_is_valid_blksize(unsigned long blksize)
--
2.18.4
^ permalink raw reply related [flat|nested] 3+ messages in thread