diff mbox series

[v2,2/2] block/null: Enable 'read-zeroes' mode by default

Message ID 20210211142656.3818078-3-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: Use 'read-zeroes=true' mode by default with 'null-co' driver | expand

Commit Message

Philippe Mathieu-Daudé Feb. 11, 2021, 2:26 p.m. UTC
The null-co driver is meant for (performance) testing.
By default, read operation does nothing, the provided buffer
is not filled with zero values and its content is unchanged.

This performance 'feature' becomes an issue from a security
perspective.  For example, using the default null-co driver,
buf[] is uninitialized, the blk_pread() call succeeds and we
then access uninitialized memory:

  static int guess_disk_lchs(BlockBackend *blk,
                             int *pcylinders, int *pheads,
                             int *psectors)
  {
      uint8_t buf[BDRV_SECTOR_SIZE];
      ...

      if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
          return -1;
      }
      /* test msdos magic */
      if (buf[510] != 0x55 || buf[511] != 0xaa) {
          return -1;
      }

We could audit all the uninitialized buffers and the
bdrv_co_preadv() handlers, but it is simpler to change the
default of this testing driver. Performance tests will have
to adapt and use 'null-co,read-zeroes=off'.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/null.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefan Hajnoczi Feb. 11, 2021, 4:22 p.m. UTC | #1
On Thu, Feb 11, 2021 at 03:26:56PM +0100, Philippe Mathieu-Daudé wrote:
> The null-co driver is meant for (performance) testing.
> By default, read operation does nothing, the provided buffer
> is not filled with zero values and its content is unchanged.
> 
> This performance 'feature' becomes an issue from a security
> perspective.  For example, using the default null-co driver,
> buf[] is uninitialized, the blk_pread() call succeeds and we
> then access uninitialized memory:
> 
>   static int guess_disk_lchs(BlockBackend *blk,
>                              int *pcylinders, int *pheads,
>                              int *psectors)
>   {
>       uint8_t buf[BDRV_SECTOR_SIZE];
>       ...
> 
>       if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
>           return -1;
>       }
>       /* test msdos magic */
>       if (buf[510] != 0x55 || buf[511] != 0xaa) {
>           return -1;
>       }
> 
> We could audit all the uninitialized buffers and the
> bdrv_co_preadv() handlers, but it is simpler to change the
> default of this testing driver. Performance tests will have
> to adapt and use 'null-co,read-zeroes=off'.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/null.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea72..f9658fd70ac 100644
--- a/block/null.c
+++ b/block/null.c
@@ -93,7 +93,7 @@  static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
         error_setg(errp, "latency-ns is invalid");
         ret = -EINVAL;
     }
-    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
+    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, true);
     qemu_opts_del(opts);
     bs->supported_write_flags = BDRV_REQ_FUA;
     return ret;