diff mbox

efidisk: Respect block_io_protocol buffer alignment

Message ID 1455807659-21474-1-git-send-email-leif.lindholm@linaro.org
State Superseded
Headers show

Commit Message

Leif Lindholm Feb. 18, 2016, 3 p.m. UTC 
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(-)

-- 
2.1.4


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

Comments

Leif Lindholm Feb. 18, 2016, 4:49 p.m. UTC | #1 
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
diff mbox

Patch

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