Enhance virtio subsystem for better comformance to virtio spec #809

jiangliu · 2019-01-03T11:56:19Z

Issue #, if available:

Description of changes:
There are some bugs in virtio subsystem which may cause firecracker hypervisor panicking, so enhance the virtio subsystem for better Spec conformance and avoid panic.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The VirtIO Spec 1.0, sec 2.4 defines the alignment constraints for VirtIO queue's descriptor table, available ring and used ring. So enforce the alignment constraints in Queue::is_valid(). Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

acatangiu · 2019-01-03T12:18:36Z

nit: change the commit summary of bbe0b4c from "virtio: fix a potential attack channel through MMIO transport layer" to something less sensational like: "virtio: harden the MMIO transport spec enforcements" or "virtio: enhance for better comformance to virtio spec".

acatangiu · 2019-01-03T12:29:23Z

Rust style test failed, please run cargo fmt --all and amend the offending commit.

jiangliu · 2019-01-03T13:21:17Z

thanks for review, I have updated the changeset according to the review comments:)

acatangiu · 2019-01-04T12:48:32Z

devices/src/virtio/mmio.rs

+        LittleEndian::write_u32(&mut buf[..], 0x8f);
+        d.write(0x70, &buf[..]);
+        assert_eq!(d.driver_status, 0x8f);
+        // TODO: assert!(d.device_activated);


Why assert!(d.device_activated) again? Is there any path to de-activate an activated device?

This is a reminder that there still two cases unhandled yet:

virtio device driver writes FALIED flags to driver_status field;

virtio device driver writes 0 to driver_status fields to reset device;

Still thinking about how to handle above two state transitions which may affect the device_activated flag. The block issue is that VirtioDevice::reset() is defined as optional and the net/block virtio backend driver doesn't support reset yet.

Please create issues for outstanding TODOs and reference them in the codebase like:

// TODO (#xxx): issue-xxx-summary

Having said that, please be aware that we've yet found no customer need to support device reset.

acatangiu · 2019-01-04T12:49:16Z

devices/src/virtio/mmio.rs

+
+        LittleEndian::write_u32(&mut buf[..], 0x0);
+        d.write(0x70, &buf[..]);
+        // TODO: assert_eq!(d.driver_status, 0x0);


This looks like a valid check, uncomment it.

acatangiu · 2019-01-04T13:50:18Z

devices/src/virtio/queue.rs

@@ -322,9 +322,6 @@ impl Queue {

    /// A consuming iterator over all available descriptor chain heads offered by the driver.
    pub fn iter<'a, 'b>(&'b mut self, mem: &'a GuestMemory) -> AvailIter<'a, 'b> {
-        if !self.is_valid(mem) {


I'm not 100% sure about this. Your logic is right, but we need to be very confident there is no way to alter any of these queue params after device activation, otherwise removing this opens up a security hole.

Other reviewers, please think hard on this part.

I've reviewed all accesses of fields validated by this function and they are indeed not modified anywhere after VirtDevice::activate() 👍

acatangiu · 2019-01-04T15:01:11Z

memory_model/src/guest_memory.rs

+    where
+        F: FnMut(&MemoryMapping, usize, usize) -> Result<()>,
+    {
+        // Fast path: content doesn't cross MemoryRegion boundary


Fast path is the same code as do_in_region(). Why not call that here directly and in case of Err move to the slow-path.

Would help de-duplicate code.

do_in_region_full() has been removed.

acatangiu · 2019-01-04T15:22:29Z

devices/src/virtio/mmio.rs

+                    == (DEVICE_ACKNOWLEDGE | DEVICE_DRIVER | DEVICE_FEATURES_OK) =>
+            {
+                self.driver_status = v;
+                if !self.device_activated && self.are_queues_valid() {


nit: add a comment here:

// If the driver incorrectly sets up the queues, the following check will fail and take the device into an unusable state.

I think it's important to call out this scenario since it's something we can't currently recover from.

devices/src/virtio/mmio.rs

acatangiu · 2019-01-04T15:28:55Z

memory_model/src/guest_memory.rs

@@ -424,6 +451,52 @@ impl GuestMemory {
        }
        Err(Error::InvalidGuestAddress(guest_addr))
    }
+
+    /// Read the whole content from one or more MemoryRegions
+    fn do_in_region_full<F>(&self, guest_addr: GuestAddress, size: usize, mut cb: F) -> Result<()>


I don't really see the point to this (big) function. It only adds value when trying to read/write an object which spans across multiple adjacent regions.

We never configure adjacent memory regions. There is either:

A single contiguous region.

A region followed by a hole followed by a second (non-adjacent) region.

I would drop do_in_region_full().

Make sense. It only matters when enabling memory hot-adding. Will remove do_in_region_full().

Hi acatangiu,
I just sent out an RFC to complete the virtio device state machine, which may help to review related patches. Will address other review comments later.
Thank!

dhrgit

Splitting an object across memory regions only makes sense if the regions are adjacent. In our case, they aren't. If they were, we could simply merge them into a single region and remove some implementation complexity.

There are at most two memory regions, separated by the 32-bit PCI gap. Check out vmm/src/lib.rs::init_guest_memory and x86_64/src/lib.rs::arch_memory_regions for the mem setup code.

dianpopa · 2019-01-04T12:09:34Z

devices/src/virtio/mmio.rs

+        if self.check_driver_status(DEVICE_FEATURES_OK, DEVICE_DRIVER_OK | DEVICE_FAILED) {
+            self.with_queue_mut(f);
+        } else {
+            #[cfg(not(release))]


Why wouldn t we want to log this warning on release versions? The MmioDevice::write() and MmioDevice::read() have similar log messages and we do log them.

It's to avoid injecting to many log messages to the log system by a malicious guest kernel. And it's ok to remove the conditional compilation.

dianpopa · 2019-01-04T12:49:00Z

devices/src/virtio/mmio.rs

+    fn update_driver_status(&mut self, v: u32) {
+        // match changed bits
+        match !self.driver_status & v {
+            DEVICE_ACKNOWLEDGE if self.driver_status == 0 => {


What do you say we declare a constant for the 0 value (i.e the value that is used to initialize the driver_status field).

Ok(MmioDevice { device: device, device_activated: false, features_select: 0, acked_features_select: 0, queue_select: 0, interrupt_status: Arc::new(AtomicUsize::new(0)), interrupt_evt: Some(EventFd::new()?), driver_status: 0, config_generation: 0, queues: queues, queue_evts: queue_evts, mem: Some(mem), })

Even if the probability is small, If we were to change this value to something else in the init code, the state machine algorithm would fail.

dianpopa · 2019-01-04T13:49:09Z

devices/src/virtio/mmio.rs

+                        if self
+                            .check_driver_status(DEVICE_DRIVER, DEVICE_FEATURES_OK | DEVICE_FAILED)
+                        {
+                            self.device.ack_features(self.acked_features_select, v);


What happens if after setting the DEVICE_DRIVER bit during initialization, a reset is issued by the guest os?
Should we still acknowledge features?

A RESET command will cause the device goes into INIT state, and the guest OS need to reinitialize the device from start. So we don't acknowledge the features after a RESET until DEVICE_DRIVER is set again.

dianpopa · 2019-01-04T14:58:11Z

devices/src/virtio/mmio.rs

                    _ => {
                        warn!("unknown virtio mmio register write: 0x{:x}", offset);
                        return;
                    }
                }
            }
-            0x100...0xfff => return self.device.write_config(offset - 0x100, data),
+            0x100...0xfff => {
+                if self.check_driver_status(DEVICE_DRIVER, DEVICE_FAILED) {


From the spec I understand that the config space can only be written after FEATURES_OK has been set:
Step 4 from 3.1.1 section:
4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
fields to check that it can support the device before accepting it.
Step 7 from 3.1.1
7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
reading and possibly writing the device’s virtio configuration space, and population of virtqueues

That's true, so we only the device state on MmioDevice::write() and let it go in MmioDevice::read().

dianpopa · 2019-01-04T15:02:19Z

devices/src/virtio/mmio.rs

+        LittleEndian::write_u32(&mut buf[..], 0x8f);
+        d.write(0x70, &buf[..]);
+        assert_eq!(d.driver_status, 0x8f);
+        // TODO: assert!(d.device_activated);


Why is this a TODO? Same for the other following TODOs?

These TODOs have been covered by the last RFC patch.

dianpopa · 2019-01-04T16:07:40Z

memory_model/src/guest_memory.rs

    }

    /// Returns the address plus the offset if it is in range.
    pub fn checked_offset(&self, addr: GuestAddress, offset: usize) -> Option<GuestAddress> {
-        addr.checked_add(offset)
-            .and_then(|a| if a < self.end_addr() { Some(a) } else { None })
+        if let Some(tgt) = addr.checked_add(offset) {


nit: I think it would be nice to rename tgt to something more intuitive?

renamed addr as base and tgt as addr, so it becomes addr = base + offset.

dianpopa · 2019-01-04T16:11:36Z

By using the above command you can check the coverage of the whole project:

cd tests
sudo env "PATH=$PATH" ./testrun.sh -p apt-get -i /home/local/ANT/dpopa/firecracker/util/s3-images-oct/ -r integration_tests/build/test_coverage.py

Unfortunately, we still do not have a dynamic process in place for making sure that the coverage does not decrease from a commit to another.
However, I noticed that before your commits the coverage was around 82.2% and after them it dropped to 81.8%. Can you please make sure that newly added functions/code branches are covered by unit tests?

jiangliu · 2019-01-04T16:13:22Z

Great, will handle the coverage issue in next round.

acatangiu · 2019-01-04T16:14:40Z

devices/src/virtio/mmio.rs

@@ -183,6 +183,22 @@ impl MmioDevice {
        }
    }

+    fn reset(&mut self) {
+        assert!(!self.device_activated);


Don't do assert in release code. I suggest logging an error and moving on (in this case return-ing).

Please remove the leftover assert! too.

sorry for my mistake, fixed:)

acatangiu · 2019-01-04T16:19:33Z

devices/src/virtio/mmio.rs

+        self.interrupt_status.store(0, Ordering::SeqCst);
+        self.driver_status = 0;
+        // . Keep interrupt_evt and queue_evts as is. There may be pending
+        //   notificaitons in those eventfds, but nothing will happen other


nit: typo in notifications

acatangiu

Changes look good to me (including the final RFC commit handling device reset). Please address outstanding comments and I will approve the PR.

acatangiu · 2019-01-04T16:57:58Z

devices/src/virtio/queue.rs

@@ -322,9 +322,6 @@ impl Queue {

    /// A consuming iterator over all available descriptor chain heads offered by the driver.
    pub fn iter<'a, 'b>(&'b mut self, mem: &'a GuestMemory) -> AvailIter<'a, 'b> {
-        if !self.is_valid(mem) {


I've reviewed all accesses of fields validated by this function and they are indeed not modified anywhere after VirtDevice::activate() 👍

The MMIO based VirtIO devices support the VirtIO 1.0 capability, but the code is a little hard to be understood. Improve code readability by adding a comment. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

The VirtIO queue code in queue.rs assumes all data structures, including the descriptor table, available ring and used ring, are consistent and legal once Queue::is_valid() returns true for an queue. If this assumption is broken, it may cause panic. On the other hand, most fields in a working queue object, including size, ready, desc_table, avail_ring and used_ring, could be modified by the MmioDevice::write() method. That means an untrusted guest kernel could easily break the assumption made by queue.rs, and then cause the firecracker hypervisor panic. So enhance the VirtIO MMIO transport driver to strictly follow the state machine defined in the VirtIO Spec IO. It closes the way for an untrusted guest kernel to modify firecracker internal data structures. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

The lifecycle of VirtIO queue object is as below: 1) MMIO transport driver creates queues 2) MMIO transport validates all queues are valid by calling Queue::is_valid() 3) Passing queues to VirtDevice by calling VirtDevice::activate() 4) VirtDevice device driver creates iterator to access available descritpors 5) MMIO transport reset the VirtDevice by calling VirtDevice::reset() 6) MMIO transport changes/destroys queues The MMIO transport driver won't touch those queues between stage 3 and stage 5, so Queue::is_valid will always return true during stage 4. Get rid of the redundant invocation of Queue::is_valid() in Queue::iter(). Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

Fetch the entire VirtIO descriptor all at once instead of fetching each field one by one. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

Split the memory copy operations into to cases: 1) Full copy by do_in_region() which must copy all requested memory content or fails. Pass an additional parameter 'size' into function do_in_region(), and validate memory region before copying memory content. By this way the destination memory will be either fully copied or untouched. 2) Partial copy by do_in_region_patial() which copies memory as much as possible and returns size of copied memory content. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

GuestMemory::address_in_range() and GuestMemory::checked_offset() assumes memory address is continuous. It's a dangerous assumption because there is a black hole just below 4G when guest memory is about 4G or more. Then it may cause invalid memory access in VirtIO subsystem. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

Complete the virtio device state machine by: 1) Handle writing FAILED flag into "driver status" control field. 2) Handle writing 0 into "driver status" control field to reset the device. If the the backend driver supports reset operation, the device will be reset. Otherwise the device will be put into FAILED state. With these changes, the virtio mmio transport layer implements a full state machine according to Virtio Spec. The backend driver is still optional to support reset operation. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

acatangiu · 2019-01-07T12:25:53Z

@jiangliu I see your comments on our review, but I think you forgot to push the changes as well 😄
I still see the original version of the code.

jiangliu · 2019-01-07T13:00:00Z

@jiangliu I see your comments on our review, but I think you forgot to push the changes as well 😄
I still see the original version of the code.
I refined the code according to review comments, but fails to run CI code coverage test due to env issues, so delayed the push. Still trying to setup the CI env:)

jiangliu · 2019-01-07T13:37:22Z

Hi,
Any suggestion about this failure? I think my changes shouldn't affect the boot time too much, and don't know how to fix this failure too.
=================================== FAILURES ===================================
__________________________ test_startup_time[ubuntu] ___________________________
integration_tests/performance/test_process_startup_time.py:49: in test_startup_time
assert cpu_startup_time_us <= MAX_STARTUP_TIME_CPU_US
E assert 8616 <= 8000

acatangiu · 2019-01-07T13:55:26Z

@jiangliu test_startup_time measures the time taken from the fork()/clone() into the jailer to the first instructions run inside firecracker. This test has some variability depending on the machine its being run on. It sometimes fails for my local setup too. Don't worry about it, it should work fine on the AWS i3.metal instance.

andreeaflorescu · 2019-01-07T13:59:37Z

@acatangiu the test is failing as part of our CI. We should take a look.

acatangiu · 2019-01-07T14:00:15Z

Oooopsie, now I see that it actually failed in our C.I.

acatangiu · 2019-01-07T14:07:09Z

@jiangliu I think it was just a system load hiccup. Please rebase your changes against latest upstream master git pull git@github.com:firecracker-microvm/firecracker.git master --rebase and force push to update the PR. That should retry the C.I. and I'm expecting it to go through nicely.

acatangiu

Please remove the RFC from the last commit summary. Code changes look good 👍 !

dhrgit

Great work, @jiangliu - thank you!

jiangliu force-pushed the bugfix/virtio branch from 563a5f5 to db07077 Compare January 3, 2019 13:17

dianpopa assigned jiangliu Jan 3, 2019

raduweiss added Feature: IO Virtualization Type: Bug Indicates an unexpected problem or unintended behavior labels Jan 3, 2019

dianpopa self-requested a review January 4, 2019 11:27

acatangiu self-requested a review January 4, 2019 11:28

acatangiu reviewed Jan 4, 2019

View reviewed changes

devices/src/virtio/mmio.rs Show resolved Hide resolved

acatangiu reviewed Jan 4, 2019

View reviewed changes

dhrgit reviewed Jan 4, 2019

View reviewed changes

dianpopa reviewed Jan 4, 2019

View reviewed changes

acatangiu reviewed Jan 4, 2019

View reviewed changes

jiangliu added 6 commits January 6, 2019 23:03

virtio: improve code readability of MmioDevice::read()

8ea84b4

The MMIO based VirtIO devices support the VirtIO 1.0 capability, but the code is a little hard to be understood. Improve code readability by adding a comment. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

virtio: optimize the way to fetch VirtIO descriptor

17415b5

Fetch the entire VirtIO descriptor all at once instead of fetching each field one by one. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>

jiangliu force-pushed the bugfix/virtio branch from 7a9aeaf to 0f720b6 Compare January 7, 2019 12:58

jiangliu force-pushed the bugfix/virtio branch from e23954a to c3a0b37 Compare January 7, 2019 15:35

acatangiu previously approved these changes Jan 7, 2019

View reviewed changes

jiangliu dismissed acatangiu’s stale review via 680b910 January 8, 2019 01:30

jiangliu force-pushed the bugfix/virtio branch from c3a0b37 to 680b910 Compare January 8, 2019 01:30

acatangiu approved these changes Jan 8, 2019

View reviewed changes

dhrgit approved these changes Jan 8, 2019

View reviewed changes

alexandruag approved these changes Jan 8, 2019

View reviewed changes

Merge branch 'master' into bugfix/virtio

237ece6

andreeaflorescu merged commit 34bc630 into firecracker-microvm:master Jan 8, 2019

Enhance virtio subsystem for better comformance to virtio spec #809

Enhance virtio subsystem for better comformance to virtio spec #809

Conversation

jiangliu commented Jan 3, 2019

acatangiu commented Jan 3, 2019

acatangiu commented Jan 3, 2019

jiangliu commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu Jan 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Jan 4, 2019

jiangliu commented Jan 4, 2019

acatangiu Jan 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu commented Jan 7, 2019

jiangliu commented Jan 7, 2019

jiangliu commented Jan 7, 2019

acatangiu commented Jan 7, 2019 • edited

andreeaflorescu commented Jan 7, 2019

acatangiu commented Jan 7, 2019

acatangiu commented Jan 7, 2019

acatangiu left a comment

Choose a reason for hiding this comment

dhrgit left a comment

Choose a reason for hiding this comment

acatangiu Jan 4, 2019 •

edited

acatangiu Jan 4, 2019 •

edited

jiangliu Jan 6, 2019 •

edited

acatangiu Jan 4, 2019 •

edited

acatangiu commented Jan 7, 2019 •

edited