Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Qcow2 disk images #237

Merged
merged 3 commits into from
Nov 29, 2024
Merged

Conversation

jakecorrenti
Copy link
Member

Use the imago crate to add support for multiple disk image types. This specifically adds support for the use of both Raw and Qcow2 disk images.

@jakecorrenti
Copy link
Member Author

cc @germag

Copy link
Collaborator

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Some slight nits but overall this LGTM.

src/devices/src/virtio/block/disk/mod.rs Outdated Show resolved Hide resolved
src/devices/Cargo.toml Outdated Show resolved Hide resolved
@tylerfanelli
Copy link
Collaborator

Nit: in 8a1795b commit description there is a typo: SyncFormatAccess<File should be SyncFormatAccess<File>

@XanClic
Copy link

XanClic commented Nov 14, 2024

I just realized one general thing (because we had that problem in qemu, for at least two CVEs): While it may seem obvious, note that open_implicit_dependencies_sync() can/will open other files, whose data will probably be accessible by the guest. Someone needs to verify this. It’s OK to say the user needs to, but still, something to note.

This becomes especially important when it comes to probing the format, as is done here, though: If you use a raw image for a guest, the guest can then write a qcow2 image header into it, and the next time you launch the VM, it will be opened as a qcow2 image. Because of the external file references qcow2 images have, this basically allows the guest access to any file on the host system the disk emulation has access to.

QEMU got around this problem by (a) very strongly encouraging users to specify the image format for every image, especially for (supposed) raw images; and (b) if raw was probed as a format, blocking writes to the first 512 bytes of the image that it would recognize as an image format header[1].

So (a) is a bit cumbersome to users, and if it were enforced for raw images, it would break compatibility. You could enforce it for non-raw images, to the same effect (i.e. format not specified = force raw), which keeps compatibility, but may be more cumbersome than you want.

That would leave only (b), i.e. to check if the offset is 0, then take the head of the I/O vector and probe it, potentially denying a write. Something like

let mut _copied_head = None;
if offset == 0 {
    let (head, tail) = iovec.split_at(cmp::max(self.mem_align(), 4) as u64);
    _copied_head = Some(head.try_into_owned(self.mem_align())?);
    let head_slice = _copied_head.as_ref().unwrap().as_ref().into_slice();
    if head_slice.get(0..4) == Some(&QCOW_MAGIC.to_be_bytes()) {
        return Err(...);
    }
    iovec = tail.with_inserted(0, head_slice);
}

Problem is that the guest could still write those four bytes one by one. qemu solves this with magic[2], but it may be reasonable to just require the guest to write the first sector as a whole (in case of a raw probed image), or at least the first four bytes, and reject anything else.

[1] More detail: When the guest writes to the first 512 bytes of an image that was probed to be raw (i.e. users didn’t specify a format, qemu couldn’t recognize any, so defaulted to using the image as raw), qemu will pass the data that is to be written to every image format driver it has, and let them check whether they would recognize it as an image in their format. If so, the write is denied. Note that for this process, the data must be copied into a bounce buffer, or the guest could exploit a race where it pretends it’s writing a normal boot sector, qemu verifies this, passes the buffer on, and then the guest modifies the buffer to be a qcow2 header right before it’s actually written to the file.

[2] For probed raw images, it sets the enforced request alignment to 512, ensuring there’s always a full read-modify-write cycle on the whole first 512 bytes, so it can check it all. Doesn’t work so easily here, because (a) imago does alignment on storage nodes, not format nodes, and (b) this is outside of imago. Now, if you really wanted to, you could emulate the same thing, but it requires a bit of hackiness:

  • (This all needs to be done aligned to both memory and request alignment, so: let full_align = cmp::max(512, cmp::max(self.req_align(), self.mem_align());)
  • You need to get the storage node (of the raw image) first, which you would do by let Mapping::Raw { storage, offset: _, writable: _ } = self.get_mapping_sync(0, full_align).unwrap().0;.
  • Then you install a strong write blocker: let _rmw_guard = storage.strong_write_blocker(0..full_align).await; (I could add a function to SyncFormatWrapper to allow running async code in its runtime)
  • Then you read the first sector (into an aligned buffer, i.e. IoBuffer::new(full_align, full_align)), paste the data from the guest into it, and probe it whether you recognize it as a format. If so, return an error.
  • If not, you write the first sector (via storage.pure_writev() to prevent deadlocks because of the write blocker)
  • Then you write the rest, if any.

@germag
Copy link
Contributor

germag commented Nov 14, 2024

@XanClic Idk how difficult could be with imago, but could also be a solution to just skip/hide the first 512 bytes of a raw image?, I mean from the guest POV the image will be 512 bytes smaller, so the guest will be able to write in the first 512 bytes of its reduced view (like a blank header for raw images)

Second thought that is incompatible with the idea of a "raw" image... so forget about it

@germag
Copy link
Contributor

germag commented Nov 14, 2024

IMO a strict a) in libkrun, with "format not specified = force raw" in the krunkit side, is better.

You will need to add a new api like krun_add_disk_with_format() that will take a format parameter, keeping krun_add_disk() (and krun_set_data_disk()) only for RAW images. I'll also add a big doc warning about this.

But, also adding the mitigations that Hanna mentioned for raw images, will be a good idea to prevent any misuse
@slp wdyt?

@XanClic
Copy link

XanClic commented Nov 14, 2024

@XanClic Idk how difficult could be with imago, but could also be a solution to just skip/hide the first 512 bytes of a raw image?, I mean from the guest POV the image will be 512 bytes smaller, so the guest will be able to write in the first 512 bytes of its reduced view (like a blank header for raw images)

Well, easy, because e.g. qemu’s raw “driver” allows you to specify offset and length parameters, and we could do the same, but…

Second thought that is incompatible with the idea of a "raw" image... so forget about it

Yes. Some image formats actually have their header in the first 512 bytes and then the rest is raw (I think there’s a vdi variant to that effect?). You could also create a thin qcow2 image that has the raw file as its external data file, but… It’s just not a raw image then.

@XanClic
Copy link

XanClic commented Nov 14, 2024

I'll also add a big doc warning about this.

Yes, that would be vital, or users may get the great idea of probing themselves, which would make it all moot. (I.e. the warning must clearly explain why probing is problematic, what the effects of opening an untrusted formatted image (e.g. in qcow2 format) can be (i.e. arbitrary file access), and that probing should be avoided at all costs.)

EDIT: To expand on the “avoid probing at all costs” point, a bit of a counterpoint again: If anyone does probing, it should be libkrun, because it controls the guest’s I/O path, and so can actually do what qemu does, which is prevent the guest from turning raw images into formatted ones. So if probing is seen as something that anyone will reasonably want, it may be the most reasonable thing to do it in libkrun and not risk users doing it worse. It is true that nobody should probe, but, you know.

(The only safe way a user of libkrun can probe an image’s format is by probing any guest image once before booting the guest for the first time (which is also the point where users should verify the image’s external dependencies (backing file, …) are safe to access), and then remembering and enforcing the format for all future uses of that file.)

@jakecorrenti
Copy link
Member Author

What I'm gathering from the feedback is that there are opportunities for misusing the disk image. Mitigation techniques:

  • Enforce the user specify the disk image format if they want to use something other than a raw file for the disk image. This could be an API like krun_add_disk_with_format(ctx_id, block_id, disk_path, disk_format, read_only) which German suggested. It's important to thoroughly document this for the user and emphasize avoiding probing at all costs. Libkrun should be the only one probing
  • If the user originally provided a raw file, prevent the guest from changing the image header to represent a formatted image. We can check on write if the guest is trying to write a different header. But we also need to prevent the guest trying to write a different header one byte at a time.

@germag
Copy link
Contributor

germag commented Nov 15, 2024

What I'm gathering from the feedback is that there are opportunities for misusing the disk image. Mitigation techniques:

* Enforce the user specify the disk image format if they want to use something other than a raw file for the disk image. This could be an API like `krun_add_disk_with_format(ctx_id, block_id, disk_path, disk_format, read_only)` which German suggested. It's important to thoroughly document this for the user and emphasize avoiding probing at all costs. Libkrun should be the only one probing

Can probing inside libkrun be avoided, expecting the lib user (like krunkit) to do the probing?

* If the user originally provided a raw file, prevent the guest from changing the image header to represent a formatted image. We can check on write if the guest is trying to write a different header. But we also need to prevent the guest trying to write a different header one byte at a time.

Idk if it's a good idea: the mitigation should be optional, like an extra parameter in krun_add_disk_with_format()

...it's not a good idea

But we also need to prevent the guest trying to write a different header one byte at a time.

...and in any order

@jakecorrenti
Copy link
Member Author

What I'm gathering from the feedback is that there are opportunities for misusing the disk image. Mitigation techniques:

* Enforce the user specify the disk image format if they want to use something other than a raw file for the disk image. This could be an API like `krun_add_disk_with_format(ctx_id, block_id, disk_path, disk_format, read_only)` which German suggested. It's important to thoroughly document this for the user and emphasize avoiding probing at all costs. Libkrun should be the only one probing

Can probing inside libkrun be avoided, expecting the lib user (like krunkit) to do the probing?

What I gather from Hanna's comment (#237 (comment)) is yes we can. We would just need to ensure that the probe is happening once and before first boot. But it sounds like it would make one of the mitigation options irrelevant.

* If the user originally provided a raw file, prevent the guest from changing the image header to represent a formatted image. We can check on write if the guest is trying to write a different header. But we also need to prevent the guest trying to write a different header one byte at a time.

Idk if it's a good idea: the mitigation should be optional, like an extra parameter in krun_add_disk_with_format()

...it's not a good idea

But we also need to prevent the guest trying to write a different header one byte at a time.

...and in any order

@XanClic
Copy link

XanClic commented Nov 15, 2024

Can probing inside libkrun be avoided, expecting the lib user (like krunkit) to do the probing?

What I gather from Hanna's comment (#237 (comment)) is yes we can. We would just need to ensure that the probe is happening once and before first boot. But it sounds like it would make one of the mitigation options irrelevant.

Indeed.

However, I personally highly doubt it is reasonable to expect every libkrun user to always ensure they probe in this secure manner. To me, it feels like just pushing the security issue can down the road, so that if something happens we can point at the documentation and say “we warned you”.

Then again, that’ll be done anyway for the question of whether a qcow2 image is safe to open, i.e. consider a downloaded qcow2 image that has /etc/shadow as its backing file[1]. libkrun can’t check whether those file references are safe, the user must do that, and who knows whether they will (experience says probably not).

...it's not a good idea

But we also need to prevent the guest trying to write a different header one byte at a time.

...and in any order

As I described, doable if you always write the header in an RMW manner, i.e. if anything is written to the first sector at all, the whole first sector is read, the new data is embedded, then you check the resulting sector data as a whole, and write it as a whole, thus ensuring you never write an image header into it.

[1] Yes, I know, shouldn’t be accessible anyway, it’s just an example.

@germag
Copy link
Contributor

germag commented Nov 15, 2024

Can probing inside libkrun be avoided, expecting the lib user (like krunkit) to do the probing?

What I gather from Hanna's comment (#237 (comment)) is yes we can. We would just need to ensure that the probe is happening once and before first boot. But it sounds like it would make one of the mitigation options irrelevant.

Indeed.

However, I personally highly doubt it is reasonable to expect every libkrun user to always ensure they probe in this secure manner. To me, it feels like just pushing the security issue can down the road, so that if something happens we can point at the documentation and say “we warned you”.

good point

Then again, that’ll be done anyway for the question of whether a qcow2 image is safe to open, i.e. consider a downloaded qcow2 image that has /etc/shadow as its backing file[1]. libkrun can’t check whether those file references are safe, the user must do that, and who knows whether they will (experience says probably not).

I was thinking about an API that can return those paths to the libkrun user, so the user can check if those paths are safe. But following your first point, that can also be pushing the security on the user shoulders. Idk how cumbersome will be adding (and using) a "safe" API, that for instance requires a list of allowed/deny locations/path prefixes for backing files.

...it's not a good idea

But we also need to prevent the guest trying to write a different header one byte at a time.

...and in any order

As I described, doable if you always write the header in an RMW manner, i.e. if anything is written to the first sector at all, the whole first sector is read, the new data is embedded, then you check the resulting sector data as a whole, and write it as a whole, thus ensuring you never write an image header into it.

[1] Yes, I know, shouldn’t be accessible anyway, it’s just an example.

@XanClic
Copy link

XanClic commented Nov 15, 2024

Idk how cumbersome will be adding (and using) a "safe" API, that for instance requires a list of allowed/deny locations/path prefixes for backing files.

If we come up with something that seems nice and generic enough, it could be done right in imago so libkrun could continue to use open_implicit_dependencies() and wouldn’t need to re-implement recursively opening the backing chain. Could be an Fn(&str) -> bool callback that’s invoked for every file imago would like to open.

@germag
Copy link
Contributor

germag commented Nov 15, 2024

Idk how cumbersome will be adding (and using) a "safe" API, that for instance requires a list of allowed/deny locations/path prefixes for backing files.

If we come up with something that seems nice and generic enough, it could be done right in imago so libkrun could continue to use open_implicit_dependencies() and wouldn’t need to re-implement recursively opening the backing chain. Could be an Fn(&str) -> bool callback that’s invoked for every file imago would like to open.

I like this idea

@jakecorrenti
Copy link
Member Author

In the latest batch of changes I tried to implement the mitigations in order to prevent the guest from misusing the disk image

@jakecorrenti jakecorrenti force-pushed the qcow2-support branch 2 times, most recently from 0b8eeef to 60076bd Compare November 18, 2024 05:16
include/libkrun.h Outdated Show resolved Hide resolved
src/devices/src/virtio/block/device.rs Outdated Show resolved Hide resolved
src/devices/src/virtio/block/disk/mod.rs Outdated Show resolved Hide resolved
@jakecorrenti
Copy link
Member Author

I just want to do a quick recap to make sure I'm getting everything:

  • We still need to come to a conclusion about probing. Does imago do it? Libkrun? Or push it onto the libkrun consumer?
  • To move this forward, keep krun_add_disk for adding Raw files. Use krun_add_disk2 to add Qcow2 files. Don't do any probing right now (essentially getting rid of commit fd6203f) and get rid of the KRUN_DISK_FORMAT_{UNSPECIFIED, RAW} constants.
  • We should come up with a safe and sane API that will be ready for libkrun 2.0. This means investigating what should be done in libkrun and/or imago.

@slp
Copy link
Contributor

slp commented Nov 20, 2024

I just want to do a quick recap to make sure I'm getting everything:

  • We still need to come to a conclusion about probing. Does imago do it? Libkrun? Or push it onto the libkrun consumer?

Neither libkrun nor imago (on behalf of libkrun) should do any probing. There's no reasonable use case for that, and the risks are significant (this was a hard-learnt lesson in QEMU). It's up to the library consumers to explicitly tell us the image format.

  • To move this forward, keep krun_add_disk for adding Raw files. Use krun_add_disk2 to add Qcow2 files. Don't do any probing right now (essentially getting rid of commit fd6203f) and get rid of the KRUN_DISK_FORMAT_{UNSPECIFIED, RAW} constants.

Please remove UNSPECIFIED but keep RAW. There may be users preferring to always use krun_add_disk2 for consistency, and also makes the transition to 2.0 easier for us.

(BTW, thanks for this work!)

include/libkrun.h Outdated Show resolved Hide resolved
@jakecorrenti jakecorrenti force-pushed the qcow2-support branch 2 times, most recently from 7593e31 to 46f797e Compare November 20, 2024 20:14
@jakecorrenti
Copy link
Member Author

Hi everyone, thank you for all of the comments. I pushed a set of changes that should address everything. PTAL when you get a chance.

src/devices/src/virtio/file_traits.rs Outdated Show resolved Hide resolved
src/devices/src/virtio/file_traits.rs Outdated Show resolved Hide resolved
@germag
Copy link
Contributor

germag commented Nov 22, 2024

LGTM

Copy link

@XanClic XanClic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One general thing: I’d switch commits 3 and 4. Commit 3 introduces the external interface for qcow2 images, but internally, the format is actually ignored. So the internal capability to handle qcow2 images should come before.

I know, it technically doesn’t matter, as long as nobody looks between commits 3 and 4.

src/devices/src/virtio/block/disk/mod.rs Outdated Show resolved Hide resolved
include/libkrun.h Outdated Show resolved Hide resolved
include/libkrun.h Outdated Show resolved Hide resolved
include/libkrun.h Outdated Show resolved Hide resolved
src/devices/src/virtio/block/disk/mod.rs Outdated Show resolved Hide resolved
src/libkrun/src/lib.rs Show resolved Hide resolved
src/devices/src/virtio/file_traits.rs Outdated Show resolved Hide resolved
@jakecorrenti
Copy link
Member Author

I still need to swap commits 2 and 3 which requires a couple changes, but I wanted to push everything else first.

@jakecorrenti jakecorrenti force-pushed the qcow2-support branch 2 times, most recently from c110ec7 to 17fc841 Compare November 22, 2024 16:50
@jakecorrenti
Copy link
Member Author

Squashed a commit, reordered them, and updated the commit messages. Let me know if there's anything else :)

Copy link

@XanClic XanClic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the comments, looks good to me, thanks!

include/libkrun.h Outdated Show resolved Hide resolved
include/libkrun.h Outdated Show resolved Hide resolved
src/devices/Cargo.toml Outdated Show resolved Hide resolved
Adds the imago crate which is necessary to allow support for multiple
disk image formats.

Signed-off-by: Jake Correnti <[email protected]>
Changes the `file` attribute in `DiskProperties` to use
`Arc<SyncFormatAccess<ImagoFile>>`. This allows libkrun to take
advantage of imago's support for multiple disk image formats. Libkrun
now supports the use of Raw or QCOW2 formats for disk images.

Implements `FileReadWriteAtVolatile` trait on `DiskProperties`

Adds an emum, `ImageType`, that describes the supported disk image
variants.

Signed-off-by: Jake Correnti <[email protected]>
Adds the `krun_add_disk2` API that requires the user to specify the
format of the disk image they're providing.

The following formats are supported:
- KRUN_DISK_FORMAT_RAW
- KRUN_DISK_FORMAT_QCOW2

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Copy link
Member Author

@slp PTAL

@jakecorrenti
Copy link
Member Author

New Code Quality failures are interesting considering no changes to the code were made... Something I should worry about in this PR?

@germag
Copy link
Contributor

germag commented Nov 29, 2024

New Code Quality failures are interesting considering no changes to the code were made... Something I should worry about in this PR?

It seems it's in a file you didn't touched, you can open a new PR to change this:

impl<'a> HvfVcpu<'a> {

to this

impl HvfVcpu<'_> {

in src/hvf/src/lib.rs:250

Copy link
Contributor

@slp slp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakecorrenti Don't worry about the CI issues, I'll fix them in a different PR. Let's merge this one already.

@slp slp merged commit 3466191 into containers:main Nov 29, 2024
2 of 5 checks passed
@slp
Copy link
Contributor

slp commented Nov 29, 2024

@jakecorrenti @XanClic @tylerfanelli @germag Now, the one million question: who volunteers to package imago for Fedora? ;-)

@XanClic
Copy link

XanClic commented Dec 2, 2024

Now, the one million question: who volunteers to package imago for Fedora? ;-)

🙈 I’ll go figure it out.

@germag
Copy link
Contributor

germag commented Dec 2, 2024

Now, the one million question: who volunteers to package imago for Fedora? ;-)

🙈 I’ll go figure it out.

I can help you with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants