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 <aryabi...@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);

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Reply via email to