Re: [Qemu-devel] [PATCH 2/5] qcow2: Metadata overlap checks

[Kevin Wolf]

Am 27.08.2013 um 13:06 hat Max Reitz geschrieben:
> Am 27.08.2013 12:17, schrieb Kevin Wolf:
> >Am 26.08.2013 um 15:04 hat Max Reitz geschrieben:
> >>Two new functions are added; the first one checks a given range in the
> >>image file for overlaps with metadata (main header, L1 tables, L2
> >>tables, refcount table and blocks).
> >>
> >>The second one should be used immediately before writing to the image
> >>file as it calls the first function and, upon collision, marks the
> >>image as corrupt and makes the BDS unusable, thereby preventing
> >>further access.
> >>
> >>Both functions take a bitmask argument specifying the structures which
> >>should be checked for overlaps, making it possible to also check
> >>metadata writes against colliding with other structures.
> >>
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>  block/qcow2-refcount.c | 142 
> >> +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  block/qcow2.h          |  28 ++++++++++
> >>  2 files changed, 170 insertions(+)

> >>+                                 int64_t offset, int64_t size)
> >>+{
> >>+    BDRVQcowState *s = bs->opaque;
> >>+    int i, j;
> >>+
> >>+    if (!size) {
> >>+        return 0;
> >>+    }
> >>+
> >>+    if (chk & QCOW2_OL_MAIN_HEADER) {
> >>+        if (offset < s->cluster_size) {
> >>+            return QCOW2_OL_MAIN_HEADER;
> >>+        }
> >>+    }
> >>+
> >>+    if ((chk & QCOW2_OL_ACTIVE_L1) && s->l1_size) {
> >>+        if (ranges_overlap(offset, size, s->l1_table_offset,
> >>+            s->l1_size * sizeof(uint64_t))) {
> >The size could be rounded up to the next cluster boundary (same thing
> >for other metadata types).
> Would this actually change anything?

Not sure. With correct images, it wouldn't, because both the old and the
new offset would be aligned to a cluster boundary, so if there is an
overlap, it would be at the start. But after all, we're dealing with
broken images here.

I don't have a strong opinion on it, take it as a suggestion that you
can implement for additional safety. But if you don't want to, I don't
think it's a horrible gap in the checks.

Kevin