[RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Thomas Huth]

Fixed-size VHD images don't have a header, only a footer. To be able
to still detect them right, support probing via the file name, too.

Without this change, images get detected as raw:

$ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
$ qemu-img info test.vhd
image: test.vhd
file format: raw
virtual size: 2 GiB (2147992064 bytes)
disk size: 8 KiB

With this change:

$ qemu-img info test.vhd
image: test.vhd
file format: vpc
virtual size: 2 GiB (2147991552 bytes)
disk size: 8 KiB

Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 I've marked the subject with RFC since I'm not quite sure whether this
 is really a good idea... please let me know what you think about it...

 block/vpc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482..be561e4b39 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
 
 static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
-    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
+    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
         return 100;
+    }
+
+    /* It could be a fixed-size image without header -> check extension, too */
+    if (filename) {
+        int len = strlen(filename);
+        if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
+            return 10;
+        }
+    }
+
     return 0;
 }
 
-- 
2.27.0

Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Thomas Huth]

On 29/03/2021 09.25, Thomas Huth wrote:
> Fixed-size VHD images don't have a header, only a footer. To be able
> to still detect them right, support probing via the file name, too.
> 
> Without this change, images get detected as raw:
> 
> $ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
> Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
> $ qemu-img info test.vhd
> image: test.vhd
> file format: raw
> virtual size: 2 GiB (2147992064 bytes)
> disk size: 8 KiB
> 
> With this change:
> 
> $ qemu-img info test.vhd
> image: test.vhd
> file format: vpc
> virtual size: 2 GiB (2147991552 bytes)
> disk size: 8 KiB
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   I've marked the subject with RFC since I'm not quite sure whether this
>   is really a good idea... please let me know what you think about it...
> 
>   block/vpc.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 17a705b482..be561e4b39 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
>   
>   static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
>   {
> -    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
> +    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
>           return 100;
> +    }
> +
> +    /* It could be a fixed-size image without header -> check extension, too */
> +    if (filename) {
> +        int len = strlen(filename);
> +        if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
> +            return 10;
> +        }
> +    }
> +
>       return 0;
>   }

Ping!

Anybody any comments on this one?

  Thomas

Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Kevin Wolf]

Am 29.03.2021 um 09:25 hat Thomas Huth geschrieben:
> Fixed-size VHD images don't have a header, only a footer. To be able
> to still detect them right, support probing via the file name, too.
> 
> Without this change, images get detected as raw:
> 
> $ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
> Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
> $ qemu-img info test.vhd
> image: test.vhd
> file format: raw
> virtual size: 2 GiB (2147992064 bytes)
> disk size: 8 KiB
> 
> With this change:
> 
> $ qemu-img info test.vhd
> image: test.vhd
> file format: vpc
> virtual size: 2 GiB (2147991552 bytes)
> disk size: 8 KiB
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  I've marked the subject with RFC since I'm not quite sure whether this
>  is really a good idea... please let me know what you think about it...

There is precedence for using the file name, and it's convenient, so
when done carefully, I think this is okay.

The classic problem we have with probing is that a malicious guest on a
probed raw image could write a $format header into the image and be
probed as something else the next time. For headers, we prevent this in
the raw driver, for filename based probes we don't.

Of course, if you call your raw image .vhd and use probing, you're
almost explicitly asking for trouble.

What happens if you do it anyway and the guest writes a VHD footer? In
theory, we can know that it's a VHD_FIXED image, and fixed image footers
don't contain anything that would allow a guest more than destroying
itself. Other than carrying the additional metadata in the footer, they
behave the same as raw images anyway.

We have a bug in vpc_open(), though: It sets the local variable
disk_type correctly, but other functions use s->footer.type instead and
we never check that it actually matches the disk type we think we're
opening.

So I think we need to add this check (and we need to add it even if we
don't change probing), and then the change to vpc_probe() is probably
okay.

Kevin

Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Max Reitz]

On 19.05.21 12:19, Thomas Huth wrote:
> On 29/03/2021 09.25, Thomas Huth wrote:
>> Fixed-size VHD images don't have a header, only a footer. To be able
>> to still detect them right, support probing via the file name, too.
>>
>> Without this change, images get detected as raw:
>>
>> $ qemu-img create -f vpc -o subformat=fixed test.vhd 2G
>> Formatting 'test.vhd', fmt=vpc size=2147483648 subformat=fixed
>> $ qemu-img info test.vhd
>> image: test.vhd
>> file format: raw
>> virtual size: 2 GiB (2147992064 bytes)
>> disk size: 8 KiB
>>
>> With this change:
>>
>> $ qemu-img info test.vhd
>> image: test.vhd
>> file format: vpc
>> virtual size: 2 GiB (2147991552 bytes)
>> disk size: 8 KiB
>>
>> Resolves: https://bugs.launchpad.net/qemu/+bug/1819182
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   I've marked the subject with RFC since I'm not quite sure whether this
>>   is really a good idea... please let me know what you think about it...
>>
>>   block/vpc.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 17a705b482..be561e4b39 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -191,8 +191,18 @@ static uint32_t vpc_checksum(void *p, size_t size)
>>     static int vpc_probe(const uint8_t *buf, int buf_size, const char 
>> *filename)
>>   {
>> -    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
>> +    if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8)) {
>>           return 100;
>> +    }
>> +
>> +    /* It could be a fixed-size image without header -> check 
>> extension, too */
>> +    if (filename) {
>> +        int len = strlen(filename);
>> +        if (len > 4 && !strcasecmp(&filename[len - 4], ".vhd")) {
>> +            return 10;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>
> Ping!
>
> Anybody any comments on this one?
>
>  Thomas

Sorry I’m replying so late, but honestly, it’s because I’m just a bit 
afraid to respond.  So, perhaps like others, I hoped someone else with a 
stronger opinion would do it in my stead.

I understand this addresses a real problem, but OTOH probing by the file 
extension intuitively seems like a bad solution to me. What’s the 
problem with simply requiring the user to spcify the format in such a case?

I mean, I can’t think of a concrete problem with probing by the filename 
extension.  The worst that can happen is that a raw image is called 
.vhd, we try to open it, and the vhd driver then says the image doesn’t 
work.  That could be a real problem, but, well, it would be kind of 
deserved.  (Unless the user really in good faith just thought .vhd would 
be a nice extension for their raw virtual HDD image.  Which, then again, 
I couldn’t blame them for.)

Perhaps we could print a message if the extension matches that advises 
the user to explicitly specify the format for such images?

Max

Re: [RFC PATCH] block/vpc: Support probing of fixed-size VHD images

[Thomas Huth]

On 19/05/2021 12.56, Kevin Wolf wrote:
[...]
> We have a bug in vpc_open(), though: It sets the local variable
> disk_type correctly, but other functions use s->footer.type instead and
> we never check that it actually matches the disk type we think we're
> opening.
> 
> So I think we need to add this check (and we need to add it even if we
> don't change probing), and then the change to vpc_probe() is probably
> okay.

Right, that looks like a potential problem. I now finally sent a patch here:

https://patchew.org/QEMU/20211012082702.792259-1-thuth@redhat.com/

  Thomas