On Thu, 13 Feb 2020 at 08:36, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 2/12/20 10:08 PM, Ard Biesheuvel wrote:
> > On Wed, 12 Feb 2020 at 16:00, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 2/12/20 12:53 PM, Ard Biesheuvel wrote:
> >>> On Wed, 12 Feb 2020 at 12:44, Hans de Goede <hdegoede@xxxxxxxxxx> 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.
