Re: [PATCH v5 3/7] qcow: Tolerate backing_fmt=, but warn on backing_fmt=raw

[Kevin Wolf]

Am 03.04.2020 um 19:58 hat Eric Blake geschrieben:
> qcow has no space in the metadata to store a backing format, and there
> are existing qcow images backed both by raw or by other formats
> (usually qcow) images, reliant on probing to tell the difference.
> While we don't recommend the creation of new qcow images (as qcow2 is
> hands-down better), we can at least insist that if the user does
> request a specific format without using -u, then it must be non-raw
> (as a raw backing file that gets inadvertently edited into some other
> format can form a security hole); if the user does not request a
> specific format or lies when using -u, then the status quo of probing
> for the backing format remains intact (although an upcoming patch will
> warn when omitting a format request).  Thus, when this series is
> complete, the only way to use a backing file for qcow without
> triggering a warning is when using -F if the backing file is non-raw
> to begin with.  Note that this is only for QemuOpts usage; there is no
> change to the QAPI to allow a format through -blockdev.
> 
> Add a new iotest 290 just for qcow, to demonstrate the new warning.
> 
> Signed-off-by: Eric Blake <address@hidden>

Somehow this feels backwards. Not specifying the backing file format at
all isn't any safer than explicitly specifying raw.

If there is a difference at all, I would say that explicitly specifying
raw means that the user is aware what they are doing. So we would have
more reason to warn against raw images if the backing format isn't
specified at all because then the user might not be aware that they are
using a backing file that probes as raw.

>  block/qcow.c               | 16 ++++++++-
>  tests/qemu-iotests/290     | 72 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/290.out | 42 ++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 130 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/290
>  create mode 100644 tests/qemu-iotests/290.out
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 8973e4e565a1..bbe48b7db4d7 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -940,11 +940,12 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
> *drv,
>  {
>      BlockdevCreateOptions *create_options = NULL;
>      BlockDriverState *bs = NULL;
> -    QDict *qdict;
> +    QDict *qdict = NULL;
>      Visitor *v;
>      const char *val;
>      Error *local_err = NULL;
>      int ret;
> +    char *backing_fmt;
> 
>      static const QDictRenames opt_renames[] = {
>          { BLOCK_OPT_BACKING_FILE,       "backing-file" },
> @@ -953,6 +954,13 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
> *drv,
>      };
> 
>      /* Parse options and convert legacy syntax */
> +    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
> +    if (backing_fmt && !strcmp(backing_fmt, "raw")) {
> +        error_setg(errp, "qcow cannot store backing format; an explicit "
> +                   "backing format of raw is unsafe");

Does this message tell that an implicit backing format of raw is safe?

> +        ret = -EINVAL;
> +        goto fail;
> +    }

The commit message promises a warning. This is not a warning, but a hard
error.

>      qdict = qemu_opts_to_qdict_filtered(opts, NULL, &qcow_create_opts, true);
> 
>      val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
> @@ -1018,6 +1026,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver 
> *drv,
> 
>      ret = 0;
>  fail:
> +    g_free(backing_fmt);
>      qobject_unref(qdict);
>      bdrv_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
> @@ -1152,6 +1161,11 @@ static QemuOptsList qcow_create_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "File name of a base image"
>          },
> +        {
> +            .name = BLOCK_OPT_BACKING_FMT,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Format of the backing image (caution: raw backing is 
> unsafe)",
> +        },
>          {
>              .name = BLOCK_OPT_ENCRYPT,
>              .type = QEMU_OPT_BOOL,
> diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
> new file mode 100755
> index 000000000000..8482de58cb4b
> --- /dev/null
> +++ b/tests/qemu-iotests/290
> @@ -0,0 +1,72 @@
> +#!/usr/bin/env bash
> +#
> +# Test qcow backing file warnings
> +#
> +# Copyright (C) 2020 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +status=1     # failure is the default!
> +
> +_cleanup()
> +{
> +     _cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow
> +_supported_proto file
> +_supported_os Linux
> +
> +size=128M
> +
> +echo
> +echo "== qcow backed by qcow =="
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img $size
> +_make_test_img -b "$TEST_IMG.base"
> +_img_info
> +_make_test_img -b "$TEST_IMG.base" -F $IMGFMT
> +_img_info
> +
> +echo
> +echo "== mismatched command line detection =="
> +
> +_make_test_img -b "$TEST_IMG.base" -F vmdk
> +# Use of -u bypasses the backing format sanity check
> +_make_test_img -u -b "$TEST_IMG.base" -F vmdk $size
> +_img_info
> +
> +echo
> +echo "== qcow backed by raw =="
> +
> +rm "$TEST_IMG.base"
> +truncate --size=$size "$TEST_IMG.base"
> +_make_test_img -b "$TEST_IMG.base"
> +_img_info
> +_make_test_img -b "$TEST_IMG.base" -F raw
> +_img_info

This test doesn't tell the difference between a warning and an error. In
both cases, the image would look the same: Either because it was
successfully created or because the old version is still there.

Kevin