Re: [Qemu-devel] [PATCH v3 7/9] raw: Prohibit dangerous writes for probed images

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Kevin Wolf (kwolf@redhat.com) wrote:


> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 401b967..2ce5409 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
>  static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
>                                        int nb_sectors, QEMUIOVector *qiov)
>  {
> +    void *buf = NULL;
> +    BlockDriver *drv;
> +    QEMUIOVector local_qiov;
> +    int ret;
> +
> +    if (bs->probed && sector_num == 0) {
> +        /* As long as these conditions are true, we can't get partial writes to
> +         * the probe buffer and can just directly check the request. */
> +        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
> +        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
> +
> +        if (nb_sectors == 0) {
> +            /* qemu_iovec_to_buf() would fail, but we want to return success
> +             * instead of -EINVAL in this case. */
> +            return 0;
> +        }
> +
> +        buf = qemu_try_blockalign(bs->file, 512);
> +        if (!buf) {
> +            ret = -ENOMEM;
> +            goto fail;
> +        }
> +
> +        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
> +        if (ret != 512) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        drv = bdrv_probe_all(buf, 512, NULL);
> +        if (drv != bs->drv) {
> +            ret = -EPERM;
> +            goto fail;
> +        }

Two things about this worry me:
   1) It allows a running guest to prod at the probing code potentially quite
hard; if there is anything nasty that can be done during probing it would
potentially make it easier for a guest to find it.

   2) We don't log anything when this failure happens so if someone hits
this by accident for some reason it'll confuse them no end.  Could we add
a (1 time?) error_report/printf just so that there's something to work with ?

Dave

> +
> +        /* Use the checked buffer, a malicious guest might be overwriting its
> +         * original buffer in the background. */
> +        qemu_iovec_init(&local_qiov, qiov->niov + 1);
> +        qemu_iovec_add(&local_qiov, buf, 512);
> +        qemu_iovec_concat(&local_qiov, qiov, 512, qiov->size - 512);
> +        qiov = &local_qiov;
> +    }
> +
>      BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
> -    return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +    ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +
> +fail:
> +    if (qiov == &local_qiov) {
> +        qemu_iovec_destroy(&local_qiov);
> +    }
> +    qemu_vfree(buf);
> +    return ret;
>  }
>  
>  static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
> @@ -158,6 +208,18 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>                      Error **errp)
>  {
>      bs->sg = bs->file->sg;
> +
> +    if (bs->probed && !bdrv_is_read_only(bs)) {
> +        fprintf(stderr,
> +                "WARNING: Image format was not specified for '%s' and probing "
> +                "guessed raw.\n"
> +                "         Automatically detecting the format is dangerous for "
> +                "raw images, write operations on block 0 will be restricted.\n"
> +                "         Specify the 'raw' format explicitly to remove the "
> +                "restrictions.\n",
> +                bs->file->filename);
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index cd94559..192fce8 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -326,6 +326,7 @@ struct BlockDriverState {
>      int sg;        /* if true, the device is a /dev/sg* */
>      int copy_on_read; /* if true, copy read backing sectors into image
>                           note this is a reference count */
> +    bool probed;
>  
>      BlockDriver *drv; /* NULL means no media */
>      void *opaque;
> @@ -414,6 +415,8 @@ struct BlockDriverState {
>  };
>  
>  int get_tmp_filename(char *filename, int size);
> +BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
> +                            const char *filename);
>  
>  void bdrv_set_io_limits(BlockDriverState *bs,
>                          ThrottleConfig *cfg);
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK