Code quality issues in the Rust SDK

[kvinwang]

Hi, team. After examining the eventlog parsing code of the Rust version of this library, I think there are several areas that might need attention.

Unchecked input data slicing may lead to panics

As shown below:

        // Parse EFI Spec Id Event structure
        let spec_id_signature = data[index..index + 16].try_into().unwrap();
        index += 16;
        let spec_id_platform_cls = get_u32(data[index..index + 4].to_vec());
        index += 4;
        let spec_id_version_minor = get_u8(data[index..index + 1].to_vec());
        index += 1;
        ...

All of the parsing logic directly slice the input data without a length check. If a user inputs malformed or incomplete data, the parser would panic. "Panicking" is not a valid error handling choice in a Rust library; it should return Err if there is any problem with the input data.

I suggest using a data decoding library rather than hand-rolling the parsing code from scratch. By using a library, you might get an input reader for free to get rid of manually managing the input data cursor. For example, the above code might become:

        // Parse EFI Spec Id Event structure
        let spec_id_signature = <[u8; 16]>::decode(input_reader)?;
        let spec_id_platform_cls = u32::decode(input_reader)?;
        let spec_id_version_minor = u8::decode(input_reader)?;
        ...

I'll send a PR to demonstrate the details of how to use a library to handle it.

.unwrap() might cause panic

.unwrap() should never be used in Rust except in unit tests or examples. The same applies to .expect("the reason") unless it is known that it would never fail. For example:

let hash: [u8; 32] = data.get(..32).context("insufficient data")?.try_into().expect("convert should never fail");

Unsafe enum transmutation could result in Undefined Behavior

There are some uses of unsafe transmute in the code. When using unsafe in Rust, you should be very careful, because unsafe Rust is harder than C/C++ to use correctly.

Take an example from this repo:

impl TdxQuote {
    pub fn parse_tdx_quote(quote: Vec<u8>) -> Result<TdxQuote, anyhow::Error> {
        let tdx_quote_header: TdxQuoteHeader = unsafe {
            transmute::<[u8; 48], TdxQuoteHeader>(
                quote[0..48]
                    .try_into()
                    .expect("slice with incorrect length"),
            )
        };
        ...
   }
}

// where the definition of TdxQuoteHeader is:
#[repr(C)]
#[derive(Clone)]
pub struct TdxQuoteHeader {
    pub version: u16,
    pub ak_type: AttestationKeyType,
    ...
}

#[repr(u16)]
#[derive(Clone, PartialEq, Debug)]
pub enum AttestationKeyType {
    ECDSA_P256 = 2,
    ECDSA_P384 = 3,
}

As you can see, there is at least one enum field ak_type in the transmuted struct. If the input data of the field ak_type is a value not in [2, 3], (for example, if some future platform got a third AttestationKeyType == 4), the code behavior becomes undefined.
The Rust compiler always optimizes the code based on the assumption that the ak_type never gets an invalid value.

See here for the demonstration code.

Additional areas for enhancement

• Prefer using &[u8] rather than Vec<u8> as function parameters.

For example, the following definition will cause unnecessary call-site heap allocation.

pub fn get_u16(data: Vec<u8>) -> u16 { ... }
// Call-site, the to_vec() will allocate a new heap memory.
fn foo() { let algo_id = get_u16(data[index..index + 2].to_vec()); }

Better:

pub fn get_u16(data: &[u8]) -> u16 { ... }
// Call-site:
fn foo() { let algo_id = get_u16(&data[index..index + 2]); } // Without extra heap allocation.

• More agnostic error handling

       match self.parse() {
            Ok(_) => (),
            Err(e) => {
                return Err(anyhow!("[select] error in parse function {:?}", e));
            }
        }

A shorter equivalent could be:

       _ = self.parse().context("[select] error in parse function")?;

And also return Err(anyhow!("...")); could be shortened to anyhow::bail!("...");.

• Prefer match on enum vars over if else
  Using match, the compiler forces you to update your logic when you add variants to the enum.
• Some other minor style issues may be found using cargo clippy.

[dongx1x]

Hi @kvinwang , thanks a lot for the comments and the demonstration, these are very helpful in improving the quality! We will update the code later.