rust-osdev/uefi-rs

`ScopedProtocol::drop` can panic when used with `OpenProtocolAttributes::GetProtocol`

Opened this issue · 0 comments

In ScopedProtocol::drop, we currently panic if close_protocol returns something other than Status::SUCCESS.

I've observed that on some firmware, this can cause a problem if the same protocol is opened twice in non-exclusive mode via OpenProtocolAttributes::GetProtocol. In general you shouldn't open the same protocol twice, but with a read-only protocol like DevicePath it should be fine to do so. However, this can lead to the following situation:

fn do_something(h1: Handle, h2: Handle2) {
    {
        let dp1 = boot::open_protocol::<DevicePath>(OpenProtocolParams { handle: h1, agent: boot::image_handle(), controller: None }).unwrap();
        let dp2 = boot::open_protocol::<DevicePath>(OpenProtocolParams { handle: h2, agent: boot::image_handle(), controller: None }).unwrap();

        // Do stuff with dp1/dp2...
    }
    // Now dp1 and dp2 are dropped.
    // If dp1 happens to be the same as dp2, the first drop will succeed,
    // but the second will fail (at least on some firmware), with `Status::NOT_FOUND`.
}

I ran into this with real code, where I didn't expect dp1 == dp2. Fortunately I noticed the issue in testing; it would have been bad if the code started panicking in production.

Although my code is fixed, I don't think we should panic here, as it's not a particularly harmful error if close_protocol fails in this case. Some potential changes:

  • Remove the panic, or change it to a verbose log (debug! or trace! perhaps)
  • Keep the panic, but don't call close_protocol if GetProtocol is used to open the protocol. The spec says "The caller is also not required to close the protocol interface with close_protocol" when GET_PROTOCOL is used.
  • Keep the panic, but allow either SUCCESS or NOT_FOUND when GetProtocol is used to open the protocol.