Should I trust or check pointers from firmware?

[wmarais]

Hi

I'm writing an UEFI application for learning and fun. What has me a little concerned is that the EFI entry point provide the SystemTable that include many more pointers as shown below.

typedef struct {
  EFI_TABLE_HEADER                 Hdr;
  CHAR16                           *FirmwareVendor;
  UINT32                           FirmwareRevision;
  EFI_HANDLE                       ConsoleInHandle;
  EFI_SIMPLE_TEXT_INPUT_PROTOCOL   *ConIn;
  EFI_HANDLE                       ConsoleOutHandle;
  EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL  *ConOut;
  EFI_HANDLE                       StandardErrorHandle;
  EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL  *StdErr;
  EFI_RUNTIME_SERVICES             *RuntimeServices;
  EFI_BOOT_SERVICES                *BootServices;
  UINTN                            NumberOfTableEntries;
  EFI_CONFIGURATION_TABLE          *ConfigurationTable;
} EFI_SYSTEM_TABLE;

Reading the specification, it makes the statement for some pointers such as ConIn:

If there is no active console, these protocols must still be present.

However many do not have such statements and I can't find anything else that explicitly offers guarantees. I have noticed that rust state to use Option<T> when the pointer can be null but since I have no control over the structure, I do not want to modify it in manner that would break the layout. (I assume that since Option<T> is an enum, wrapping the pointers in Option<T> will change the layout of the structure?)

My understanding is that rust do not provide a mechanism for checking these embedded raw pointers? To that effect, I'm transmuting the pointers and checking if they are null before calling them. I'm not sure if this is right or if I have missed something obvious. The code below shows and example of what I'm doing.

use core::mem::transmute;

use crate::uefi::protocols::InputKey;
use crate::uefi::{Boolean, Event, Status, EFI_ABORTED};

macro_rules! ptr_is_valid {
    ($ptr: expr, $type: ty) => {
        if transmute::<$type, usize>($ptr) == 0 { false } else { true }
    };
}

macro_rules! self_to_mut_ptr {
    ($self: expr) => {
        core::ptr::addr_of!(*$self) as *mut Self
    };
}

#[repr(C)]
pub struct SimpleTextIntputProtocol {
    reset: InputReset,
    read_key_stroke: InputReadKey,
    wait_for_key: Event,
}

impl SimpleTextIntputProtocol {
    pub fn reset(&mut self, extended_verification: Boolean) -> Status {
        unsafe {
            if ptr_is_valid!(self.reset, InputReset) {
                (self.reset)(self_to_mut_ptr!(self), extended_verification)
            } else {
                panic!("Null-pointer exception!")
            }
        }
    }
}

type InputReset =
    unsafe extern "C" fn(this: *mut SimpleTextIntputProtocol, extended_verification: Boolean) -> Status;

pub type InputReadKey =
    unsafe extern "C" fn(this: *mut SimpleTextIntputProtocol, key: *mut InputKey) -> Status;

pub type InputResetEx =
    unsafe extern "C" fn(this: *mut SimpleTextIntputProtocol, extended_verification: Boolean) -> Status;

From my engineering background, I've learned the hard way that when you design electronics, the hardware should not trust the software / firmware and vice versa. Noting rust's focus on safety, it feels like I have missed something here and doing something wrong.

Any advice or obvious mistakes?

[kpreid]

@wmarais

However many do not have such statements and I can't find anything else that explicitly offers guarantees.

It is an unfortunate fact of the world that many C APIs are not documented well enough to confidently write code against them that avoids UB in all cases (or in other words, well enough to write a sound Rust wrapper). But certainly if the documentation sometimes offers a clearly stated not-null guarantee, you can cautiously and reasonably conclude that you should check for null in all the other cases.

@wmarais

I have noticed that rust state to use Option<T> when the pointer can be null but since I have no control over the structure, I do not want to modify it in manner that would break the layout. (I assume that since Option<T> is an enum, wrapping the pointers in Option<T> will change the layout of the structure?)

No, it is guaranteed that when you put most non-nullable pointer types (such as any fn pointer type) in an Option, the representation of the result is, in layout and ABI, a nullable pointer. Consult the documentation for which types this applies to.

@wmarais

To that effect, I'm transmuting the pointers and checking if they are null before calling them.

        if transmute::<$type, usize>($ptr) == 0 { false } else { true }

This code is unsound. fn pointers are intrinsically non-nullable, so the time you get a fn pointer, your program has already entered undefined behavior; you must use Option<InputReset> instead of accepting a null pointer with type InputReset.

(And the likely outcome of this undefined behavior is that the compiler deletes this == 0 test entirely because it is unreachable, so you wouldn't even have achieved your sanity check.)

@wmarais

macro_rules! self_to_mut_ptr {
    ($self: expr) => {
        core::ptr::addr_of!(*$self) as *mut Self
    };
}

The code in this can be written more concisely and idiomatically as any one of:

• ptr::from_mut(self) (most strictly typed option)
• self as *mut Self (reference to pointer cast)
• &raw mut *self (if you like the dereference-then-reference style)

I would recommend you pick whichever one of these you like, and delete both of the macros. Macros should not be used when functions will do, and in this case you don't even need a function.

[danjl1100]

@kpreid

it is guaranteed that when you put most non-nullable pointer types (such as any fn pointer type) in an Option, the representation of the result is, in layout and ABI, a nullable pointer. Consult the documentation for which types this applies to.

It seems like Option<NonNull<T>> may be something to look into? Though I'm not sure what gotchas there are if any pointers are dangling...(?)

NonNull in std::ptr - Rust

[kpreid]

@danjl1100

It seems like Option<NonNull<T>> may be something to look into?

You can freely use Option<NonNull<T>> as a substitute for *mut T or *const T. Unlike the Option<fn()> case, this is not necessary to avoid UB but merely a convenience that replaces null checks you have to do manually with Option matches the type system requires of you.

In your sample code you're trying to accept possibly-null function pointers, which are already a non-nullable pointer type in Rust, so NonNull doesn't help you and you only need to use Option<fn()>. But if you have any nullable data pointers, NonNull may be a good thing to use there — in the end, using NonNull is up to whether it helps your code be correct and visibly so.

@danjl1100

Though I'm not sure what gotchas there are if any pointers are dangling...(?)

NonNull doesn’t care if the pointer is dangling; in fact, there's even a helper function for it. In general, all the unsafe-to-dereference pointer types can be dangling.

I think so can the safe-to-dereference types (it's just extremely dangerous to have such a value around) but that’s a more under-documented and under-defined area.

[CAD97]

@kpreid

You can freely use Option<NonNull<T>> as a substitute for *mut T or *const T.

Caveat: variance. It's usually a good idea to use PhantomData to achieve "by example" variance alongside NonNull.

@kpreid

I think so can the safe-to-dereference types [dangle] (it's just extremely dangerous to have such a value around)

(I am a member of t-opsem but this should not be construed to represent the team position)

For reference types this is a dangerous enough simplification that it might as well be wrong.

rambling on said details

Technically, you can safely have a reference value which dangles thanks to non-lexical lifetimes, you're just prevented from using the value in any manner. This is okay, even using unsafe that isn't apparent to borrowck, because Rust doesn't have typed memory, even on the stack. Values only ever matter once you use them, but anything more than a pure place mention can result in using the value for this purpose. (And you can't syntactically tell a pure place mention from an impure one.)

So the validity of the quoted statement depends entirely on how you understand “having such a value around.” And also relies on borrow model opsem specifics which are not formally accepted as being the case stably and forever, as well as what things are allowed to be done from safe code. Negative reasoning (i.e. “this scenario is okay because that problem can't happen”) is always dangerous (new functionality could be added that breaks it) and it's much more ideal to use positive reasoning to justify validity. (Soundness reasoning may fundamentally have some negative reasoning to it, since it's a proof of absence, but it's the difference between arguing that UB cannot be reached (negative) and showing that any reachable state is defined (positive).)

For safe collection types built on pointers^[1], it's technically true, but it's not possible to create a dangling value, by construction (at least not without causing “Library UB” and thus stretching “technically allowed” to its breaking point).

---

1. Box is special, but there's an open RFC with some T-opsem backing (but not full consensus yet) to remove the current opsem magic from Box. (And collections can be built out of Box instead of out of NonNull, giving them whatever Box magic exists.) ↩︎

[wmarais]

Thanks @kpreid, @danjl1100 and @CAD97. Sorry for the slow reply, I got distracted reading up on the things you discussed. For anyone looking at the topic in future, I have incorporated the modification in the code below.

use crate::uefi::protocols::InputKey;
use crate::uefi::{Boolean, Event, Status, EFI_DEVICE_ERROR};

#[repr(C)]
pub struct SimpleTextIntputProtocol {
    reset: Option<InputReset>,
    read_key_stroke: Option<InputReadKey>,
    wait_for_key: Option<Event>,
}

impl SimpleTextIntputProtocol {
    pub fn new(
        reset: Option<InputReset>,
        read_key_stroke: Option<InputReadKey>,
        wait_for_key: Option<Event>,
    ) -> Self {
        Self {
            reset,
            read_key_stroke,
            wait_for_key,
        }
    }

    pub fn reset(&self, extended_verification: Boolean) -> Status {
        match self.reset {
            Some(reset) => unsafe { reset(self as *const Self, extended_verification) },
            None => EFI_DEVICE_ERROR,
        }
    }

    pub fn read_key_stroke(&self, key: *mut InputKey) -> Status {
        match self.read_key_stroke {
            Some(read_keystroke) => unsafe { read_keystroke(self as *const Self, key) },
            None => EFI_DEVICE_ERROR,
        }
    }

    pub fn set_wait_for_key(&mut self, event: Event) {
        self.wait_for_key = Some(event)
    }
}

type InputReset = unsafe extern "C" fn(
    this: *const SimpleTextIntputProtocol,
    extended_verification: Boolean,
) -> Status;

pub type InputReadKey =
    unsafe extern "C" fn(this: *const SimpleTextIntputProtocol, key: *mut InputKey) -> Status;

#[cfg(test)]
mod unit_tests {
    use super::*;

    #[test]
    fn null_pointer_safety() {
        let stip = SimpleTextIntputProtocol::new(None, None, None);

        let mut input_key = InputKey::new(0, 0);

        assert_eq!(stip.reset(Boolean::Flase), EFI_DEVICE_ERROR);
        assert_eq!(
            stip.read_key_stroke(core::ptr::from_mut(&mut input_key)),
            EFI_DEVICE_ERROR
        );
    }
}