Re: [misra] Re: [PATCH v3 1/2] efi: Add a function to check if Secure Boot mode is enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.09.2025 12:36, Andrew Cooper wrote:
> On 05/09/2025 11:05 am, Gerald Elder-Vass wrote:
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index e12fa1a7ec04..e7e3dffa7ddc 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>>                     " last line will be ignored.\r\n");
>>  }
>>  
>> +static void __init init_secure_boot_mode(void)
>> +{
>> +    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
>> +    EFI_STATUS status;
>> +    uint8_t data = 0;
>> +    UINTN size = sizeof(data);
>> +    UINT32 attr = 0;
>> +
>> +    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
>> +                                 &size, &data);
> 
> This turns out to be a MISRA R7.4 violation, complaining about casing a
> string literal to a non-const pointer.
> 
> The real problem here is that the EFI spec.  GetVariable() ought to take
> a const CHAR16 *, but doesn't.
> 
> We could fix this with:
> 
> diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
> index a616d1238aa4..56775d553109 100644
> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -224,7 +224,7 @@ VOID
>  typedef
>  EFI_STATUS
>  (EFIAPI *EFI_GET_VARIABLE) (
> -    IN CHAR16                       *VariableName,
> +    IN const CHAR16                 *VariableName,
>      IN EFI_GUID                     *VendorGuid,
>      OUT UINT32                      *Attributes OPTIONAL,
>      IN OUT UINTN                    *DataSize,
> 
> but I fear this might get some objections.

The interface lacking the const in principle means that we can't rely on
there being implementations which actually do fiddle with the string.
Hence ...

> I don't think we want to be deviating every use of GetVariable() for a
> problem ultimately outside of our control.
> 
> Another option would be to have a wrapper for GetVariable() which does
> the cast once, which lets us deviate in one place only.

... this doesn't look like a viable route to me. (Nor a scalable one,
as down the road we then may need more such wrappers.)

> Thoughts?

Why not instead use

    static CHAR16 __initdata str_SecureBoot[] = L"SecureBoot";

and be done?

Jan