linux-kernel - Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+aCy1HczXwRxhMSghXMij1uREWTO_EgK9t0HKKFhh0paSKfeA@mail.gmail.com>
Date:	Sat, 16 Jul 2016 13:12:53 +0530
From:	Pranay Srivastava <pranjas@...il.com>
To:	Markus Pargmann <mpa@...gutronix.de>
Cc:	nbd-general@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	Wouter Verhelst <w@...r.be>
Subject: Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

Hi,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@...gutronix.de> wrote:
> After NBD_DO_IT exited the block device may still be used. Make sure
> that we handle intended disconnects differently and do not allow any
> changed of the nbd device.
>
> This patch should avoid that the nbd-client connects to a different server
> and the users of the block device are suddenly reading/writing from a
> different backend device.
>
> For timeouts it is still possible to setup a new socket so that the
> connection may be refreshed without creating problems for all users.

But Shouldn't time out be checked for last end point?

For example, consider the following steps

1) Timeout occurs but server[nbd-s1] comes up again albeit with a different
    network address.

2) The previous network address of server [nbd-s1] has now been assigned to
    another new nbd server [nbd-s2]

3) A new nbd-client tries to setup the socket again, Negotiation would
be done again
    [correct?]. If correct then wouldn't we be sending data to wrong
device this time?

So instead can't we put a mechanism in place for network address + mac
to be same
for allowing clients to reconnect? Do let me know if this is not of concern.

4) If 3) doesn't apply then let's disallow all ioctls until nbd device
is reset.

>
> Signed-off-by: Markus Pargmann <mpa@...gutronix.de>
> ---
>  drivers/block/nbd.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 620660f3ff0f..39358efac73e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -708,6 +708,18 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                        unsigned int cmd, unsigned long arg)
>  {
> +       /*
> +        * After a disconnect was instructed, do not allow any further actions
> +        * on the block device that would lead to a new connected endpoint.
> +        * This condition stays until nbd_reset was called either because all
> +        * users closed the device or because of CLEAR_SOCK.
> +        */
> +       if (nbd->disconnect &&
> +           cmd != NBD_CLEAR_SOCK && cmd != NBD_PRINT_DEBUG) {
> +               dev_info(disk_to_dev(nbd->disk), "Device is still busy after instructing a disconnect\n");
> +               return -EBUSY;
> +       }
> +
>         switch (cmd) {
>         case NBD_DISCONNECT: {
>                 struct request sreq;
> @@ -733,11 +745,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>         }
>
>         case NBD_CLEAR_SOCK:
> -               sock_shutdown(nbd);
> -               nbd_clear_que(nbd);
> -               BUG_ON(!list_empty(&nbd->queue_head));
> -               BUG_ON(!list_empty(&nbd->waiting_queue));
> -               kill_bdev(bdev);
> +               if (nbd->disconnect) {
> +                       nbd_reset(nbd);
> +               } else {
> +                       sock_shutdown(nbd);
> +                       nbd_clear_que(nbd);
> +                       BUG_ON(!list_empty(&nbd->queue_head));
> +                       BUG_ON(!list_empty(&nbd->waiting_queue));
> +                       kill_bdev(bdev);
> +               }
>                 return 0;
>
>         case NBD_SET_SOCK: {
> @@ -812,8 +828,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 mutex_lock(&nbd->tx_lock);
>                 nbd->task_recv = NULL;
>
> -               if (nbd->disconnect) /* user requested, ignore socket errors */
> +               if (nbd->disconnect) { /* user requested, ignore socket errors */
> +                       sock_shutdown(nbd);
>                         error = 0;
> +               }
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>
> --
> 2.1.4
>



-- 
        ---P.K.S