Hi,
On 2/13/20 9:55 AM, Ard Biesheuvel wrote:
On Thu, 13 Feb 2020 at 09:03, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
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.
Hmm, actually, I don't think we can rely on alignment == 16, given
that the compiler assumes that RSP is 16 byte aligned already, which
is not the case in reality.
So we'll probably have to add some compiler foo as well.
It really seems easier to me to just kmalloc everything which is
bigger then 8 bytes? I see you are modifying the efi_thunk_get_variable
function, my idea was to do the kmalloc in efivar_init(), that already
kmalloc's the variable_name, so doing the same for the vendor guid inside
efi_init() seems like an ok solution to me?
Regards,
Hans
