Re: [Qemu-block] [PATCH] qcow2: Check the L1 table parameters from all internal snapshots

[Max Reitz]

On 2018-02-09 12:37, Alberto Garcia wrote:
> The code that reads the qcow2 snapshot table takes the offset and size
> of all snapshots' L1 table without doing any kind of checks.
> 
> Although qcow2_snapshot_load_tmp() does verify that the table size is
> valid, the table offset is not checked at all. On top of that there
> are several other code paths that deal with internal snapshots that
> don't use that function or do any other check.
> 
> This patch verifies that all L1 tables are correctly aligned and the
> size does not exceed the maximum allowed valued.
> 
> The check from qcow2_snapshot_load_tmp() is removed since it's now
> useless (opening an image will fail before that).
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2-snapshot.c     | 14 ++++++++++----
>  tests/qemu-iotests/080     | 10 +++++++++-
>  tests/qemu-iotests/080.out | 10 ++++++++--
>  3 files changed, 27 insertions(+), 7 deletions(-)

I don't like completely refusing to open a qcow2 image if one of the
snapshots is invalid, without giving the user any way of fixing it.

With this patch, the final two images created in 080 cannot be opened at
all (not even with qemu-img info).  Without it, you can, you just can't
load the snapshots.  (Well, in one case.  In the other, you can even do
that, but that's a bug, as you correctly claim.)

More importantly, though, without this patch, you can delete the
snapshots.  qemu-img will complain a bit, and you'll have leaks
afterwards, but that's nothing qemu-img check can't fix.  With this
patch, you can't, because the image cannot be opened at all so it's
basically gone for good (unless you get a hex editor).

Another thing is that the error messages are not really useful...
(which is already an issue, but this patch doesn't make it better.)

Max

signature.asc
Description: OpenPGP digital signature