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 <address@hidden>
> > ---
> > 
> > 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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel