Issues with safety traits and interrupting attestation

[L0czek]

As promised I re-pasted the description of #342 so we can continue the discussion after the PR was merged.

Encountered issues

Using improperly initialized memory

Upon Realm's REC creation in the rmi REC_CREATE handler Islet does the following:

        let mut rec_granule = get_granule_if!(rec, GranuleState::Delegated)?;
        #[cfg(not(kani))]
        // `page_table` is currently not reachable in model checking harnesses
        rmm.page_table.map(rec, true);
        let mut rec = rec_granule.content_mut::<Rec<'_>>()?;

It uses the SafetyAssured and SafetyAssumed traits to basically convert a raw pointer to a Rust reference. Later, the rec object is initialized by a safe function Rec::init:

        match prepare_args(&mut rd, params.mpidr) {
            Ok((vcpuid, vttbr, vmpidr)) => {
                ret[1] = vcpuid;
                rec.init(owner, vcpuid, params.flags, vttbr, vmpidr)?;
            }
            Err(_) => return Err(Error::RmiErrorInput),
        }

Inside this Rec::init function, someone forgot to initialize the attest_state field. This is of course a simple mistake, but I think that it was caused by a bigger issue. I understand that Rust doesn't yet have any syntax allowing to construct an object in-place by providing some pointer. The current solution implemented in Islet is to cast the raw pointer to Rust reference and then call a some init function which assumes that it is operating on a valid object. That's why it is possible to forget about some fields in a constructor. I think we should discuss better ways of handling this issue, as either the Rec::init function should be made unsafe as this initialization is semantically unsafe, or we should design a better mechanism. Naturally, marking the init function as unsafe will not solve this problem, but it will be a warning that here Rust cannot ensure complete safety. The other solution might be to implement a mechanism similar to how the Box pointer can be created using a custom memory allocator. On the high level, this ensures that the created object will be placed in a memory region controlled by the provided allocator. In this scenario, the object is first created on the stack using the Rust typical struct creation syntax and is the moved to the allocated memory region. To minimize performance impact, Rust compiles relies on the LLVM framework to perform Return Value optimization. This should initialize the object directly in the destination, instead of copying it. In a nutshell, we could implement something like this:

    /// Allocates memory in the given allocator then places `x` into it.
    ///
    /// This doesn't actually allocate if `T` is zero-sized.
    ///
    /// # Examples
    ///
    /// ```
    /// #![feature(allocator_api)]
    ///
    /// use std::alloc::System;
    ///
    /// let five = Box::new_in(5, System);
    /// ```
    #[cfg(not(no_global_oom_handling))]
    #[unstable(feature = "allocator_api", issue = "32838")]
    #[must_use]
    #[inline]
    pub fn new_in(x: T, alloc: A) -> Self
    where
        A: Allocator,
    {
        let mut boxed = Self::new_uninit_in(alloc);
        unsafe {
            boxed.as_mut_ptr().write(x);
            boxed.assume_init()
        }
    }

I quickly patched this issue by initializing the attest_state field inside the Rec::init. Nevertheless, I still think that we should discuss how to rework the Recs, and Rds initialization as this defeats the point of using Rust and really starts looking like C or C++.

The attestation irq exit test

The attestation_rec_exit_irq attempts to test whether reading the attestation token from rmm can be interrupted by an IRQ. Since, the IRQ is a lower level exception, it cannot interrupt the control flow of Islet. As a result, it needs to be actively checked during RSI handling. For example, the TF-rmm does this as follows:

while (attest_data->token_sign_ctx.state == ATTEST_SIGN_IN_PROGRESS) {
	attest_token_continue_sign_state(attest_data, res);
	if (check_pending_irq()) {
		res->action = UPDATE_REC_EXIT_TO_HOST;
		rec_exit->exit_reason = RMI_EXIT_IRQ;
		return;
	}
}

In Islet this is currently impossible to recreate.

Issue 1

pub fn get_token(challenge: &[u8], measurements: &[Measurement], hash_algo: u8) -> Vec<u8> {
    // TODO: consider storing attestation object somewhere,
    // as RAK and token do not change during rmm lifetime.
    Attestation::new(&plat_token(), &realm_attest_key()).create_attestation_token(
        challenge,
        measurements,
        hash_algo,
    )
}

... and ..

fn get_token_part(
    rd: &Rd,
    context: &mut Rec<'_>,
    size: usize,
) -> core::result::Result<(Vec<u8>, usize), Error> {
    let hash_algo = rd.hash_algo();
    let measurements = rd.measurements;

    // FIXME: This should be stored instead of generating it for every call.
    let token =
        crate::rsi::attestation::get_token(context.attest_challenge(), &measurements, hash_algo);

To make this test work as ARM has intended, we should fix these two TODOs, as these issues are a performance nightmare. When the creation of the realm token will be interruptible by IRQs we cannot regenerate the token every time the Realm asks for it.

Issue 2

    fn sign(secret_key: p384::SecretKey, data: &[u8]) -> Vec<u8> {
        let signing_key = p384::ecdsa::SigningKey::from_bytes(&secret_key.to_bytes())
            .expect("Failed to generate signing key");

        let signature: p384::ecdsa::Signature = signing_key
            .try_sign(data)
            .expect("Failed to create P384 signature");
        signature.to_vec()
    }

This code is responsible for signing the attestation token. We must rewrite it to a "init, update, finish" type API to implement an IRQ checking loop similar to how TF-rmm does it.

DUCT TAPE solution

To make this test working, I applied the following makeshift:

        {
            let (token_part, token_left) = get_token_part(&rd, rec, buffer_size)?;

            if is_irq_pending() {
                error!("IRQ is pending while fetching token");
                set_reg(rec, 0, INCOMPLETE)?;
                set_reg(rec, 1, 0)?;
                ret[0] = rmi::SUCCESS_REC_ENTER;
                return Ok(());
            }

            unsafe {
                let pa_ptr = attest_pa as *mut u8;
                core::ptr::copy(token_part.as_ptr(), pa_ptr.add(pa_offset), token_part.len());
            }
            if token_left == 0 {
                set_reg(rec, 0, SUCCESS)?;
                rec.set_attest_state(RmmRecAttestState::NoAttestInProgress);
            } else {
                set_reg(rec, 0, INCOMPLETE)?;
            }
            set_reg(rec, 1, token_part.len())?;
        }

The helper function:

pub fn is_irq_pending() -> bool {
    let val: u64;

    unsafe {
        core::arch::asm!(
            "mrs {}, ISR_EL1",
            out(reg) val
        )
    }

    return val != 0;
}

Thanks to this, the test passes, but for some reason when I run the entire test suite it fails. Besides that, this solution is a performance nightmare as it generated the token and then checks for IRQ and discards the token if an IRQ is pending.