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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

On 11/07/2014 12:39 PM, Kevin Wolf wrote:
> If the user neglects to specify the image format, QEMU probes the
> image to guess it automatically, for convenience.
> 

[for those patches in 1-6 where I did not leave comments, I'm happy with
them, and saw that Max already gave R-b so I didn't spend thorough
review time on them]

> 
> The other differences of this patch to the old one are that it doesn't
> silently write something different than the guest requested by zeroing
> out some bytes (it fails the request instead) and that it doesn't
> maintain a list of signatures in the raw driver (it calls the usual
> probe function instead).
> 
> Note that this change doesn't introduce new breakage for false positive
> cases where the guest legitimately writes data into the first sector
> that matches the signatures of an image format (e.g. for nested virt):
> These cases were broken before, only the failure mode changes from
> corruption after the next restart (when the wrong format is probed) to
> failing the problematic write request.

I would feel better if this commit message explicitly mentioned that the
failed write can ONLY occur when probing occurs; therefore, a user can
ensure that guests can legitimately write anything to the first sector
by explicitly providing a format.  But at least the error message does it.

I'm still not 100% convinced this is the patch we want, but am happy
enough that it won't break libvirt (which strives to always pass a
format), so I'm comfortable leaving a review.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   |  5 +++--
>  block/raw_bsd.c           | 57 ++++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block_int.h |  3 +++
>  3 files changed, 62 insertions(+), 3 deletions(-)

> @@ -158,6 +202,17 @@ 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'.\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",

This error message works fairly well for me.  Maybe the first line could
be a bit longer:

WARNING: Image format was not specified for '%s', and raw was assumed.\n

or maybe:

WARNING: Image format was not specified for '%s', and probing guessed raw.\n

but even with your original shorter wording,

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]