[PATCH v3] efidisk: pass buffers with higher alignment

[PATCH v3] efidisk: pass buffers with higher alignment

[Stefan Agner]

Some devices report a IoAlign value of 2, however seem to require a
buffer with higher alignment.

The UEFI specification is saying: "IoAlign values of 0 and 1 mean that
the buffer can be placed anywhere in memory. Otherwise, IoAlign must
be a power of 2, and the requirement is that the start address of a
buffer must be evenly divisible by IoAlign with no remainder."

It seems that this got misinterpreted by some vendors assuming IoAlign
is 2^IoAlign. There is also such a hint in an example in earlier
versions of the Driver Writer's Guide:
ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary

However, it is unsafe to just blindly align buffers by 2^IoAlign, as
this would lead to an overflow for systems which use block size
alignment (e.g. 512 bytes, for example U-Boot).

Work around by using an alignment of at least BlockSize (typically 512
bytes) in case alignment is requested. Also make sure that IoAlign is
still respected as per UEFI specification if a higher alignment than
block size is requested.

Note: The problem has only noticed with compressed squashfs. It seems
that ext4 (and presumably other file system drivers) pass buffers with
a higher alignment already.

Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 grub-core/disk/efi/efidisk.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 9e20af70e..c4eb4f4e7 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
   d = disk->data;
   bio = d->block_io;
 
-  /* Set alignment to 1 if 0 specified */
-  io_align = bio->media->io_align ? bio->media->io_align : 1;
+  /*
+   * If IoAlign is > 1, it should represent the required alignment. However,
+   * some UEFI implementation on Intel NUC systems seem to use IoAlign=2 but
+   * require 2^IoAlign.
+   * Make sure to align to at least block size if IO alignment is required.
+   */
+  if (bio->media->io_align > 1)
+    {
+      if (bio->media->io_align < bio->media->block_size)
+        io_align = bio->media->block_size;
+      else
+        io_align = bio->media->io_align;
+    }
+  else
+    io_align = 1;
+
   num_bytes = size << disk->log_sector_size;
 
   if ((grub_addr_t) buf & (io_align - 1))
-- 
2.36.1

Re: [PATCH v3] efidisk: pass buffers with higher alignment

[Stefan Agner]

On 2022-05-18 10:59, Stefan Agner wrote:
> Some devices report a IoAlign value of 2, however seem to require a
> buffer with higher alignment.

After releasing Home Assistant OS 8.0 publicly, some systems still
refuse to boot even with this patch applied. See bug reports collected
in this issue:
https://github.com/home-assistant/operating-system/issues/1912

At least in one instance using block size alignment helped. I am going
to change to block size aligned unconditionally, and see how that
behaves in wider deployment.

--
Stefan

> 
> The UEFI specification is saying: "IoAlign values of 0 and 1 mean that
> the buffer can be placed anywhere in memory. Otherwise, IoAlign must
> be a power of 2, and the requirement is that the start address of a
> buffer must be evenly divisible by IoAlign with no remainder."
> 
> It seems that this got misinterpreted by some vendors assuming IoAlign
> is 2^IoAlign. There is also such a hint in an example in earlier
> versions of the Driver Writer's Guide:
> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary
> 
> However, it is unsafe to just blindly align buffers by 2^IoAlign, as
> this would lead to an overflow for systems which use block size
> alignment (e.g. 512 bytes, for example U-Boot).
> 
> Work around by using an alignment of at least BlockSize (typically 512
> bytes) in case alignment is requested. Also make sure that IoAlign is
> still respected as per UEFI specification if a higher alignment than
> block size is requested.
> 
> Note: The problem has only noticed with compressed squashfs. It seems
> that ext4 (and presumably other file system drivers) pass buffers with
> a higher alignment already.
> 
> Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  grub-core/disk/efi/efidisk.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 9e20af70e..c4eb4f4e7 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk,
> grub_disk_addr_t sector,
>    d = disk->data;
>    bio = d->block_io;
>  
> -  /* Set alignment to 1 if 0 specified */
> -  io_align = bio->media->io_align ? bio->media->io_align : 1;
> +  /*
> +   * If IoAlign is > 1, it should represent the required alignment. However,
> +   * some UEFI implementation on Intel NUC systems seem to use IoAlign=2 but
> +   * require 2^IoAlign.
> +   * Make sure to align to at least block size if IO alignment is required.
> +   */
> +  if (bio->media->io_align > 1)
> +    {
> +      if (bio->media->io_align < bio->media->block_size)
> +        io_align = bio->media->block_size;
> +      else
> +        io_align = bio->media->io_align;
> +    }
> +  else
> +    io_align = 1;
> +
>    num_bytes = size << disk->log_sector_size;
>  
>    if ((grub_addr_t) buf & (io_align - 1))

Re: [PATCH v3] efidisk: pass buffers with higher alignment

[Daniel Kiper]

Hey,

On Thu, May 19, 2022 at 09:36:41AM +0200, Stefan Agner wrote:
> On 2022-05-18 10:59, Stefan Agner wrote:
> > Some devices report a IoAlign value of 2, however seem to require a
> > buffer with higher alignment.
>
> After releasing Home Assistant OS 8.0 publicly, some systems still
> refuse to boot even with this patch applied. See bug reports collected
> in this issue:
> https://github.com/home-assistant/operating-system/issues/1912
>
> At least in one instance using block size alignment helped. I am going
> to change to block size aligned unconditionally, and see how that
> behaves in wider deployment.

Any updates here? I would be more than happy to get this fixed.

Daniel

Re: [PATCH v3] efidisk: pass buffers with higher alignment

[Stefan Agner]

On 2022-05-26 17:27, Daniel Kiper wrote:
> Hey,
> 
> On Thu, May 19, 2022 at 09:36:41AM +0200, Stefan Agner wrote:
>> On 2022-05-18 10:59, Stefan Agner wrote:
>> > Some devices report a IoAlign value of 2, however seem to require a
>> > buffer with higher alignment.
>>
>> After releasing Home Assistant OS 8.0 publicly, some systems still
>> refuse to boot even with this patch applied. See bug reports collected
>> in this issue:
>> https://github.com/home-assistant/operating-system/issues/1912
>>
>> At least in one instance using block size alignment helped. I am going
>> to change to block size aligned unconditionally, and see how that
>> behaves in wider deployment.
> 
> Any updates here? I would be more than happy to get this fixed.

Sorry for the delay.

I've included that patch in HAOS 8.1, and all of the previously reported
boot issues have been resolved. At this point, I am not aware of a
single boot failure caused by GRUB2 (from our analytics, we have
estimated 3k running on native x86-64 machines and about 14k on virtual
x86-64 machines).

I'll post a v4 of the patch soon.

--
Stefan