Re: [RFC PATCH] block/null: Use 'read-zeroes' mode by default

[Eric Blake]

On 2/9/21 11:01 AM, 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 can confuse security experts. 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=on'.

Wouldn't this rather be read-zeroes=off when doing performance testing?

> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC maybe a stricter approach is required?

Since the null driver is only for testing in the first place, opting in
to speed over security seems like a reasonable tradeoff.  But I consider
the patch incomplete without an audit of the iotests that will want to
use explicit read-zeroes=off.

> ---
>  block/null.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;
> 

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