Re: New EFI thunk alignment WARN_ON in 5.6 triggers multiple times

[Ard Biesheuvel <ardb@kernel.org>]

On Thu, 13 Feb 2020 at 08:36, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/12/20 10:08 PM, Ard Biesheuvel wrote:
> > On Wed, 12 Feb 2020 at 16:00, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 2/12/20 12:53 PM, Ard Biesheuvel wrote:
> >>> On Wed, 12 Feb 2020 at 12:44, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> Hi Ard,
> >>>>
> >>>> While booting 5.6-rc1 on one of my test machines I noticed the WARN_ON
> >>>> on line 198 of arch/x86/platform/efi/efi_64.c trigger many times.
> >>>>
> >>>> I've done some debugging on this an this is caused by the following
> >>>> call path:
> >>>>
> >>>> drivers/firmware/efi/vars.c: efivar_init():
> >>>>
> >>>>            unsigned long variable_name_size = 1024;
> >>>>            efi_char16_t *variable_name;
> >>>>            efi_guid_t vendor_guid;
> >>>>
> >>>>            variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> >>>>            if (!variable_name) {
> >>>>                    printk(KERN_ERR "efivars: Memory allocation failed.\n");
> >>>>                    return -ENOMEM;
> >>>>            }
> >>>>
> >>>>           ...
> >>>>
> >>>>
> >>>>            do {
> >>>>                    variable_name_size = 1024;
> >>>>
> >>>>                    status = ops->get_next_variable(&variable_name_size,
> >>>>                                                    variable_name,
> >>>>                                                    &vendor_guid);
> >>>>           ...
> >>>>
> >>>> arch/x86/platform/efi/efi_64.c: efi_thunk_get_next_variable()
> >>>>
> >>>>           ...
> >>>>            phys_vendor = virt_to_phys_or_null(vendor);
> >>>>           ...
> >>>>
> >>>> arch/x86/platform/efi/efi_64.c: virt_to_phys_or_null_size()
> >>>>
> >>>>           ...
> >>>>           WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> >>>>           ...
> >>>>
> >>>> Specifically the problem is that the efi_guid_t vendor_guid has an 8 bytes
> >>>> aligned address and the WARN_ON checks for it being aligned to\
> >>>> sizeof(efi_guid_t) which is 16 bytes.
> >>>>
> >>>> I've fixed this for now with the following local fix, but I'm not sure
> >>>> what the alignment rules actually are so I'm not sure this is correct:
> >>>>
> >>>> --- a/arch/x86/platform/efi/efi_64.c
> >>>> +++ b/arch/x86/platform/efi/efi_64.c
> >>>> @@ -181,6 +181,7 @@ static inline phys_addr_t
> >>>>     virt_to_phys_or_null_size(void *va, unsigned long size)
> >>>>     {
> >>>>           bool bad_size;
> >>>> +       int alignment;
> >>>>
> >>>>           if (!va)
> >>>>                   return 0;
> >>>> @@ -195,7 +196,8 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
> >>>>            */
> >>>>           bad_size = size > PAGE_SIZE || !is_power_of_2(size);
> >>>>
> >>>> -       WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> >>>> +       alignment = size > 8 ? 8 : size;
> >>>> +       WARN_ON(!IS_ALIGNED((unsigned long)va, alignment) || bad_size);
> >>>>
> >>>>           return slow_virt_to_phys(va);
> >>>>     }
> >>>>
> >>>>
> >>>> I have a feeling that this is the right thing to do, but as said I'm not 100%
> >>>> sure. If you can confirm that this is the right fix, then I can submit this
> >>>> upstream.
> >>>>
> >>>
> >>>
> >>> It seems that the purpose of the alignment check is to ensure that the
> >>> data does not cross a page boundary, so that the data is guaranteed to
> >>> be contiguous in the physical address space as well.
> >>>
> >>> So in that light, the fix is most definitely wrong, although I am not
> >>> sure how this is supposed to work generally.
> >>
> >> I'm not sure that is what it is trying to check, if that is what it is
> >> trying to check then the code is certainly wrong.
> >>
> >> Let me first quote the entire check:
> >>
> >>           /*
> >>            * A fully aligned variable on the stack is guaranteed not to
> >>            * cross a page bounary. Try to catch strings on the stack by
> >>            * checking that 'size' is a power of two.
> >>            */
> >>           bad_size = size > PAGE_SIZE || !is_power_of_2(size);
> >>
> >>           WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> >>
> >> AFAIK EFI is using the identity map, and the kernel stack is
> >> physically contiguous, so crossing a page boundary should not a
> >> problem.
> >
> > CONFIG_HAVE_ARCH_VMAP_STACK=y means the kernel stack may not be
> > contiguous in physical memory, which is why this was added in the
> > first place.
> >
> > We do allocate a special stack for mixed mode, but we only switch to
> > it in the .S thunking code, so at this point, we are still running
> > from the vmap'ed stack
> >
> >> Also notice how the bad_size thing is talking about
> >> page boundary crossing, but the thing triggering is the
> >> IS_ALIGNED check. AFAIK there is no requirement for a struct, e.g.
> >> an UUID (which is the problem here) to be aligned to its size,
> >> it just needs to be 8 byte / 64 bit aligned, which it is yet
> >> the IS_ALIGNED check is failing because it is checking for
> >> a larger, in this case twice as large, but possibly it will
> >> end up checking for a much larger alignment.
> >>
> >
> > The idea is that a data structure of size < PAGE_SIZE is guaranteed
> > not to cross a page boundary if it is aligned to a power-of-2 upper
> > bound of its size. This has nothing to do with the data type or the
> > minimal alignment of any of its constituent parts.
>
> Ok, so I guess that the correct fix is to switch to kmalloc-ing
> "efi_guid_t vendor_guid" in efivar_init() instead of declaring it on
> the stack?
>

I'd prefer it if we updated the efi_thunk_* routines to use a
efi_guid_t on its stack that is naturally aligned, along the lines of

--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -658,6 +658,7 @@ static efi_status_t
 efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
                       u32 *attr, unsigned long *data_size, void *data)
 {
+       efi_guid_t guid __aligned(sizeof(efi_guid_t)) = *vendor;
        efi_status_t status;
        u32 phys_name, phys_vendor, phys_attr;
        u32 phys_data_size, phys_data;
@@ -666,7 +667,7 @@ efi_thunk_get_variable(efi_char16_t *name,
efi_guid_t *vendor,
        spin_lock_irqsave(&efi_runtime_lock, flags);

        phys_data_size = virt_to_phys_or_null(data_size);
-       phys_vendor = virt_to_phys_or_null(vendor);
+       phys_vendor = virt_to_phys_or_null(&guid);
        phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
        phys_attr = virt_to_phys_or_null(attr);
        phys_data = virt_to_phys_or_null_size(data, *data_size);


As for the crashes with >4 GB memory size: I suppose we should just
cap the memory to 4GB when mixed mode is detected, although I think
this is mostly a theoretical case.