[PATCH] efidisk: Respect block_io_protocol buffer alignment

[PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

Returned from the OpenProtocol operation, the grub_efi_block_io_media
structure contains the io_align field, specifying the minimum alignment
required for buffers used in any data transfers with the device.

Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
this boundary, if the buffer passed to it does not already meet the
requirements.

Reported-by: Jeremy Linton <jeremy.linton@arm.com>
---

This modified version is contained entirely within the efidisk driver.
There is some excessive copying going on, but it removes the risk of
the changes interfering with other disk drivers.

 grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 1c00e3e..901133f 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
 {
   struct grub_efidisk_data *d;
   grub_efi_block_io_t *bio;
+  grub_efi_status_t status;
+  grub_size_t io_align, num_bytes;
+  char *aligned_buf;
 
   d = disk->data;
   bio = d->block_io;
 
-  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
-		     bio->media->media_id,
-		     (grub_efi_uint64_t) sector,
-		     (grub_efi_uintn_t) size << disk->log_sector_size,
-		     buf);
+  /* Set alignment to 1 if 0 specified */
+  io_align = bio->media->io_align ? bio->media->io_align : 1;
+  num_bytes = size << disk->log_sector_size;
+
+  if ((unsigned long) buf & (io_align - 1))
+    {
+      aligned_buf = grub_memalign (io_align, num_bytes);
+      if (! aligned_buf)
+	return GRUB_EFI_OUT_OF_RESOURCES;
+      if (wr)
+	grub_memcpy (aligned_buf, buf, num_bytes);
+    }
+  else
+    {
+      aligned_buf = buf;
+    }
+
+  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
+			bio->media->media_id, (grub_efi_uint64_t) sector,
+			(grub_efi_uintn_t) num_bytes, aligned_buf);
+
+  if ((unsigned long) buf & (io_align - 1))
+    {
+      if (!wr)
+	grub_memcpy (buf, aligned_buf, num_bytes);
+      grub_free (aligned_buf);
+    }
+
+  return status;
 }
 
 static grub_err_t
-- 
2.1.4

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Andrei Borzenkov]

17.02.2016 22:44, Leif Lindholm пишет:
> Returned from the OpenProtocol operation, the grub_efi_block_io_media
> structure contains the io_align field, specifying the minimum alignment
> required for buffers used in any data transfers with the device.
> 
> Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> this boundary, if the buffer passed to it does not already meet the
> requirements.
> 
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> 
> This modified version is contained entirely within the efidisk driver.
> There is some excessive copying going on, but it removes the risk of
> the changes interfering with other disk drivers.
> 
>  grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 1c00e3e..901133f 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
>  {
>    struct grub_efidisk_data *d;
>    grub_efi_block_io_t *bio;
> +  grub_efi_status_t status;
> +  grub_size_t io_align, num_bytes;
> +  char *aligned_buf;
>  
>    d = disk->data;
>    bio = d->block_io;
>  
> -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> -		     bio->media->media_id,
> -		     (grub_efi_uint64_t) sector,
> -		     (grub_efi_uintn_t) size << disk->log_sector_size,
> -		     buf);
> +  /* Set alignment to 1 if 0 specified */
> +  io_align = bio->media->io_align ? bio->media->io_align : 1;

We probably should sanity check that it is power of two in
grub_efidisk_open.

> +  num_bytes = size << disk->log_sector_size;
> +
> +  if ((unsigned long) buf & (io_align - 1))

grub_addr_t?

> +    {
> +      aligned_buf = grub_memalign (io_align, num_bytes);
> +      if (! aligned_buf)
> +	return GRUB_EFI_OUT_OF_RESOURCES;
> +      if (wr)
> +	grub_memcpy (aligned_buf, buf, num_bytes);
> +    }
> +  else
> +    {
> +      aligned_buf = buf;
> +    }
> +
> +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> +			bio->media->media_id, (grub_efi_uint64_t) sector,
> +			(grub_efi_uintn_t) num_bytes, aligned_buf);
> +
> +  if ((unsigned long) buf & (io_align - 1))
> +    {
> +      if (!wr)
> +	grub_memcpy (buf, aligned_buf, num_bytes);
> +      grub_free (aligned_buf);
> +    }
> +
> +  return status;
>  }
>  
>  static grub_err_t
> 

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

On Thu, Feb 18, 2016 at 06:36:06AM +0300, Andrei Borzenkov wrote:
> 17.02.2016 22:44, Leif Lindholm пишет:
> > Returned from the OpenProtocol operation, the grub_efi_block_io_media
> > structure contains the io_align field, specifying the minimum alignment
> > required for buffers used in any data transfers with the device.
> > 
> > Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> > this boundary, if the buffer passed to it does not already meet the
> > requirements.
> > 
> > Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> > ---
> > 
> > This modified version is contained entirely within the efidisk driver.
> > There is some excessive copying going on, but it removes the risk of
> > the changes interfering with other disk drivers.
> > 
> >  grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> > index 1c00e3e..901133f 100644
> > --- a/grub-core/disk/efi/efidisk.c
> > +++ b/grub-core/disk/efi/efidisk.c
> > @@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
> >  {
> >    struct grub_efidisk_data *d;
> >    grub_efi_block_io_t *bio;
> > +  grub_efi_status_t status;
> > +  grub_size_t io_align, num_bytes;
> > +  char *aligned_buf;
> >  
> >    d = disk->data;
> >    bio = d->block_io;
> >  
> > -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> > -		     bio->media->media_id,
> > -		     (grub_efi_uint64_t) sector,
> > -		     (grub_efi_uintn_t) size << disk->log_sector_size,
> > -		     buf);
> > +  /* Set alignment to 1 if 0 specified */
> > +  io_align = bio->media->io_align ? bio->media->io_align : 1;
> 
> We probably should sanity check that it is power of two in
> grub_efidisk_open.

The UEFI specification says:
"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."

So we certainly could sanity check it, but what's the fallback if
firmware presents an invalid value? Scream loudly that things are
likely to break and then bail out, or silently round up to the
nearest power of two?

Of the two, the former seems more palatable.

> > +  num_bytes = size << disk->log_sector_size;
> > +
> > +  if ((unsigned long) buf & (io_align - 1))
> 
> grub_addr_t?

Yeah, sorry, got lost in refactoring.

> > +    {
> > +      aligned_buf = grub_memalign (io_align, num_bytes);
> > +      if (! aligned_buf)
> > +	return GRUB_EFI_OUT_OF_RESOURCES;
> > +      if (wr)
> > +	grub_memcpy (aligned_buf, buf, num_bytes);
> > +    }
> > +  else
> > +    {
> > +      aligned_buf = buf;
> > +    }
> > +
> > +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> > +			bio->media->media_id, (grub_efi_uint64_t) sector,
> > +			(grub_efi_uintn_t) num_bytes, aligned_buf);
> > +
> > +  if ((unsigned long) buf & (io_align - 1))
> > +    {
> > +      if (!wr)
> > +	grub_memcpy (buf, aligned_buf, num_bytes);
> > +      grub_free (aligned_buf);
> > +    }
> > +
> > +  return status;
> >  }
> >  
> >  static grub_err_t
> > 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Andrei Borzenkov]

On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 18, 2016 at 06:36:06AM +0300, Andrei Borzenkov wrote:
>> 17.02.2016 22:44, Leif Lindholm пишет:
>> > Returned from the OpenProtocol operation, the grub_efi_block_io_media
>> > structure contains the io_align field, specifying the minimum alignment
>> > required for buffers used in any data transfers with the device.
>> >
>> > Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
>> > this boundary, if the buffer passed to it does not already meet the
>> > requirements.
>> >
>> > Reported-by: Jeremy Linton <jeremy.linton@arm.com>
>> > ---
>> >
>> > This modified version is contained entirely within the efidisk driver.
>> > There is some excessive copying going on, but it removes the risk of
>> > the changes interfering with other disk drivers.
>> >
>> >  grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
>> >  1 file changed, 32 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
>> > index 1c00e3e..901133f 100644
>> > --- a/grub-core/disk/efi/efidisk.c
>> > +++ b/grub-core/disk/efi/efidisk.c
>> > @@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
>> >  {
>> >    struct grub_efidisk_data *d;
>> >    grub_efi_block_io_t *bio;
>> > +  grub_efi_status_t status;
>> > +  grub_size_t io_align, num_bytes;
>> > +  char *aligned_buf;
>> >
>> >    d = disk->data;
>> >    bio = d->block_io;
>> >
>> > -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
>> > -                bio->media->media_id,
>> > -                (grub_efi_uint64_t) sector,
>> > -                (grub_efi_uintn_t) size << disk->log_sector_size,
>> > -                buf);
>> > +  /* Set alignment to 1 if 0 specified */
>> > +  io_align = bio->media->io_align ? bio->media->io_align : 1;
>>
>> We probably should sanity check that it is power of two in
>> grub_efidisk_open.
>
> The UEFI specification says:
> "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."
>
> So we certainly could sanity check it, but what's the fallback if
> firmware presents an invalid value? Scream loudly that things are
> likely to break and then bail out, or silently round up to the
> nearest power of two?
>

grub_efidisk_open() fails if sector size is bad, I guess we should
follow the suite. Please also add alignment print to grub_dprintf
there.

It may be interesting to optionally keep stats how often we hit
non-aligned buffer. I wonder what alignment requirement your system
has - is it sector size?

> Of the two, the former seems more palatable.
>
>> > +  num_bytes = size << disk->log_sector_size;
>> > +
>> > +  if ((unsigned long) buf & (io_align - 1))
>>
>> grub_addr_t?
>
> Yeah, sorry, got lost in refactoring.
>
>> > +    {
>> > +      aligned_buf = grub_memalign (io_align, num_bytes);
>> > +      if (! aligned_buf)
>> > +   return GRUB_EFI_OUT_OF_RESOURCES;
>> > +      if (wr)
>> > +   grub_memcpy (aligned_buf, buf, num_bytes);
>> > +    }
>> > +  else
>> > +    {
>> > +      aligned_buf = buf;
>> > +    }
>> > +
>> > +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
>> > +                   bio->media->media_id, (grub_efi_uint64_t) sector,
>> > +                   (grub_efi_uintn_t) num_bytes, aligned_buf);
>> > +
>> > +  if ((unsigned long) buf & (io_align - 1))

And here too of course.

>> > +    {
>> > +      if (!wr)
>> > +   grub_memcpy (buf, aligned_buf, num_bytes);
>> > +      grub_free (aligned_buf);
>> > +    }
>> > +
>> > +  return status;
>> >  }
>> >
>> >  static grub_err_t
>> >

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

On Thu, Feb 18, 2016 at 03:00:38PM +0300, Andrei Borzenkov wrote:
> On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Feb 18, 2016 at 06:36:06AM +0300, Andrei Borzenkov wrote:
> >> 17.02.2016 22:44, Leif Lindholm пишет:
> >> > Returned from the OpenProtocol operation, the grub_efi_block_io_media
> >> > structure contains the io_align field, specifying the minimum alignment
> >> > required for buffers used in any data transfers with the device.
> >> >
> >> > Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> >> > this boundary, if the buffer passed to it does not already meet the
> >> > requirements.
> >> >
> >> > Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> >> > ---
> >> >
> >> > This modified version is contained entirely within the efidisk driver.
> >> > There is some excessive copying going on, but it removes the risk of
> >> > the changes interfering with other disk drivers.
> >> >
> >> >  grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
> >> >  1 file changed, 32 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> >> > index 1c00e3e..901133f 100644
> >> > --- a/grub-core/disk/efi/efidisk.c
> >> > +++ b/grub-core/disk/efi/efidisk.c
> >> > @@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
> >> >  {
> >> >    struct grub_efidisk_data *d;
> >> >    grub_efi_block_io_t *bio;
> >> > +  grub_efi_status_t status;
> >> > +  grub_size_t io_align, num_bytes;
> >> > +  char *aligned_buf;
> >> >
> >> >    d = disk->data;
> >> >    bio = d->block_io;
> >> >
> >> > -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> >> > -                bio->media->media_id,
> >> > -                (grub_efi_uint64_t) sector,
> >> > -                (grub_efi_uintn_t) size << disk->log_sector_size,
> >> > -                buf);
> >> > +  /* Set alignment to 1 if 0 specified */
> >> > +  io_align = bio->media->io_align ? bio->media->io_align : 1;
> >>
> >> We probably should sanity check that it is power of two in
> >> grub_efidisk_open.
> >
> > The UEFI specification says:
> > "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."
> >
> > So we certainly could sanity check it, but what's the fallback if
> > firmware presents an invalid value? Scream loudly that things are
> > likely to break and then bail out, or silently round up to the
> > nearest power of two?
> 
> grub_efidisk_open() fails if sector size is bad, I guess we should
> follow the suite. Please also add alignment print to grub_dprintf
> there.

Good call, that would be interesting to have.
I'll also add a grub_error() message, given it is a fatal system
error.

> It may be interesting to optionally keep stats how often we hit
> non-aligned buffer. I wonder what alignment requirement your system
> has - is it sector size?

In this instance it was (/happened to coincide with?) page size.

> > Of the two, the former seems more palatable.
> >
> >> > +  num_bytes = size << disk->log_sector_size;
> >> > +
> >> > +  if ((unsigned long) buf & (io_align - 1))
> >>
> >> grub_addr_t?
> >
> > Yeah, sorry, got lost in refactoring.
> >
> >> > +    {
> >> > +      aligned_buf = grub_memalign (io_align, num_bytes);
> >> > +      if (! aligned_buf)
> >> > +   return GRUB_EFI_OUT_OF_RESOURCES;
> >> > +      if (wr)
> >> > +   grub_memcpy (aligned_buf, buf, num_bytes);
> >> > +    }
> >> > +  else
> >> > +    {
> >> > +      aligned_buf = buf;
> >> > +    }
> >> > +
> >> > +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> >> > +                   bio->media->media_id, (grub_efi_uint64_t) sector,
> >> > +                   (grub_efi_uintn_t) num_bytes, aligned_buf);
> >> > +
> >> > +  if ((unsigned long) buf & (io_align - 1))
> 
> And here too of course.

Already in my tree :)

New version coming up.

/
    Leif

[PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

Returned from the OpenProtocol operation, the grub_efi_block_io_media
structure contains the io_align field, specifying the minimum alignment
required for buffers used in any data transfers with the device.

Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
this boundary, if the buffer passed to it does not already meet the
requirements.

Also sanity check the io_align field in grub_efidisk_open() for
power-of-two-ness and bail if invalid.

Reported-by: Jeremy Linton <jeremy.linton@arm.com>
---

v3 - Fixed up types and added a check on the io_align field, as per
     Andrei's comments.

 grub-core/disk/efi/efidisk.c | 53 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 1c00e3e..c1afbe4 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -463,6 +463,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
   int num;
   struct grub_efidisk_data *d = 0;
   grub_efi_block_io_media_t *m;
+  unsigned long i = 0;
 
   grub_dprintf ("efidisk", "opening %s\n", name);
 
@@ -491,10 +492,21 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
 
   disk->id = ((num << GRUB_CHAR_BIT) | name[0]);
   m = d->block_io->media;
+  /* Ensure required buffer alignment is a power of two (or is zero). */
+  do
+    {
+      if ((m->io_align & (1UL << i)) && (m->io_align & ~(1UL << i)))
+	return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d",
+			   m->io_align);
+    }
+  while (++i < (sizeof (grub_addr_t) * 8));
+
   /* FIXME: Probably it is better to store the block size in the disk,
      and total sectors should be replaced with total blocks.  */
-  grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
-		m, (unsigned long long) m->last_block, m->block_size);
+  grub_dprintf ("efidisk",
+		"m = %p, last block = %llx, block size = %x, io align = %x\n",
+		m, (unsigned long long) m->last_block, m->block_size,
+		m->io_align);
   disk->total_sectors = m->last_block + 1;
   /* Don't increase this value due to bug in some EFI.  */
   disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
@@ -524,15 +536,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
 {
   struct grub_efidisk_data *d;
   grub_efi_block_io_t *bio;
+  grub_efi_status_t status;
+  grub_size_t io_align, num_bytes;
+  char *aligned_buf;
 
   d = disk->data;
   bio = d->block_io;
 
-  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
-		     bio->media->media_id,
-		     (grub_efi_uint64_t) sector,
-		     (grub_efi_uintn_t) size << disk->log_sector_size,
-		     buf);
+  /* Set alignment to 1 if 0 specified */
+  io_align = bio->media->io_align ? bio->media->io_align : 1;
+  num_bytes = size << disk->log_sector_size;
+
+  if ((grub_addr_t) buf & (io_align - 1))
+    {
+      aligned_buf = grub_memalign (io_align, num_bytes);
+      if (! aligned_buf)
+	return GRUB_EFI_OUT_OF_RESOURCES;
+      if (wr)
+	grub_memcpy (aligned_buf, buf, num_bytes);
+    }
+  else
+    {
+      aligned_buf = buf;
+    }
+
+  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
+			bio->media->media_id, (grub_efi_uint64_t) sector,
+			(grub_efi_uintn_t) num_bytes, aligned_buf);
+
+  if ((grub_addr_t) buf & (io_align - 1))
+    {
+      if (!wr)
+	grub_memcpy (buf, aligned_buf, num_bytes);
+      grub_free (aligned_buf);
+    }
+
+  return status;
 }
 
 static grub_err_t
-- 
2.1.4

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4212 bytes --]

On 18.02.2016 16:00, Leif Lindholm wrote:
> Returned from the OpenProtocol operation, the grub_efi_block_io_media
> structure contains the io_align field, specifying the minimum alignment
> required for buffers used in any data transfers with the device.
> 
> Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> this boundary, if the buffer passed to it does not already meet the
> requirements.
> 
> Also sanity check the io_align field in grub_efidisk_open() for
> power-of-two-ness and bail if invalid.
> 
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> 
> v3 - Fixed up types and added a check on the io_align field, as per
>      Andrei's comments.
> 
>  grub-core/disk/efi/efidisk.c | 53 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 1c00e3e..c1afbe4 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -463,6 +463,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>    int num;
>    struct grub_efidisk_data *d = 0;
>    grub_efi_block_io_media_t *m;
> +  unsigned long i = 0;
>  
>    grub_dprintf ("efidisk", "opening %s\n", name);
>  
> @@ -491,10 +492,21 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>  
>    disk->id = ((num << GRUB_CHAR_BIT) | name[0]);
>    m = d->block_io->media;
> +  /* Ensure required buffer alignment is a power of two (or is zero). */
> +  do
> +    {
> +      if ((m->io_align & (1UL << i)) && (m->io_align & ~(1UL << i)))
> +	return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d",
> +			   m->io_align);
> +    }
> +  while (++i < (sizeof (grub_addr_t) * 8));
> +
if (m->io_align & (m->io_align - 1)) { ...error handling... }

>    /* FIXME: Probably it is better to store the block size in the disk,
>       and total sectors should be replaced with total blocks.  */
> -  grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
> -		m, (unsigned long long) m->last_block, m->block_size);
> +  grub_dprintf ("efidisk",
> +		"m = %p, last block = %llx, block size = %x, io align = %x\n",
> +		m, (unsigned long long) m->last_block, m->block_size,
> +		m->io_align);
>    disk->total_sectors = m->last_block + 1;
>    /* Don't increase this value due to bug in some EFI.  */
>    disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
> @@ -524,15 +536,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
>  {
>    struct grub_efidisk_data *d;
>    grub_efi_block_io_t *bio;
> +  grub_efi_status_t status;
> +  grub_size_t io_align, num_bytes;
> +  char *aligned_buf;
>  
>    d = disk->data;
>    bio = d->block_io;
>  
> -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> -		     bio->media->media_id,
> -		     (grub_efi_uint64_t) sector,
> -		     (grub_efi_uintn_t) size << disk->log_sector_size,
> -		     buf);
> +  /* Set alignment to 1 if 0 specified */
> +  io_align = bio->media->io_align ? bio->media->io_align : 1;
> +  num_bytes = size << disk->log_sector_size;
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      aligned_buf = grub_memalign (io_align, num_bytes);
> +      if (! aligned_buf)
> +	return GRUB_EFI_OUT_OF_RESOURCES;
Rather than constantly allocating and deallocating perhaps we should
allocate maximum size and then always use the same buffer? See
max_agglomerate. We already limit to at most 640KiB per read on EFI due
to EFI bugs.
> +      if (wr)
> +	grub_memcpy (aligned_buf, buf, num_bytes);
> +    }
> +  else
> +    {
> +      aligned_buf = buf;
> +    }
> +
> +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> +			bio->media->media_id, (grub_efi_uint64_t) sector,
> +			(grub_efi_uintn_t) num_bytes, aligned_buf);
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      if (!wr)
> +	grub_memcpy (buf, aligned_buf, num_bytes);
> +      grub_free (aligned_buf);
> +    }
> +
> +  return status;
>  }
>  
>  static grub_err_t
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

On Thu, Feb 18, 2016 at 04:27:58PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 18.02.2016 16:00, Leif Lindholm wrote:
> > --- a/grub-core/disk/efi/efidisk.c
> > +++ b/grub-core/disk/efi/efidisk.c
> > @@ -463,6 +463,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
> >    int num;
> >    struct grub_efidisk_data *d = 0;
> >    grub_efi_block_io_media_t *m;
> > +  unsigned long i = 0;
> >  
> >    grub_dprintf ("efidisk", "opening %s\n", name);
> >  
> > @@ -491,10 +492,21 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
> >  
> >    disk->id = ((num << GRUB_CHAR_BIT) | name[0]);
> >    m = d->block_io->media;
> > +  /* Ensure required buffer alignment is a power of two (or is zero). */
> > +  do
> > +    {
> > +      if ((m->io_align & (1UL << i)) && (m->io_align & ~(1UL << i)))
> > +	return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d",
> > +			   m->io_align);
> > +    }
> > +  while (++i < (sizeof (grub_addr_t) * 8));
> > +
>
> if (m->io_align & (m->io_align - 1)) { ...error handling... }

Fair enough.

> > +  if ((grub_addr_t) buf & (io_align - 1))
> > +    {
> > +      aligned_buf = grub_memalign (io_align, num_bytes);
> > +      if (! aligned_buf)
> > +	return GRUB_EFI_OUT_OF_RESOURCES;
>
> Rather than constantly allocating and deallocating perhaps we should
> allocate maximum size and then always use the same buffer? See
> max_agglomerate. We already limit to at most 640KiB per read on EFI due
> to EFI bugs.

It'd start getting a bit fiddly, since you may have different
alignment requirements for different devices.
Certainly not impossible, but could we make the code functional first
and consider potential optimisations later?

/
    Leif

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Andrei Borzenkov]

18.02.2016 18:00, Leif Lindholm пишет:
> Returned from the OpenProtocol operation, the grub_efi_block_io_media
> structure contains the io_align field, specifying the minimum alignment
> required for buffers used in any data transfers with the device.
> 
> Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> this boundary, if the buffer passed to it does not already meet the
> requirements.
> 
> Also sanity check the io_align field in grub_efidisk_open() for
> power-of-two-ness and bail if invalid.
> 
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> 
> v3 - Fixed up types and added a check on the io_align field, as per
>      Andrei's comments.
> 
>  grub-core/disk/efi/efidisk.c | 53 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 1c00e3e..c1afbe4 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -463,6 +463,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>    int num;
>    struct grub_efidisk_data *d = 0;
>    grub_efi_block_io_media_t *m;
> +  unsigned long i = 0;
>  
>    grub_dprintf ("efidisk", "opening %s\n", name);
>  
> @@ -491,10 +492,21 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>  
>    disk->id = ((num << GRUB_CHAR_BIT) | name[0]);
>    m = d->block_io->media;
> +  /* Ensure required buffer alignment is a power of two (or is zero). */
> +  do
> +    {
> +      if ((m->io_align & (1UL << i)) && (m->io_align & ~(1UL << i)))
> +	return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d",
> +			   m->io_align);
> +    }

Please move check and error return after grub_dprintf together with
other checks so we can see what values cause failure.

> +  while (++i < (sizeof (grub_addr_t) * 8));
> +
>    /* FIXME: Probably it is better to store the block size in the disk,
>       and total sectors should be replaced with total blocks.  */
> -  grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
> -		m, (unsigned long long) m->last_block, m->block_size);
> +  grub_dprintf ("efidisk",
> +		"m = %p, last block = %llx, block size = %x, io align = %x\n",
> +		m, (unsigned long long) m->last_block, m->block_size,
> +		m->io_align);
>    disk->total_sectors = m->last_block + 1;
>    /* Don't increase this value due to bug in some EFI.  */
>    disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
> @@ -524,15 +536,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
>  {
>    struct grub_efidisk_data *d;
>    grub_efi_block_io_t *bio;
> +  grub_efi_status_t status;
> +  grub_size_t io_align, num_bytes;
> +  char *aligned_buf;
>  
>    d = disk->data;
>    bio = d->block_io;
>  
> -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> -		     bio->media->media_id,
> -		     (grub_efi_uint64_t) sector,
> -		     (grub_efi_uintn_t) size << disk->log_sector_size,
> -		     buf);
> +  /* Set alignment to 1 if 0 specified */
> +  io_align = bio->media->io_align ? bio->media->io_align : 1;
> +  num_bytes = size << disk->log_sector_size;
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      aligned_buf = grub_memalign (io_align, num_bytes);
> +      if (! aligned_buf)
> +	return GRUB_EFI_OUT_OF_RESOURCES;
> +      if (wr)
> +	grub_memcpy (aligned_buf, buf, num_bytes);
> +    }
> +  else
> +    {
> +      aligned_buf = buf;
> +    }
> +
> +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> +			bio->media->media_id, (grub_efi_uint64_t) sector,
> +			(grub_efi_uintn_t) num_bytes, aligned_buf);
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      if (!wr)
> +	grub_memcpy (buf, aligned_buf, num_bytes);
> +      grub_free (aligned_buf);
> +    }
> +
> +  return status;
>  }
>  
>  static grub_err_t
> 

[PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

Returned from the OpenProtocol operation, the grub_efi_block_io_media
structure contains the io_align field, specifying the minimum alignment
required for buffers used in any data transfers with the device.

Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
this boundary, if the buffer passed to it does not already meet the
requirements.

Also sanity check the io_align field in grub_efidisk_open() for
power-of-two-ness and bail if invalid.

Reported-by: Jeremy Linton <jeremy.linton@arm.com>
---

v4 - Improved alignment checking as suggested by Vladimir, and moved
     the error exit as requested by Andrei.

 grub-core/disk/efi/efidisk.c | 48 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 1c00e3e..e4f4c25 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -493,8 +493,15 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
   m = d->block_io->media;
   /* FIXME: Probably it is better to store the block size in the disk,
      and total sectors should be replaced with total blocks.  */
-  grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
-		m, (unsigned long long) m->last_block, m->block_size);
+  grub_dprintf ("efidisk",
+		"m = %p, last block = %llx, block size = %x, io align = %x\n",
+		m, (unsigned long long) m->last_block, m->block_size,
+		m->io_align);
+
+  /* Ensure required buffer alignment is a power of two (or is zero). */
+  if (m->io_align & (m->io_align - 1))
+    return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d", m->io_align);
+
   disk->total_sectors = m->last_block + 1;
   /* Don't increase this value due to bug in some EFI.  */
   disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
@@ -524,15 +531,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
 {
   struct grub_efidisk_data *d;
   grub_efi_block_io_t *bio;
+  grub_efi_status_t status;
+  grub_size_t io_align, num_bytes;
+  char *aligned_buf;
 
   d = disk->data;
   bio = d->block_io;
 
-  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
-		     bio->media->media_id,
-		     (grub_efi_uint64_t) sector,
-		     (grub_efi_uintn_t) size << disk->log_sector_size,
-		     buf);
+  /* Set alignment to 1 if 0 specified */
+  io_align = bio->media->io_align ? bio->media->io_align : 1;
+  num_bytes = size << disk->log_sector_size;
+
+  if ((grub_addr_t) buf & (io_align - 1))
+    {
+      aligned_buf = grub_memalign (io_align, num_bytes);
+      if (! aligned_buf)
+	return GRUB_EFI_OUT_OF_RESOURCES;
+      if (wr)
+	grub_memcpy (aligned_buf, buf, num_bytes);
+    }
+  else
+    {
+      aligned_buf = buf;
+    }
+
+  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
+			bio->media->media_id, (grub_efi_uint64_t) sector,
+			(grub_efi_uintn_t) num_bytes, aligned_buf);
+
+  if ((grub_addr_t) buf & (io_align - 1))
+    {
+      if (!wr)
+	grub_memcpy (buf, aligned_buf, num_bytes);
+      grub_free (aligned_buf);
+    }
+
+  return status;
 }
 
 static grub_err_t
-- 
2.1.4

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]

On 18.02.2016 19:05, Leif Lindholm wrote:
> Returned from the OpenProtocol operation, the grub_efi_block_io_media
> structure contains the io_align field, specifying the minimum alignment
> required for buffers used in any data transfers with the device.
> 
> Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> this boundary, if the buffer passed to it does not already meet the
> requirements.
> 
> Also sanity check the io_align field in grub_efidisk_open() for
> power-of-two-ness and bail if invalid.
> 
I have committed this variant to make tests pass, so that I can release
beta3. Optimisations can go in separately after this
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> 
> v4 - Improved alignment checking as suggested by Vladimir, and moved
>      the error exit as requested by Andrei.
> 
>  grub-core/disk/efi/efidisk.c | 48 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 1c00e3e..e4f4c25 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -493,8 +493,15 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>    m = d->block_io->media;
>    /* FIXME: Probably it is better to store the block size in the disk,
>       and total sectors should be replaced with total blocks.  */
> -  grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
> -		m, (unsigned long long) m->last_block, m->block_size);
> +  grub_dprintf ("efidisk",
> +		"m = %p, last block = %llx, block size = %x, io align = %x\n",
> +		m, (unsigned long long) m->last_block, m->block_size,
> +		m->io_align);
> +
> +  /* Ensure required buffer alignment is a power of two (or is zero). */
> +  if (m->io_align & (m->io_align - 1))
> +    return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d", m->io_align);
> +
>    disk->total_sectors = m->last_block + 1;
>    /* Don't increase this value due to bug in some EFI.  */
>    disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
> @@ -524,15 +531,42 @@ grub_efidisk_readwrite (struct grub_disk *disk, grub_disk_addr_t sector,
>  {
>    struct grub_efidisk_data *d;
>    grub_efi_block_io_t *bio;
> +  grub_efi_status_t status;
> +  grub_size_t io_align, num_bytes;
> +  char *aligned_buf;
>  
>    d = disk->data;
>    bio = d->block_io;
>  
> -  return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> -		     bio->media->media_id,
> -		     (grub_efi_uint64_t) sector,
> -		     (grub_efi_uintn_t) size << disk->log_sector_size,
> -		     buf);
> +  /* Set alignment to 1 if 0 specified */
> +  io_align = bio->media->io_align ? bio->media->io_align : 1;
> +  num_bytes = size << disk->log_sector_size;
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      aligned_buf = grub_memalign (io_align, num_bytes);
> +      if (! aligned_buf)
> +	return GRUB_EFI_OUT_OF_RESOURCES;
> +      if (wr)
> +	grub_memcpy (aligned_buf, buf, num_bytes);
> +    }
> +  else
> +    {
> +      aligned_buf = buf;
> +    }
> +
> +  status =  efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> +			bio->media->media_id, (grub_efi_uint64_t) sector,
> +			(grub_efi_uintn_t) num_bytes, aligned_buf);
> +
> +  if ((grub_addr_t) buf & (io_align - 1))
> +    {
> +      if (!wr)
> +	grub_memcpy (buf, aligned_buf, num_bytes);
> +      grub_free (aligned_buf);
> +    }
> +
> +  return status;
>  }
>  
>  static grub_err_t
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]