virtio-queue: Error out on invalid available ring index #3156

dianpopa · 2022-10-03T10:16:21Z

The number of descriptor chain heads to process should always be smaller or equal to the queue size, as the driver should never ask the VMM to process a available ring entry more than once. Checking and reporting such incorrect driver behavior can prevent potential hanging and Denial-of-Service from happening on the VMM side.

Issue reported in rust-vmm/vm-virtio and fixed in
rust-vmm/vm-virtio#196.

Signed-off-by: Diana Popa dpopa@amazon.com
Co-authored-by: Bo Chen chen.bo@intel.com

Fixes #3149.

License Acceptance

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

PR Checklist

All commits in this PR are signed (git commit -s).
If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
New unsafe code is documented.
API changes follow the Runbook for Firecracker API changes.
User-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

This functionality can be added in rust-vmm.

JonathanWoollett-Light · 2022-10-03T13:33:12Z

src/devices/src/virtio/queue.rs

+        if len > self.actual_size() {
+            // We are choosing to interrupt execution since this could be a potential malicious driver
+            // scenario. This way we also eliminate the risk of repeatedly logging and potentially
+            // clogging the microVM through the log system.
+            panic!("The number of available virtio descriptors is greater than queue size!");
+        }


Suggested change

if len > self.actual_size() {

// We are choosing to interrupt execution since this could be a potential malicious driver

// scenario. This way we also eliminate the risk of repeatedly logging and potentially

// clogging the microVM through the log system.

panic!("The number of available virtio descriptors is greater than queue size!");

}

// We are choosing to interrupt execution since this could be a potential malicious driver

// scenario. This way we also eliminate the risk of repeatedly logging and potentially

// clogging the microVM through the log system.

assert!(len > self.actual_size(), "The number of available virtio descriptors is greater than queue size!");

I prefer to use panic over assert in production code.
At least, this is what we have targeted to do until now so that it is easier to track the panic branches we have in production code.
Also, we have a panic handler installed when starting Firecracker (for flushing metrics and whatnot) which I am not sure would get called if we use asssert.

At least, this is what we have targeted to do until now so that it is easier to track the panic branches we have in production code.

Could you expand, how do we currently track panic branches?

Also, we have a panic handler installed when starting Firecracker (for flushing metrics and whatnot) which I am not sure would get called if we use asssert.

The docs for std::assert specify:

This will invoke the panic! macro if the provided expression cannot be evaluated to true at runtime.

At least, this is what we have targeted to do until now so that it is easier to track the panic branches we have in production code.

Could you expand, how do we currently track panic branches?

We do not have an automated process or something of the kind. But if we were to use asserts in production code (which are extensively used throughout unit tests) is going to be quite challenging to uncover those if the need arises. That would be one of the practical aspects of it.

Also, we have a panic handler installed when starting Firecracker (for flushing metrics and whatnot) which I am not sure would get called if we use asssert.

The docs for std::assert specify:

This will invoke the panic! macro if the provided expression cannot be evaluated to true at runtime.

Nice, thanks for looking this up!
Can you tell me is there is a strong reason for using assert over panic here?

I prefer to use panic over assert in production code. At least, this is what we have targeted to do until now so that it is easier to track the panic branches we have in production code. Also, we have a panic handler installed when starting Firecracker (for flushing metrics and whatnot) which I am not sure would get called if we use asssert.

It is simpler, but beyond that, no strong reasons.

JonathanWoollett-Light · 2022-10-03T13:34:48Z

src/devices/src/virtio/queue.rs

+            if len > self.actual_size() {
+                // We are choosing to interrupt execution since this could be a potential malicious driver
+                // scenario. This way we also eliminate the risk of repeatedly logging and potentially
+                // clogging the microVM through the log system.
+                panic!("The number of available virtio descriptors is greater than queue size!");
+            }


Suggested change

if len > self.actual_size() {

// We are choosing to interrupt execution since this could be a potential malicious driver

// scenario. This way we also eliminate the risk of repeatedly logging and potentially

// clogging the microVM through the log system.

panic!("The number of available virtio descriptors is greater than queue size!");

}

// We are choosing to interrupt execution since this could be a potential malicious driver

// scenario. This way we also eliminate the risk of repeatedly logging and potentially

// clogging the microVM through the log system.

assert!(len > self.actual_size(), "The number of available virtio descriptors is greater than queue size!");

See above (#3156 (comment)).

src/devices/src/virtio/queue.rs

JonathanWoollett-Light · 2022-10-04T09:26:20Z

src/devices/src/virtio/queue.rs

+        for j in 0..4 {
+            vq.dtable[j].set(
+                0x1000 * (j + 1) as u64,
+                0x1000,
+                VIRTQ_DESC_F_NEXT,
+                (j + 1) as u16,
+            );
+        }


Nit: It is always safe to cast a u64 into a usize but it is not always safe to do the reverse (with stricter linting this would currently raise a warning).

Suggested change

for j in 0..4 {

vq.dtable[j].set(

0x1000 * (j + 1) as u64,

0x1000,

VIRTQ_DESC_F_NEXT,

(j + 1) as u16,

);

}

for j in 0u64..4u64 {

vq.dtable[usize::from(j)].set(

0x1000 * (j + 1),

0x1000,

VIRTQ_DESC_F_NEXT,

(u16::from(j) + 1),

);

}

This is a good point @JonathanWoollett-Light! However, this applies to all queue tests in this file. Can you please open an issue for this? Thanks!

With a wider context, maybe an issue should relate to adding #![warn(clippy::as_conversions)] (https://rust-lang.github.io/rust-clippy/master/)?

Or maybe modify add this restriction in the cargo clippy command directly if possible.

Created basic issue #3161

JonathanWoollett-Light · 2022-10-04T09:27:52Z

src/devices/src/virtio/queue.rs

+        for j in 0..4 {
+            vq.dtable[j].set(
+                0x1000 * (j + 1) as u64,
+                0x1000,
+                VIRTQ_DESC_F_NEXT,
+                (j + 1) as u16,
+            );
+        }


Nit: It is always safe to cast a u64 into a usize but it is not always safe to do the reverse (with stricter linting this would currently raise a warning).

Suggested change

for j in 0..4 {

vq.dtable[j].set(

0x1000 * (j + 1) as u64,

0x1000,

VIRTQ_DESC_F_NEXT,

(j + 1) as u16,

);

}

for j in 0u64..4u64 {

vq.dtable[usize::from(j)].set(

0x1000 * (j + 1),

0x1000,

VIRTQ_DESC_F_NEXT,

(u16::from(j) + 1),

);

}

Applies to all virtio queue unit tests. We need to open an issue for this.

We would also take into account newlines when checking that lines following co-author are only additional co-authors. Signed-off-by: Diana Popa <dpopa@amazon.com>

The number of descriptor chain heads to process should always be smaller or equal to the queue size, as the driver should never ask the VMM to process a available ring entry more than once. Checking and reporting such incorrect driver behavior can prevent potential hanging and Denial-of-Service from happening on the VMM side. Issue reported in rust-vmm/vm-virtio and fixed in rust-vmm/vm-virtio#196. Signed-off-by: Diana Popa <dpopa@amazon.com> Co-authored-by: Bo Chen <chen.bo@intel.com>

dianpopa self-assigned this Oct 3, 2022

dianpopa requested a review from a team October 3, 2022 10:27

dianpopa added Type: Bug Indicates an unexpected problem or unintended behavior NextRelease labels Oct 3, 2022

dianpopa force-pushed the fix_virtio_dos branch 2 times, most recently from 62cbafc to 9c71485 Compare October 3, 2022 13:15

JonathanWoollett-Light reviewed Oct 3, 2022

View reviewed changes