linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-efi <linux-efi@vger.kernel.org>
Subject: Re: New EFI thunk alignment WARN_ON in 5.6 triggers multiple times
Date: Wed, 12 Feb 2020 11:53:06 +0000	[thread overview]
Message-ID: <CAKv+Gu9fh_reYp65QXCeL4X3iovrCQFGNZWS3jMkpveZLgre-A@mail.gmail.com> (raw)
In-Reply-To: <aaf658ab-82ff-4a35-2984-77150924d3a7@redhat.com>

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.

  reply	other threads:[~2020-02-12 11:53 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-12 11:44 New EFI thunk alignment WARN_ON in 5.6 triggers multiple times Hans de Goede
2020-02-12 11:53 ` Ard Biesheuvel [this message]
2020-02-12 15:00   ` Hans de Goede
2020-02-12 21:08     ` Ard Biesheuvel
2020-02-13  7:36       ` Hans de Goede
2020-02-13  8:03         ` Ard Biesheuvel
2020-02-13  8:55           ` Ard Biesheuvel
2020-02-13 10:03             ` Hans de Goede
2020-02-13 10:18               ` Ard Biesheuvel
2020-02-12 21:57     ` Arvind Sankar
2020-02-13  0:34       ` Arvind Sankar
2020-02-13  7:34         ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu9fh_reYp65QXCeL4X3iovrCQFGNZWS3jMkpveZLgre-A@mail.gmail.com \
    --to=ardb@kernel.org \
    --cc=hdegoede@redhat.com \
    --cc=linux-efi@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).