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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/09/2025 11:44 am, Jan Beulich wrote:
> 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.

Well, the IN and absence of OUT does mean this in practice.

> 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?

I suppose, but that's still awkward to use.

~Andrew