block/null: Use 'read-zeroes' mode by default

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

Posted by Philippe Mathieu-Daudé 3 years, 2 months ago

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'.

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

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

Posted by Eric Blake 3 years, 2 months ago

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

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

Posted by Philippe Mathieu-Daudé 3 years, 2 months ago

On 2/9/21 6:11 PM, Eric Blake wrote:
> 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?

Oops, yes ;)

> 
>>
>> 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.

Correct. I don't know about each iotest but I can send a patch with
explicit option, so review would be trivial.

Thanks,

Phil.

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

Posted by Max Reitz 3 years, 2 months ago

On 09.02.21 18:01, 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:

I suppose in practice it’s going to be uninitialized guest memory most 
of the time, so it isn’t that bad, but yes.

Thanks!

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

I think this is good.  If we do want a stricter approach, we might 
remove read-zeroes altogether (but I suppose that would require a 
deprecation period then) and add a new null-unsafe driver or something 
in its stead (that we can the conditionally compile out, or 
distributions can choose not to whitelist, or, or, or...).

If we just follow through with this patch, I don’t think we need a 
deprecation period, because this can well be considered a bug fix; and 
because I don’t know of any use for read-zeroes=false except for some 
very special performance tests.

> ---
>   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;

The documentation in qapi/block-core.json has to be changed, too.

Are there any iotests (or other tests) that don’t set read-zeroes? 
Should they continue to use read-zeroes=false?

Max