Re: [PATCH v2 2/2] firmware_loader: fix soundness issue in `request_internal`

[Christian Schrefl]

Greetings

On 08.07.24 10:07 PM, Danilo Krummrich wrote:
> `request_internal` must be called with one of the following function
> pointers: request_firmware(), firmware_request_nowarn(),
> firmware_request_platform() or request_firmware_direct().
> 
> The previous `FwFunc` alias did not guarantee this, which is unsound.
> 
> In order to fix this up, implement `FwFunc` as new type with a
> corresponding type invariant.
> 
> Reported-by: Gary Guo <gary@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/20240620143611.7995e0bb@eugeo/
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> ---
> v2:
>   - provide method for each wrapped `FwFunc` (Christian)> ---
>  rust/kernel/firmware.rs | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 106a928a535e..2ba03af9f036 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -7,10 +7,23 @@
>  use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
>  use core::ptr::NonNull;
>  
> -// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> -// `firmware_request_platform`, `bindings::request_firmware_direct`
> -type FwFunc =
> -    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32;
> +/// # Invariants
> +///
> +/// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> +/// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`.
> +struct FwFunc(
> +    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32,
> +);
> +
> +impl FwFunc {
> +    fn request() -> Self {
> +        Self(bindings::request_firmware)
> +    }
> +
> +    fn request_nowarn() -> Self {
> +        Self(bindings::firmware_request_nowarn)
> +    }

I'm not sure if we should have a comment here that describes how the invariant is fulfilled.

In this case its not too bad since the Invariants are described just a few lines above, so:

Reviewed-by: Christian Schrefl <chrisi.schrefl@xxxxxxxxx>


Cheers,
Christian