Re: [PATCH v2 1/2] nbd: fix use-after-freed crash for nbd->recv_workq

[Xiubo Li <xiubli@redhat.com>]

On 2020/11/3 12:12, Ming Lei wrote:
> On Mon, Nov 02, 2020 at 10:07:57PM -0500, xiubli@redhat.com wrote:
>> 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 has reached zero and the
>> related resource has been released, if another process tries to
>> send the DISCONENCT cmd by using the netlink, it will potentially
>> crash like this.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   drivers/block/nbd.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index f46e26c9d9b3..3bb8281bb753 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -2003,16 +2003,31 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
>>   
>>   static void nbd_disconnect_and_put(struct nbd_device *nbd)
>>   {
>> +	bool flush = true;
>> +
>>   	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.
>> +	 * In very rare case that the nbd->recv_workq may already have been
>> +	 * released by the recv_work().
>>   	 */
>> -	flush_workqueue(nbd->recv_workq);
>> +	if (likely(!nbd->recv_workq))
> It is actually unlikely, but doesn't make any sense to annotate the check
> since it isn't fast path, so please remove it.

Sure, will do it.

Thanks.


>
>> +		refcount_inc(&nbd->config_refs);
>> +	else
>> +		flush = false;
>> +	mutex_unlock(&nbd->config_lock);
>> +
>> +	if (flush) {
>> +		/*
>> +		 * 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);
>> +		nbd_config_put(nbd);
>> +	}
>> +
>>   	if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF,
>>   			       &nbd->config->runtime_flags))
>>   		nbd_config_put(nbd);
> The approach is fine, and the reason is that nbd_disconnect_and_put() still
> can be called via netlink when the nbd device has been closed:
>
> Once the above likely() is removed, feel free to add:
>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>
>
>
> Thanks,
> Ming
>