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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 09.02.2018 um 14:04 hat Max Reitz geschrieben:
> 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 <berto@igalia.com>
> > ---
> >  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).

How about we move the check to bdrv_open() as proposed, but make it
conditional so that it's skipped with BDRV_O_CHECK and then add a way to
fix the situation with qemu-img check -r?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]