[Devel] [PATCH rh7] block/nbd: Fix NULL pointer dereference

[Kirill Tkhai]

On 11.09.2019 15:45, Andrey Ryabinin wrote:
> The following commands trigger NULL-ptr dereference in ioctl(NBD_DO_IT):
> 
> 	$ modprobe nbd
> 	$ qemu-img create -f qcow2 xxx 10G
> 	$ while true; do qemu-nbd -v -f qcow2  --detect-zeroes=on xxx -r -c /dev/nbd0 --cache=none --aio=native; done &
> 	$ while true; do qemu-nbd -d /dev/nbd0; done &
> 
>  BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
>  IP: [<ffffffffc0a2ae73>] __nbd_ioctl+0x343/0x970 [nbd]
> 
>  Call Trace:
>    nbd_ioctl+0x6a/0x1a0 [nbd]
>    blkdev_ioctl+0x2ea/0xa30
>    block_ioctl+0x41/0x50
>    do_vfs_ioctl+0x3b0/0x5a0
>    SyS_ioctl+0xa1/0xc0
>    system_call_fastpath+0x22/0x27
> 
> NBD_DO_IT unlocks nbd->tx_lock and accesses nbd->sock in nbd_do_it();
> Parallel ioctl(NBD_CLEAR_SOCK) nullifies nbd->sock which might cause
> NULL-ptr deref in nbd_do_it().
> 
> Fix the issue by taking nbd->tx_lock in nbd_do_it() to access nbd->sock.
> This should protect us from parallel NBD_CLEAR_SOCK.
> 
> https://jira.sw.ru/browse/PSBM-97690
> Signed-off-by: Andrey Ryabinin <aryabinin at virtuozzo.com>
> ---
>  drivers/block/nbd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index e0c6b623585d..2452b49efd56 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -419,7 +419,15 @@ static int nbd_do_it(struct nbd_device *nbd)
>  
>  	BUG_ON(nbd->magic != NBD_MAGIC);
>  
> +	mutex_lock(&nbd->tx_lock);
> +	if (!nbd->sock) {
> +		mutex_unlock(&nbd->tx_lock);
> +		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
> +		return -EINVAL;
> +	}
>  	sk_set_memalloc(nbd->sock->sk);
> +	mutex_unlock(&nbd->tx_lock);
> +
>  	nbd->pid = task_pid_nr(current);
>  	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
>  	if (ret) {

The problem may be a bit bigger, then only zero nbd->sock. NBD_CLEAR_SOCK also clears
nbd->file and makes some other cleanup. It may switch the driver in inconsistent state.
nbd_do_it() may access freed memory in case of clear happens in parallel. I'd proposed
something like this:

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e0c6b623585d..6d1c0229280a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -420,11 +420,9 @@ static int nbd_do_it(struct nbd_device *nbd)
 	BUG_ON(nbd->magic != NBD_MAGIC);
 
 	sk_set_memalloc(nbd->sock->sk);
-	nbd->pid = task_pid_nr(current);
 	ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
 	if (ret) {
 		dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
-		nbd->pid = 0;
 		return ret;
 	}
 
@@ -432,7 +430,6 @@ static int nbd_do_it(struct nbd_device *nbd)
 		nbd_end_request(req);
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
-	nbd->pid = 0;
 	return 0;
 }
 
@@ -631,7 +628,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  
 	case NBD_CLEAR_SOCK: {
 		struct file *file;
+		int ret;
 
+		while (nbd->pid) {
+			mutex_unlock(&nbd->tx_lock);
+			ret = wait_event_interruptible(!nbd->pid);
+			mutex_lock(&nbd->tx_lock);
+			if (ret)
+				return ret;
+		}
 		nbd->sock = NULL;
 		file = nbd->file;
 		nbd->file = NULL;
@@ -705,6 +710,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (!nbd->file)
 			return -EINVAL;
 
+		nbd->pid = task_pid_nr(current);
 		mutex_unlock(&nbd->tx_lock);
 
 		if (nbd->flags & NBD_FLAG_READ_ONLY)
@@ -721,6 +727,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 					nbd->disk->disk_name);
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
+			nbd->pid = 0;
 			return PTR_ERR(thread);
 		}
 		wake_up_process(thread);
@@ -728,6 +735,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		kthread_stop(thread);
 
 		mutex_lock(&nbd->tx_lock);
+		nbd->pid = 0;
 		if (error)
 			return error;
 		sock_shutdown(nbd, 0);