efidisk: Respect block_io_protocol buffer alignment

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(-)

-- 
2.1.4


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

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