rust-vmm / vm-superio Goto Github PK

We have a bug in the RTC implementation by which we are not setting the correct value to the Load Register (LR). This register is suppose to be initialized with the time elapsed since the UNIX Epoch, otherwise the guest will have the time set to 1970.

We should talk about:

• input/output (is it trusted/untrusted?)
• threats & mitigations

The first version of this crate will contain a minimal serial console implementation.

The RTC is needed for booting an aarch64 machine.

We can use the RTC implementation from Firecracker as a starting point: https://github.com/firecracker-microvm/firecracker/blob/master/src/devices/src/legacy/rtc_pl031.rs

Similarly to the serial console implementation, the RTC in vm-device should only handle the emulation part (event handling and Bus implementation should not be included as they're specific to the VMM consuming vm-superio).

Nice to have:

• metrics (these can also be defined later)

The README file notes that

The Fifo Control Register (FCR) is not emulated; there is no support yet for directly controlling the FIFOs (which, in this implementation, are always enabled).

but the Enabled bit (0x1) is not the only thing in the FCR; it also has bits for flushing the receive and transmit FIFOs (0x2, 0x4). FreeBSD makes use of these; for now I'm adding a workaround which checks if the flush was successful and manually drains the FIFO instead if not, but it would be great if this could be fixed in vm-superio.

Add metrics for the error conditions that can happen in the RTC device. For example, read & writes at invalid offsets.

If the out resource has a limited size (e.g. a named pipe), when it is at full capacity, a write request will result in blocking the driver indefinitely. This is caused by the THR empty interrupt no longer being sent, relevant lines here.
This could be fixed, for example by always sending the THRE interrupt (even if the write failed).

The BSD-3-Clause should be added as well (besides Apache License).

#1 uses an EventFd for notifying the driver about events. We should replace this rather specific mechanism with some abstraction.

We have identified a possible DoS issue in rust-vmm/vm-superio v0.1.0.

Issue Description

The rust-vmm/vm-superio implementation of the serial console which emulates a UART port type 16550A allows buffering an unlimited number of bytes from input sources when using the FIFO functionality. This issue can not be triggered from the guest side. This issue presents no impact to AWS Services.

Impact

All VMMs that are using the FIFO functionality to forward host-side input from an untrusted source to the guest can be subject to a DoS issue. This issue cannot be triggered from serial output generated by the guest. When no rate limiting is in place, the host can be subject to memory pressure, impacting all other VMs running on the same host. Rate limiting the input from the host side also mitigates the issue.

Affected Systems

rust-vmm/vm-superio v0.1.0.

Proposed Mitigation

Impact can be mitigated by upgrading to vm-superio 0.1.1, configuring memory limits to the process that is using vm-superio, or by rate limiting the writes to the process standard input.

After #26 was merged, the threat is still present but currently mitigated. The documentation should be updated accordingly.

Our serial console implements only one FIFO (the RX one).

We need to implement the TX FIFO as well because otherwise it is hard to follow the specification. Not implementing this fifo has the following side effect - we are flooding the guest with interrupts. In non-FIFO mode, an interrupt will be generated when the THR gets empty (therefore, for each received byte). In FIFO mode the interrupt is sent when the TX FIFO gets completely empty (every FIFO_SIZE bytes).
A 16550 spec example here. It is worth mentioning, the spec defines the FIFO_SIZE as 16 bytes for UART 16550(A). However, it can be for example 64 bytes as well, as it can be seen here.

Context

If you look at serial.rs, this is the current situation:
At the top, we have some const defined

//FIFO enabled.
const IIR_FIFO_BITS: u8 = 0b1100_0000;
const IIR_RDA_BIT: u8 = 0b0000_0100;
const IIR_NONE_BIT: u8 = 0b0000_0001;
const IIR_THR_EMPTY_BIT: u8 = 0b0000_0010;

The pattern is <register>_<field_name>.
An example of usage in the wild is:

    fn add_interrupt(&mut self, interrupt_bits: u8) {
        self.interrupt_identification &= !IIR_NONE_BIT;
        self.interrupt_identification |= interrupt_bits;
    }

Problem

1. The naming scheme suggests a possible grouping of these consts.
2. It's not intuitive what's going on with that code, due to manual bit manipulation.

Proposal

First proposal

I know it's not preferable to include external dependencies like bitfield but I think I simple macro will do the trick.

I would propose a macro which will allow the following:

/// Interrupt Identification Register
defreg!(IIR, u8, [
    FIFO[8-7],
    RDA[2-2],
    THR_EMPTY[1-1],
    NONE[0-0],
]);
// Example of usage:

   fn add_interrupt(&mut self, interrupt_bits: u8) {
        self.interrupt_identification.clear_bit(IIR::NONE);
        self.interrupt_identification.set_bit(interrupt_bits);
    }

The parameters are: the struct name, size of the wrapped value, and list of fields and position of those fields (e.g. NONE is one bit long, and it's the lsb).
We could also add another input for a default value (and remove all the DEFAULTconsts).

Second proposal

A simple struct based wrapper. Playground.

#[derive(Debug)]
struct IIR(u8);

impl IIR{
    const FIFO_BITS: u8 = 0b1100_0000;
    const NONE_BIT: u8 = 0b0000_0001;
    const THR_EMPTY_BIT: u8 = 0b0000_0010;
    const RDA_BIT: u8 = 0b0000_0100;
}

impl default::Default for IIR{
    fn default() -> Self{
        Self(IIR::NONE_BIT)
    }
}

We can then define the similar methods as above (clear_bit and set_bit).

• #34
• #25 - dropped

Think about whether it makes sense to have performance tests for serial console.

It looks like we're using an old version.

Once we have a metric module in vmm_sys_util (rust-vmm/vmm-sys-util#94), we can use it to add metrics for serial console.