| Message ID | 20210329072559.2668780-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] block/vpc: Support probing of fixed-size VHD images | expand |
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
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
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
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
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; }
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(-)