[Qemu-devel] [PATCH 0/6] luks: Implement .bdrv_co_create

[Qemu-devel] [PATCH 0/6] luks: Implement .bdrv_co_create

[Kevin Wolf]

This series implements the .bdrv_co_create callback for luks, adds an
image creation test for it and contains some bonus fixes for bugs that
the test triggered.

Kevin Wolf (6):
  luks: Separate image file creation from formatting
  luks: Create block_crypto_co_create_generic()
  luks: Support .bdrv_co_create
  luks: Turn invalid assertion into check
  luks: Catch integer overflow for huge sizes
  qemu-iotests: Test luks QMP image creation

 qapi/block-core.json         |  17 +++-
 block/crypto.c               | 150 +++++++++++++++++++++---------
 tests/qemu-iotests/208       | 211 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/208.out   | 136 ++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |   2 +-
 tests/qemu-iotests/group     |   1 +
 6 files changed, 473 insertions(+), 44 deletions(-)
 create mode 100755 tests/qemu-iotests/208
 create mode 100644 tests/qemu-iotests/208.out

-- 
2.13.6

[Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

[Kevin Wolf]

The crypto driver used to create the image file in a callback from the
crypto subsystem. If we want to implement .bdrv_co_create, this needs to
go away because that callback will get a reference to an already
existing block node.

Move the image file creation to block_crypto_create_generic().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index e6095e7807..77871640cc 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
 
 
 struct BlockCryptoCreateData {
-    const char *filename;
-    QemuOpts *opts;
     BlockBackend *blk;
     uint64_t size;
 };
@@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
                                       Error **errp)
 {
     struct BlockCryptoCreateData *data = opaque;
-    int ret;
 
     /* User provided size should reflect amount of space made
      * available to the guest, so we must take account of that
      * which will be used by the crypto header
      */
-    data->size += headerlen;
-
-    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
-    ret = bdrv_create_file(data->filename, data->opts, errp);
-    if (ret < 0) {
-        return -1;
-    }
-
-    data->blk = blk_new_open(data->filename, NULL, NULL,
-                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
-    if (!data->blk) {
-        return -1;
-    }
-
-    return 0;
+    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
+                        errp);
 }
 
 
@@ -333,11 +317,10 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
     struct BlockCryptoCreateData data = {
         .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                          BDRV_SECTOR_SIZE),
-        .opts = opts,
-        .filename = filename,
     };
     QDict *cryptoopts;
 
+    /* Parse options */
     cryptoopts = qemu_opts_to_qdict(opts, NULL);
 
     create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
@@ -345,6 +328,20 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
         return -1;
     }
 
+    /* Create protocol layer */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    data.blk = blk_new_open(filename, NULL, NULL,
+                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+                            errp);
+    if (!data.blk) {
+        return -EINVAL;
+    }
+
+    /* Create format layer */
     crypto = qcrypto_block_create(create_opts, NULL,
                                   block_crypto_init_func,
                                   block_crypto_write_func,
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> The crypto driver used to create the image file in a callback from the
> crypto subsystem. If we want to implement .bdrv_co_create, this needs to
> go away because that callback will get a reference to an already
> existing block node.
> 
> Move the image file creation to block_crypto_create_generic().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/crypto.c | 37 +++++++++++++++++--------------------
>   1 file changed, 17 insertions(+), 20 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:08PM +0100, Kevin Wolf wrote:
> The crypto driver used to create the image file in a callback from the
> crypto subsystem. If we want to implement .bdrv_co_create, this needs to
> go away because that callback will get a reference to an already
> existing block node.
> 
> Move the image file creation to block_crypto_create_generic().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index e6095e7807..77871640cc 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
>  
>  
>  struct BlockCryptoCreateData {
> -    const char *filename;
> -    QemuOpts *opts;
>      BlockBackend *blk;
>      uint64_t size;
>  };
> @@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
>                                        Error **errp)
>  {
>      struct BlockCryptoCreateData *data = opaque;
> -    int ret;
>  
>      /* User provided size should reflect amount of space made
>       * available to the guest, so we must take account of that
>       * which will be used by the crypto header
>       */
> -    data->size += headerlen;
> -
> -    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
> -    ret = bdrv_create_file(data->filename, data->opts, errp);
> -    if (ret < 0) {
> -        return -1;
> -    }
> -
> -    data->blk = blk_new_open(data->filename, NULL, NULL,
> -                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
> -    if (!data->blk) {
> -        return -1;
> -    }
> -
> -    return 0;
> +    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
> +                        errp);
>  }

I wonder if we should pass in the value of the PREALLOC_MODE from the
block_crypto_create_generic() function via BlockCryptoCreateData, so
that we honour it. By using PREALLOC_MODE_OFF here, all LUKS volumes
created with PREALLOC_MODE_FULL are going to have several MB of
unallocated extents at the end of the file.

> 
>  
> @@ -333,11 +317,10 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
>      struct BlockCryptoCreateData data = {
>          .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>                           BDRV_SECTOR_SIZE),
> -        .opts = opts,
> -        .filename = filename,
>      };
>      QDict *cryptoopts;
>  
> +    /* Parse options */
>      cryptoopts = qemu_opts_to_qdict(opts, NULL);
>  
>      create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
> @@ -345,6 +328,20 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
>          return -1;
>      }
>  
> +    /* Create protocol layer */
> +    ret = bdrv_create_file(filename, opts, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    data.blk = blk_new_open(filename, NULL, NULL,
> +                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> +                            errp);
> +    if (!data.blk) {
> +        return -EINVAL;
> +    }
> +
> +    /* Create format layer */
>      crypto = qcrypto_block_create(create_opts, NULL,
>                                    block_crypto_init_func,
>                                    block_crypto_write_func,
> -- 
> 2.13.6
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

[Kevin Wolf]

Am 12.03.2018 um 12:35 hat Daniel P. Berrangé geschrieben:
> On Fri, Mar 09, 2018 at 06:27:08PM +0100, Kevin Wolf wrote:
> > The crypto driver used to create the image file in a callback from the
> > crypto subsystem. If we want to implement .bdrv_co_create, this needs to
> > go away because that callback will get a reference to an already
> > existing block node.
> > 
> > Move the image file creation to block_crypto_create_generic().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/crypto.c | 37 +++++++++++++++++--------------------
> >  1 file changed, 17 insertions(+), 20 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index e6095e7807..77871640cc 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
> >  
> >  
> >  struct BlockCryptoCreateData {
> > -    const char *filename;
> > -    QemuOpts *opts;
> >      BlockBackend *blk;
> >      uint64_t size;
> >  };
> > @@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
> >                                        Error **errp)
> >  {
> >      struct BlockCryptoCreateData *data = opaque;
> > -    int ret;
> >  
> >      /* User provided size should reflect amount of space made
> >       * available to the guest, so we must take account of that
> >       * which will be used by the crypto header
> >       */
> > -    data->size += headerlen;
> > -
> > -    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
> > -    ret = bdrv_create_file(data->filename, data->opts, errp);
> > -    if (ret < 0) {
> > -        return -1;
> > -    }
> > -
> > -    data->blk = blk_new_open(data->filename, NULL, NULL,
> > -                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
> > -    if (!data->blk) {
> > -        return -1;
> > -    }
> > -
> > -    return 0;
> > +    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
> > +                        errp);
> >  }
> 
> I wonder if we should pass in the value of the PREALLOC_MODE from the
> block_crypto_create_generic() function via BlockCryptoCreateData, so
> that we honour it. By using PREALLOC_MODE_OFF here, all LUKS volumes
> created with PREALLOC_MODE_FULL are going to have several MB of
> unallocated extents at the end of the file.

That would be an easy way to implement preallocation support. But you
can't create LUKS volumes with PREALLOC_MODE_FULL today because it
doesn't support preallocation at all (there's no option for it in
&block_crypto_create_opts_luks), so implementing that is matter for a
different patch.

Kevin

Re: [Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 12:43:47PM +0100, Kevin Wolf wrote:
> Am 12.03.2018 um 12:35 hat Daniel P. Berrangé geschrieben:
> > On Fri, Mar 09, 2018 at 06:27:08PM +0100, Kevin Wolf wrote:
> > > The crypto driver used to create the image file in a callback from the
> > > crypto subsystem. If we want to implement .bdrv_co_create, this needs to
> > > go away because that callback will get a reference to an already
> > > existing block node.
> > > 
> > > Move the image file creation to block_crypto_create_generic().
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/crypto.c | 37 +++++++++++++++++--------------------
> > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/block/crypto.c b/block/crypto.c
> > > index e6095e7807..77871640cc 100644
> > > --- a/block/crypto.c
> > > +++ b/block/crypto.c
> > > @@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
> > >  
> > >  
> > >  struct BlockCryptoCreateData {
> > > -    const char *filename;
> > > -    QemuOpts *opts;
> > >      BlockBackend *blk;
> > >      uint64_t size;
> > >  };
> > > @@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
> > >                                        Error **errp)
> > >  {
> > >      struct BlockCryptoCreateData *data = opaque;
> > > -    int ret;
> > >  
> > >      /* User provided size should reflect amount of space made
> > >       * available to the guest, so we must take account of that
> > >       * which will be used by the crypto header
> > >       */
> > > -    data->size += headerlen;
> > > -
> > > -    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
> > > -    ret = bdrv_create_file(data->filename, data->opts, errp);
> > > -    if (ret < 0) {
> > > -        return -1;
> > > -    }
> > > -
> > > -    data->blk = blk_new_open(data->filename, NULL, NULL,
> > > -                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
> > > -    if (!data->blk) {
> > > -        return -1;
> > > -    }
> > > -
> > > -    return 0;
> > > +    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
> > > +                        errp);
> > >  }
> > 
> > I wonder if we should pass in the value of the PREALLOC_MODE from the
> > block_crypto_create_generic() function via BlockCryptoCreateData, so
> > that we honour it. By using PREALLOC_MODE_OFF here, all LUKS volumes
> > created with PREALLOC_MODE_FULL are going to have several MB of
> > unallocated extents at the end of the file.
> 
> That would be an easy way to implement preallocation support. But you
> can't create LUKS volumes with PREALLOC_MODE_FULL today because it
> doesn't support preallocation at all (there's no option for it in
> &block_crypto_create_opts_luks), so implementing that is matter for a
> different patch.

Hah, ops, I forgot that ! In that case,

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 2/6] luks: Create block_crypto_co_create_generic()

[Kevin Wolf]

Everything that refers to the protocol layer or QemuOpts is moved out of
block_crypto_create_generic(), so that the remaining function is
suitable to be called by a .bdrv_co_create implementation.

LUKS is the only driver that actually implements the old interface, and
we don't intend to use it in any new drivers, so put the moved out code
directly into a LUKS function rather than creating a generic
intermediate one.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 77871640cc..b0a4cb3388 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -306,43 +306,29 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
 }
 
 
-static int block_crypto_create_generic(QCryptoBlockFormat format,
-                                       const char *filename,
-                                       QemuOpts *opts,
-                                       Error **errp)
+static int block_crypto_co_create_generic(BlockDriverState *bs,
+                                          int64_t size,
+                                          QCryptoBlockCreateOptions *opts,
+                                          Error **errp)
 {
-    int ret = -EINVAL;
-    QCryptoBlockCreateOptions *create_opts = NULL;
+    int ret;
+    BlockBackend *blk;
     QCryptoBlock *crypto = NULL;
-    struct BlockCryptoCreateData data = {
-        .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                         BDRV_SECTOR_SIZE),
-    };
-    QDict *cryptoopts;
-
-    /* Parse options */
-    cryptoopts = qemu_opts_to_qdict(opts, NULL);
+    struct BlockCryptoCreateData data;
 
-    create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
-    if (!create_opts) {
-        return -1;
-    }
+    blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
 
-    /* Create protocol layer */
-    ret = bdrv_create_file(filename, opts, errp);
+    ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
-        return ret;
+        goto cleanup;
     }
 
-    data.blk = blk_new_open(filename, NULL, NULL,
-                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                            errp);
-    if (!data.blk) {
-        return -EINVAL;
-    }
+    data = (struct BlockCryptoCreateData) {
+        .blk = blk,
+        .size = size,
+    };
 
-    /* Create format layer */
-    crypto = qcrypto_block_create(create_opts, NULL,
+    crypto = qcrypto_block_create(opts, NULL,
                                   block_crypto_init_func,
                                   block_crypto_write_func,
                                   &data,
@@ -355,10 +341,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
 
     ret = 0;
  cleanup:
-    QDECREF(cryptoopts);
     qcrypto_block_free(crypto);
-    blk_unref(data.blk);
-    qapi_free_QCryptoBlockCreateOptions(create_opts);
+    blk_unref(blk);
     return ret;
 }
 
@@ -563,8 +547,51 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
                                                          QemuOpts *opts,
                                                          Error **errp)
 {
-    return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS,
-                                       filename, opts, errp);
+    QCryptoBlockCreateOptions *create_opts = NULL;
+    BlockDriverState *bs = NULL;
+    QDict *cryptoopts;
+    int64_t size;
+    int ret;
+
+    /* Parse options */
+    size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+
+    cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
+                                             &block_crypto_create_opts_luks,
+                                             true);
+
+    create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
+                                                cryptoopts, errp);
+    if (!create_opts) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Create protocol layer */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Create format layer */
+    ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    bdrv_unref(bs);
+    qapi_free_QCryptoBlockCreateOptions(create_opts);
+    QDECREF(cryptoopts);
+    return ret;
 }
 
 static int block_crypto_get_info_luks(BlockDriverState *bs,
-- 
2.13.6

Re: [Qemu-devel] [PATCH 2/6] luks: Create block_crypto_co_create_generic()

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> Everything that refers to the protocol layer or QemuOpts is moved out of
> block_crypto_create_generic(), so that the remaining function is
> suitable to be called by a .bdrv_co_create implementation.
> 
> LUKS is the only driver that actually implements the old interface, and
> we don't intend to use it in any new drivers, so put the moved out code
> directly into a LUKS function rather than creating a generic
> intermediate one.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 61 insertions(+), 34 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/6] luks: Create block_crypto_co_create_generic()

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:09PM +0100, Kevin Wolf wrote:
> Everything that refers to the protocol layer or QemuOpts is moved out of
> block_crypto_create_generic(), so that the remaining function is
> suitable to be called by a .bdrv_co_create implementation.
> 
> LUKS is the only driver that actually implements the old interface, and
> we don't intend to use it in any new drivers, so put the moved out code
> directly into a LUKS function rather than creating a generic
> intermediate one.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 34 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

This adds the .bdrv_co_create driver callback to luks, which enables
image creation over QMP.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 17 ++++++++++++++++-
 block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 524d51567a..07039bfe9c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3452,6 +3452,21 @@
             '*preallocation':   'PreallocMode' } }
 
 ##
+# @BlockdevCreateOptionsLUKS:
+#
+# Driver specific image creation options for LUKS.
+#
+# @file             Node to create the image format on
+# @size             Size of the virtual disk in bytes
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsLUKS',
+  'data': { 'file':             'BlockdevRef',
+            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
+            'size':             'size' } }
+
+##
 # @BlockdevCreateOptionsNfs:
 #
 # Driver specific image creation options for NFS.
@@ -3643,7 +3658,7 @@
       'http':           'BlockdevCreateNotSupported',
       'https':          'BlockdevCreateNotSupported',
       'iscsi':          'BlockdevCreateNotSupported',
-      'luks':           'BlockdevCreateNotSupported',
+      'luks':           'BlockdevCreateOptionsLUKS',
       'nbd':            'BlockdevCreateNotSupported',
       'nfs':            'BlockdevCreateOptionsNfs',
       'null-aio':       'BlockdevCreateNotSupported',
diff --git a/block/crypto.c b/block/crypto.c
index b0a4cb3388..2035f9ab13 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -543,6 +543,39 @@ static int block_crypto_open_luks(BlockDriverState *bs,
                                      bs, options, flags, errp);
 }
 
+static int coroutine_fn
+block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
+{
+    BlockdevCreateOptionsLUKS *luks_opts;
+    BlockDriverState *bs = NULL;
+    QCryptoBlockCreateOptions create_opts;
+    int ret;
+
+    assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
+    luks_opts = &create_options->u.luks;
+
+    bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
+    if (bs == NULL) {
+        return -EIO;
+    }
+
+    create_opts = (QCryptoBlockCreateOptions) {
+        .format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
+        .u.luks = *luks_opts->qcrypto,
+    };
+
+    ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
+                                         errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    bdrv_unref(bs);
+    return ret;
+}
+
 static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
                                                          QemuOpts *opts,
                                                          Error **errp)
@@ -647,6 +680,7 @@ BlockDriver bdrv_crypto_luks = {
     .bdrv_open          = block_crypto_open_luks,
     .bdrv_close         = block_crypto_close,
     .bdrv_child_perm    = bdrv_format_default_perms,
+    .bdrv_co_create     = block_crypto_co_create_luks,
     .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
     .bdrv_truncate      = block_crypto_truncate,
     .create_opts        = &block_crypto_create_opts_luks,
-- 
2.13.6

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to luks, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qapi/block-core.json | 17 ++++++++++++++++-
>   block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 524d51567a..07039bfe9c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3452,6 +3452,21 @@
>               '*preallocation':   'PreallocMode' } }
>   
>   ##
> +# @BlockdevCreateOptionsLUKS:
> +#
> +# Driver specific image creation options for LUKS.
> +#
> +# @file             Node to create the image format on
> +# @size             Size of the virtual disk in bytes
> +#
> +# Since: 2.12

Well, as long as we make it by Tuesday :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to luks, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 17 ++++++++++++++++-
>  block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 524d51567a..07039bfe9c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3452,6 +3452,21 @@
>              '*preallocation':   'PreallocMode' } }
>  
>  ##
> +# @BlockdevCreateOptionsLUKS:
> +#
> +# Driver specific image creation options for LUKS.
> +#
> +# @file             Node to create the image format on
> +# @size             Size of the virtual disk in bytes
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsLUKS',
> +  'data': { 'file':             'BlockdevRef',
> +            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
> +            'size':             'size' } }

s/qcrypto/crypto/ in this field.


I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
base struct and just inherit from it to add file/size.

I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
or cons of inheritance vs embedding in this case.

> +
> +##
>  # @BlockdevCreateOptionsNfs:
>  #
>  # Driver specific image creation options for NFS.
> @@ -3643,7 +3658,7 @@
>        'http':           'BlockdevCreateNotSupported',
>        'https':          'BlockdevCreateNotSupported',
>        'iscsi':          'BlockdevCreateNotSupported',
> -      'luks':           'BlockdevCreateNotSupported',
> +      'luks':           'BlockdevCreateOptionsLUKS',
>        'nbd':            'BlockdevCreateNotSupported',
>        'nfs':            'BlockdevCreateOptionsNfs',
>        'null-aio':       'BlockdevCreateNotSupported',
> diff --git a/block/crypto.c b/block/crypto.c
> index b0a4cb3388..2035f9ab13 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -543,6 +543,39 @@ static int block_crypto_open_luks(BlockDriverState *bs,
>                                       bs, options, flags, errp);
>  }
>  
> +static int coroutine_fn
> +block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
> +{
> +    BlockdevCreateOptionsLUKS *luks_opts;
> +    BlockDriverState *bs = NULL;
> +    QCryptoBlockCreateOptions create_opts;
> +    int ret;
> +
> +    assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
> +    luks_opts = &create_options->u.luks;
> +
> +    bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
> +    if (bs == NULL) {
> +        return -EIO;
> +    }
> +
> +    create_opts = (QCryptoBlockCreateOptions) {
> +        .format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
> +        .u.luks = *luks_opts->qcrypto,
> +    };
> +
> +    ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
> +                                         errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    ret = 0;
> +fail:
> +    bdrv_unref(bs);
> +    return ret;
> +}
> +
>  static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
>                                                           QemuOpts *opts,
>                                                           Error **errp)
> @@ -647,6 +680,7 @@ BlockDriver bdrv_crypto_luks = {
>      .bdrv_open          = block_crypto_open_luks,
>      .bdrv_close         = block_crypto_close,
>      .bdrv_child_perm    = bdrv_format_default_perms,
> +    .bdrv_co_create     = block_crypto_co_create_luks,
>      .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
>      .bdrv_truncate      = block_crypto_truncate,
>      .create_opts        = &block_crypto_create_opts_luks,
> -- 
> 2.13.6
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to luks, which enables
> > image creation over QMP.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 17 ++++++++++++++++-
> >  block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 524d51567a..07039bfe9c 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3452,6 +3452,21 @@
> >              '*preallocation':   'PreallocMode' } }
> >  
> >  ##
> > +# @BlockdevCreateOptionsLUKS:
> > +#
> > +# Driver specific image creation options for LUKS.
> > +#
> > +# @file             Node to create the image format on
> > +# @size             Size of the virtual disk in bytes
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > +  'data': { 'file':             'BlockdevRef',
> > +            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
> > +            'size':             'size' } }
> 
> s/qcrypto/crypto/ in this field.
> 
> I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
> as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> base struct and just inherit from it to add file/size.
> 
> I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
> or cons of inheritance vs embedding in this case.

I don't think QAPI has a way to embed it in JSON, but still provide a
QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
inheritance, but instead of having the struct type reused, you get all
field definitions copied. I guess you could then manually cast to the
base type and it would still work, but it didn't really feel clean.

Kevin

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 12:47:21PM +0100, Kevin Wolf wrote:
> Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> > On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > > This adds the .bdrv_co_create driver callback to luks, which enables
> > > image creation over QMP.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  qapi/block-core.json | 17 ++++++++++++++++-
> > >  block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
> > >  2 files changed, 50 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 524d51567a..07039bfe9c 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -3452,6 +3452,21 @@
> > >              '*preallocation':   'PreallocMode' } }
> > >  
> > >  ##
> > > +# @BlockdevCreateOptionsLUKS:
> > > +#
> > > +# Driver specific image creation options for LUKS.
> > > +#
> > > +# @file             Node to create the image format on
> > > +# @size             Size of the virtual disk in bytes
> > > +#
> > > +# Since: 2.12
> > > +##
> > > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > > +  'data': { 'file':             'BlockdevRef',
> > > +            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
> > > +            'size':             'size' } }
> > 
> > s/qcrypto/crypto/ in this field.
> > 
> > I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
> > as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> > base struct and just inherit from it to add file/size.
> > 
> > I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
> > or cons of inheritance vs embedding in this case.
> 
> I don't think QAPI has a way to embed it in JSON, but still provide a
> QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
> inheritance, but instead of having the struct type reused, you get all
> field definitions copied. I guess you could then manually cast to the
> base type and it would still work, but it didn't really feel clean.

IIRC, QAPI generates a method

   $BASETYPE *qapi_$TYPENAME_base(const $TYPENAME *obj)

which just does the blind cast to the base type.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

Am 12.03.2018 um 12:50 hat Daniel P. Berrangé geschrieben:
> On Mon, Mar 12, 2018 at 12:47:21PM +0100, Kevin Wolf wrote:
> > Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> > > On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > > > This adds the .bdrv_co_create driver callback to luks, which enables
> > > > image creation over QMP.
> > > > 
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > ---
> > > >  qapi/block-core.json | 17 ++++++++++++++++-
> > > >  block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 50 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index 524d51567a..07039bfe9c 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -3452,6 +3452,21 @@
> > > >              '*preallocation':   'PreallocMode' } }
> > > >  
> > > >  ##
> > > > +# @BlockdevCreateOptionsLUKS:
> > > > +#
> > > > +# Driver specific image creation options for LUKS.
> > > > +#
> > > > +# @file             Node to create the image format on
> > > > +# @size             Size of the virtual disk in bytes
> > > > +#
> > > > +# Since: 2.12
> > > > +##
> > > > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > > > +  'data': { 'file':             'BlockdevRef',
> > > > +            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
> > > > +            'size':             'size' } }
> > > 
> > > s/qcrypto/crypto/ in this field.
> > > 
> > > I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
> > > as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> > > base struct and just inherit from it to add file/size.
> > > 
> > > I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
> > > or cons of inheritance vs embedding in this case.
> > 
> > I don't think QAPI has a way to embed it in JSON, but still provide a
> > QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
> > inheritance, but instead of having the struct type reused, you get all
> > field definitions copied. I guess you could then manually cast to the
> > base type and it would still work, but it didn't really feel clean.
> 
> IIRC, QAPI generates a method
> 
>    $BASETYPE *qapi_$TYPENAME_base(const $TYPENAME *obj)
> 
> which just does the blind cast to the base type.

Oh, thanks, I didn't know that one! That makes it an offical API that
the QAPI generators must keep working, so it should be fine to use. I'll
give it a try then.

Kevin

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Eric Blake]

On 03/12/2018 06:47 AM, Kevin Wolf wrote:

>>> +##
>>> +{ 'struct': 'BlockdevCreateOptionsLUKS',
>>> +  'data': { 'file':             'BlockdevRef',
>>> +            'qcrypto':          'QCryptoBlockCreateOptionsLUKS',
>>> +            'size':             'size' } }
>>
>> s/qcrypto/crypto/ in this field.
>>
>> I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
>> as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
>> base struct and just inherit from it to add file/size.
>>
>> I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
>> or cons of inheritance vs embedding in this case.
> 
> I don't think QAPI has a way to embed it in JSON, but still provide a
> QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
> inheritance, but instead of having the struct type reused, you get all
> field definitions copied. I guess you could then manually cast to the
> base type and it would still work, but it didn't really feel clean.

The QAPI generators generate a function (qapi_NAME_base()) which will 
convert any derived type back to the parent class, which is cleaner than 
manually casting; if that makes the decision to use inheritance any easier.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 4/6] luks: Turn invalid assertion into check

[Kevin Wolf]

The .bdrv_getlength implementation of the crypto block driver asserted
that the payload offset isn't after EOF. This is an invalid assertion to
make as the image file could be corrupted. Instead, check it and return
-EIO if the file is too small for the payload offset.

Zero length images are fine, so trigger -EIO only on offset > len, not
on offset >= len as the assertion did before.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index 2035f9ab13..4908d8627f 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
 
     uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
     assert(offset < INT64_MAX);
-    assert(offset < len);
+
+    if (offset > len) {
+       return -EIO;
+    }
 
     len -= offset;
 
-- 
2.13.6

Re: [Qemu-devel] [PATCH 4/6] luks: Turn invalid assertion into check

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> The .bdrv_getlength implementation of the crypto block driver asserted
> that the payload offset isn't after EOF. This is an invalid assertion to
> make as the image file could be corrupted. Instead, check it and return
> -EIO if the file is too small for the payload offset.

Good catch.  Probably not a CVE (unless someone can argue some way that 
causing a crash on an attempt to load a maliciously corrupted file can 
be used as a denial of service across a privilege boundary), but 
definitely needs fixing.

> 
> Zero length images are fine, so trigger -EIO only on offset > len, not
> on offset >= len as the assertion did before.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/crypto.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 2035f9ab13..4908d8627f 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
>   
>       uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
>       assert(offset < INT64_MAX);

Umm, if the file can be corrupted, what's to prevent someone from 
sticking in a negative size that fails this assertion?

> -    assert(offset < len);
> +
> +    if (offset > len) {
> +       return -EIO;
> +    }
>   
>       len -= offset;
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/6] luks: Turn invalid assertion into check

[Kevin Wolf]

Am 09.03.2018 um 21:19 hat Eric Blake geschrieben:
> On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> > The .bdrv_getlength implementation of the crypto block driver asserted
> > that the payload offset isn't after EOF. This is an invalid assertion to
> > make as the image file could be corrupted. Instead, check it and return
> > -EIO if the file is too small for the payload offset.
> 
> Good catch.  Probably not a CVE (unless someone can argue some way that
> causing a crash on an attempt to load a maliciously corrupted file can be
> used as a denial of service across a privilege boundary), but definitely
> needs fixing.
> 
> > 
> > Zero length images are fine, so trigger -EIO only on offset > len, not
> > on offset >= len as the assertion did before.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/crypto.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 2035f9ab13..4908d8627f 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
> >       uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
> >       assert(offset < INT64_MAX);
> 
> Umm, if the file can be corrupted, what's to prevent someone from sticking
> in a negative size that fails this assertion?

In qcrypto_block_luks_open():

    block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
    block->payload_offset = luks->header.payload_offset *
            block->sector_size

The sector size is 512LL, and luks->header.payload_offset is 32 bit.

But I just saw that block_crypto_truncate() has another wrong assertion.
Maybe I should fix that and write a test case for it. Not sure if I'll
add it to this series or as a follow-up during the freeze.

Kevin

Re: [Qemu-devel] [PATCH 4/6] luks: Turn invalid assertion into check

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:11PM +0100, Kevin Wolf wrote:
> The .bdrv_getlength implementation of the crypto block driver asserted
> that the payload offset isn't after EOF. This is an invalid assertion to
> make as the image file could be corrupted. Instead, check it and return
> -EIO if the file is too small for the payload offset.
> 
> Zero length images are fine, so trigger -EIO only on offset > len, not
> on offset >= len as the assertion did before.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 2035f9ab13..4908d8627f 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
>  
>      uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
>      assert(offset < INT64_MAX);
> -    assert(offset < len);
> +
> +    if (offset > len) {
> +       return -EIO;
> +    }
>  
>      len -= offset;

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 5/6] luks: Catch integer overflow for huge sizes

[Kevin Wolf]

When you request an image size close to UINT64_MAX, the addition of the
crypto header may cause an integer overflow. Catch it instead of
silently truncating the image size.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 4908d8627f..1b46519c53 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -102,6 +102,11 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
 {
     struct BlockCryptoCreateData *data = opaque;
 
+    if (headerlen > UINT64_MAX - data->size) {
+        error_setg(errp, "The requested file size is too large");
+        return -EFBIG;
+    }
+
     /* User provided size should reflect amount of space made
      * available to the guest, so we must take account of that
      * which will be used by the crypto header
-- 
2.13.6

Re: [Qemu-devel] [PATCH 5/6] luks: Catch integer overflow for huge sizes

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> When you request an image size close to UINT64_MAX, the addition of the
> crypto header may cause an integer overflow. Catch it instead of
> silently truncating the image size.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/crypto.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 4908d8627f..1b46519c53 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -102,6 +102,11 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
>   {
>       struct BlockCryptoCreateData *data = opaque;
>   
> +    if (headerlen > UINT64_MAX - data->size) {

INT64_MAX, please.  We are further bounded by having to fit within off_t 
(signed) rather than uint64_t.

> +        error_setg(errp, "The requested file size is too large");
> +        return -EFBIG;
> +    }
> +
>       /* User provided size should reflect amount of space made
>        * available to the guest, so we must take account of that
>        * which will be used by the crypto header
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 5/6] luks: Catch integer overflow for huge sizes

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:12PM +0100, Kevin Wolf wrote:
> When you request an image size close to UINT64_MAX, the addition of the
> crypto header may cause an integer overflow. Catch it instead of
> silently truncating the image size.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 4908d8627f..1b46519c53 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -102,6 +102,11 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
>  {
>      struct BlockCryptoCreateData *data = opaque;
>  
> +    if (headerlen > UINT64_MAX - data->size) {
> +        error_setg(errp, "The requested file size is too large");
> +        return -EFBIG;
> +    }
> +
>      /* User provided size should reflect amount of space made
>       * available to the guest, so we must take account of that
>       * which will be used by the crypto header

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

(if using INT64_MAX as Eric suggests)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 6/6] qemu-iotests: Test luks QMP image creation

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/208       | 211 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/208.out   | 136 ++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |   2 +-
 tests/qemu-iotests/group     |   1 +
 4 files changed, 349 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/208
 create mode 100644 tests/qemu-iotests/208.out

diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
new file mode 100755
index 0000000000..ab4650d0ae
--- /dev/null
+++ b/tests/qemu-iotests/208
@@ -0,0 +1,211 @@
+#!/bin/bash
+#
+# Test luks and file image creation
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+                          | _filter_qemu | _filter_imgfmt \
+                          | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG_FILE",
+      "size": 0
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "file",
+      "node-name": "imgfile",
+      "filename": "$TEST_IMG_FILE"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "imgfile",
+      "qcrypto": { "key-secret": "keysec0" },
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (with non-default options) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((64 * 1024 * 1024))
+
+run_qemu -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG_FILE",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG_FILE"
+      },
+      "size": $size,
+      "qcrypto": {
+          "key-secret": "keysec0",
+          "cipher-alg": "twofish-128",
+          "cipher-mode": "ctr",
+          "ivgen-alg": "plain64",
+          "ivgen-hash-alg": "md5",
+          "hash-alg": "sha1",
+          "iter-time": 10
+      }
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Invalid BlockdevRef ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "this doesn't exist",
+      "qcrypto": {},
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Zero size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+         -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "qcrypto": { "key-secret": "keysec0" },
+      "size": 0
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+         -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "qcrypto": { "key-secret": "keysec0" },
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "qcrypto": { "key-secret": "keysec0" },
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "qcrypto": { "key-secret": "keysec0" },
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/208.out b/tests/qemu-iotests/208.out
new file mode 100644
index 0000000000..1123661f68
--- /dev/null
+++ b/tests/qemu-iotests/208.out
@@ -0,0 +1,136 @@
+QA output created by 208
+
+=== Successful image creation (defaults) ===
+
+Testing: -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+Format specific information:
+    ivgen alg: plain64
+    hash alg: sha256
+    cipher alg: aes-256
+    uuid: 00000000-0000-0000-0000-000000000000
+    cipher mode: xts
+    slots:
+        [0]:
+            active: true
+            iters: 1024
+            key offset: 4096
+            stripes: 4000
+        [1]:
+            active: false
+            key offset: 262144
+        [2]:
+            active: false
+            key offset: 520192
+        [3]:
+            active: false
+            key offset: 778240
+        [4]:
+            active: false
+            key offset: 1036288
+        [5]:
+            active: false
+            key offset: 1294336
+        [6]:
+            active: false
+            key offset: 1552384
+        [7]:
+            active: false
+            key offset: 1810432
+    payload offset: 2068480
+    master key iters: 1024
+
+=== Successful image creation (with non-default options) ===
+
+Testing: -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+Format specific information:
+    ivgen alg: plain64
+    hash alg: sha1
+    cipher alg: twofish-128
+    uuid: 00000000-0000-0000-0000-000000000000
+    cipher mode: ctr
+    slots:
+        [0]:
+            active: true
+            iters: 1024
+            key offset: 4096
+            stripes: 4000
+        [1]:
+            active: false
+            key offset: 69632
+        [2]:
+            active: false
+            key offset: 135168
+        [3]:
+            active: false
+            key offset: 200704
+        [4]:
+            active: false
+            key offset: 266240
+        [5]:
+            active: false
+            key offset: 331776
+        [6]:
+            active: false
+            key offset: 397312
+        [7]:
+            active: false
+            key offset: 462848
+    payload offset: 528384
+    master key iters: 1024
+
+=== Invalid BlockdevRef ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=this doesn't exist nor node_name=this doesn't exist"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+
+=== Zero size ===
+
+Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 0 (0 bytes)
+
+=== Invalid sizes ===
+
+Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "The requested file size is too large"}}
+{"error": {"class": "GenericError", "desc": "Image size cannot be negative"}}
+{"error": {"class": "GenericError", "desc": "Image size cannot be negative"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+*** done
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 6fa0495e10..9a65a11026 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -332,7 +332,7 @@ _img_info()
 
     discard=0
     regex_json_spec_start='^ *"format-specific": \{'
-    $QEMU_IMG info "$@" "$TEST_IMG" 2>&1 | \
+    $QEMU_IMG info $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" 2>&1 | \
         sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
             -e "s#$TEST_DIR#TEST_DIR#g" \
             -e "s#$IMGFMT#IMGFMT#g" \
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c401791fcd..574227e761 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -204,3 +204,4 @@
 205 rw auto quick
 206 rw auto
 207 rw auto
+208 rw auto
-- 
2.13.6

Re: [Qemu-devel] [PATCH 6/6] qemu-iotests: Test luks QMP image creation

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/208       | 211 +++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/208.out   | 136 ++++++++++++++++++++++++++++
>   tests/qemu-iotests/common.rc |   2 +-
>   tests/qemu-iotests/group     |   1 +
>   4 files changed, 349 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/208
>   create mode 100644 tests/qemu-iotests/208.out

I've seen another patch using 208 - someone gets to renumber ;)

> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1	# failure is the default!

Whatever happened to Jeff's efforts to reduce pointless boilerplate?


> +function do_run_qemu()
> +{
> +    echo Testing: "$@"
> +    $QEMU -nographic -qmp stdio -serial none "$@"

Is -nodefaults better than -serial none?

At any rate, my comments are trivial, whether or not you do something 
about them, so

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] qemu-iotests: Test luks QMP image creation

[Daniel P. Berrangé]

On Fri, Mar 09, 2018 at 06:27:13PM +0100, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/208       | 211 +++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/208.out   | 136 ++++++++++++++++++++++++++++
>  tests/qemu-iotests/common.rc |   2 +-
>  tests/qemu-iotests/group     |   1 +
>  4 files changed, 349 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
> 
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 0000000000..ab4650d0ae
> --- /dev/null
> +++ b/tests/qemu-iotests/208


> +
> +run_qemu -object secret,id=keysec0,data="foo" <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +      "driver": "file",
> +      "filename": "$TEST_IMG_FILE",
> +      "size": 0
> +  }
> +}
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +      "driver": "$IMGFMT",
> +      "file": {
> +          "driver": "file",
> +          "filename": "$TEST_IMG_FILE"
> +      },
> +      "size": $size,
> +      "qcrypto": {
> +          "key-secret": "keysec0",
> +          "cipher-alg": "twofish-128",
> +          "cipher-mode": "ctr",
> +          "ivgen-alg": "plain64",
> +          "ivgen-hash-alg": "md5",
> +          "hash-alg": "sha1",
> +          "iter-time": 10

I'd recommend using  'iter-time': 10, in every single x-blockdev-create
command in this file, otherwise this test will be unneccessarily slow to
execute as each x-blockdev-create will burn 2 seconds of time.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|