Re: [PATCH v4] Avoid crash calling PrintErrMesg from efi_multiboot2

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Nov 14, 2024 at 3:04 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 14.11.2024 15:02, Frediano Ziglio wrote:
> > On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 19.08.2024 16:29, Frediano Ziglio wrote:
> >>> --- a/xen/common/efi/boot.c
> >>> +++ b/xen/common/efi/boot.c
> >>> @@ -287,19 +287,36 @@ static bool __init match_guid(const EFI_GUID 
> >>> *guid1, const EFI_GUID *guid2)
> >>>  /* generic routine for printing error messages */
> >>>  static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
> >>>  {
> >>> -    static const CHAR16* const ErrCodeToStr[] __initconstrel = {
> >>> -        [~EFI_ERROR_MASK & EFI_NOT_FOUND]           = L"Not found",
> >>> -        [~EFI_ERROR_MASK & EFI_NO_MEDIA]            = L"The device has 
> >>> no media",
> >>> -        [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED]       = L"Media changed",
> >>> -        [~EFI_ERROR_MASK & EFI_DEVICE_ERROR]        = L"Device error",
> >>> -        [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED]    = L"Volume 
> >>> corrupted",
> >>> -        [~EFI_ERROR_MASK & EFI_ACCESS_DENIED]       = L"Access denied",
> >>> -        [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES]    = L"Out of 
> >>> resources",
> >>> -        [~EFI_ERROR_MASK & EFI_VOLUME_FULL]         = L"Volume is full",
> >>> -        [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION]  = L"Security 
> >>> violation",
> >>> -        [~EFI_ERROR_MASK & EFI_CRC_ERROR]           = L"CRC error",
> >>> -        [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA]    = L"Compromised 
> >>> data",
> >>> -        [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL]    = L"Buffer too 
> >>> small",
> >>> +#define ERROR_MESSAGE_LIST \
> >>> +    ERROR_MESSAGE(EFI_NOT_FOUND, "Not found") \
> >>> +    ERROR_MESSAGE(EFI_NO_MEDIA, "The device has no media") \
> >>> +    ERROR_MESSAGE(EFI_MEDIA_CHANGED, "Media changed") \
> >>> +    ERROR_MESSAGE(EFI_DEVICE_ERROR, "Device error") \
> >>> +    ERROR_MESSAGE(EFI_VOLUME_CORRUPTED, "Volume corrupted") \
> >>> +    ERROR_MESSAGE(EFI_ACCESS_DENIED, "Access denied") \
> >>> +    ERROR_MESSAGE(EFI_OUT_OF_RESOURCES, "Out of resources") \
> >>> +    ERROR_MESSAGE(EFI_VOLUME_FULL, "Volume is full") \
> >>> +    ERROR_MESSAGE(EFI_SECURITY_VIOLATION, "Security violation") \
> >>> +    ERROR_MESSAGE(EFI_CRC_ERROR, "CRC error") \
> >>> +    ERROR_MESSAGE(EFI_COMPROMISED_DATA, "Compromised data") \
> >>> +    ERROR_MESSAGE(EFI_BUFFER_TOO_SMALL, "Buffer too small")
> >>> +
> >>> +    static const struct ErrorStrings {
> >>> +        CHAR16 start;
> >>> +#undef ERROR_MESSAGE
> >>> +#define ERROR_MESSAGE(code, str) CHAR16 msg_ ## code[sizeof(str)];
> >>> +        ERROR_MESSAGE_LIST
> >>> +    } ErrorStrings __initconst = {
> >>> +        0
> >>> +#undef ERROR_MESSAGE
> >>> +#define ERROR_MESSAGE(code, str) , L ## str
> >>> +        ERROR_MESSAGE_LIST
> >>> +    };
> >>> +    static const uint16_t ErrCodeToStr[] __initconst = {
> >>> +#undef ERROR_MESSAGE
> >>> +#define ERROR_MESSAGE(code, str) \
> >>> +        [~EFI_ERROR_MASK & code] = offsetof(struct ErrorStrings, msg_ ## 
> >>> code),
> >>> +        ERROR_MESSAGE_LIST
> >>>      };
> >>>      EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
> >>>
> >>
> >> A while ago Andrew and I discussed this, and I was apparently wrongly 
> >> expecting
> >> him to come back here, as (iirc; no record of this that I could find in 
> >> the mail
> >> archives, so I'm sorry if my recollection is wrong) he was the one to 
> >> object. We
> >> concluded that it wants at least considering to undo the respective part of
> >> 00d5d5ce23e6, finding a different solution to the Clang issue there.
> >
> > I thought this patch was already applied.
> > I didn't remember any clang issue.
> > As far as I know, this was delayed by an issue that turned out to be 
> > different.
> > So, any reason why not to merge the original patch?
>
> Afaict the alternative would result in tidier code, and hence might indeed be
> preferable. But since the reason I didn't long commit the patch is Andrew
> wanting it to not be committed, it'll need to be him to chime in here. Even
> if only to indicate that I'm misremembering.
>
> Jan

What alternative are you talking about? Was it something discussed elsewhere?

Frediano