[PATCH 1/3] ieee1275: ofdisk dangling pointer

[PATCH 1/3] ieee1275: ofdisk dangling pointer

[Eric Snowberg]

Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
"Don't continue to query block-size if disk doesn't have it.”
a dangling pointer was introduced.

Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
and then used again within the call to grub_ofdisk_get_block_size. This
solves many memory corruption issues we were seeing.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 grub-core/disk/ieee1275/ofdisk.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 331769b..4a5632c 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
     op = ofdisk_hash_find (devpath);
     if (!op)
       op = ofdisk_hash_add (devpath, NULL);
-    else
-      grub_free (devpath);
     if (!op)
-      return grub_errno;
+      {
+        grub_free (devpath);
+        return grub_errno;
+      }
     disk->id = (unsigned long) op;
     disk->data = op->open_path;
 
-- 
1.7.1

[PATCH 2/3] ieee1275: ofdisk memory leak

[Eric Snowberg]

Fix memory leak added within commit:
87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to
query block-size if disk doesn't have it.”

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 grub-core/disk/ieee1275/ofdisk.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 4a5632c..297f058 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
 
     err = grub_ofdisk_get_block_size (devpath, &block_size, op);
     if (err)
-      return err;
+      {
+        grub_free (devpath);
+        return err;
+      }
     if (block_size != 0)
       {
 	for (disk->log_sector_size = 0;
@@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
       disk->log_sector_size = 9;
   }
 
+  grub_free (devpath);
   return 0;
 }
 
-- 
1.7.1

[PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it

[Eric Snowberg]

Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
to query block-size if disk doesn't have it.”  Disks that returned 0 to the
block-size query, still get queried every time.

Fix logic in grub_ofdisk_get_block_size so the block size is not requested
upon each open since it will not change.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 grub-core/disk/ieee1275/ofdisk.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 297f058..a75ea51 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -35,7 +35,8 @@ struct ofdisk_hash_ent
   char *grub_devpath;
   int is_boot;
   int is_removable;
-  int block_size_fails;
+  int block_size_retries;
+  grub_uint32_t block_size;
   /* Pointer to shortest available name on nodes representing canonical names,
      otherwise NULL.  */
   const char *shortest;
@@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
       disk->log_sector_size = 9;
   }
 
+  if (last_ihandle)
+    grub_ieee1275_close (last_ihandle);
+
+  last_ihandle = 0;
+  last_devpath = NULL;
+  grub_ieee1275_open (devpath, &last_ihandle);
   grub_free (devpath);
+
+  if (! last_ihandle)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
+
   return 0;
 }
 
@@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
       grub_ieee1275_cell_t size2;
     } args_ieee1275;
 
+  if ((op->block_size_retries >= 2) || (op->block_size > 0))
+    {
+      *block_size = op->block_size;
+      return GRUB_ERR_NONE;
+    }
+
   if (last_ihandle)
     grub_ieee1275_close (last_ihandle);
 
@@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
     return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
 
   *block_size = 0;
-
-  if (op->block_size_fails >= 2)
-    return GRUB_ERR_NONE;
+  op->block_size_retries++;
 
   INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2);
   args_ieee1275.method = (grub_ieee1275_cell_t) "block-size";
@@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
   if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1)
     {
       grub_dprintf ("disk", "can't get block size: failed call-method\n");
-      op->block_size_fails++;
     }
   else if (args_ieee1275.result)
     {
       grub_dprintf ("disk", "can't get block size: %lld\n",
 		    (long long) args_ieee1275.result);
-      op->block_size_fails++;
     }
   else if (args_ieee1275.size1
 	   && !(args_ieee1275.size1 & (args_ieee1275.size1 - 1))
 	   && args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384)
     {
-      op->block_size_fails = 0;
+      op->block_size = args_ieee1275.size1;
       *block_size = args_ieee1275.size1;
     }
 
+  grub_ieee1275_close (last_ihandle);
+  last_ihandle = 0;
+
   return 0;
 }
-- 
1.7.1

Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 26.10.2015 22:43, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
> to query block-size if disk doesn't have it.”  Disks that returned 0 to the
> block-size query, still get queried every time.
> 
> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
> upon each open since it will not change.
> 
Is it true for removable disks as well? What about someone unplugging
USB stick and plugging-in 4K USB HDD?
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c |   30 +++++++++++++++++++++++-------
>  1 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 297f058..a75ea51 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -35,7 +35,8 @@ struct ofdisk_hash_ent
>    char *grub_devpath;
>    int is_boot;
>    int is_removable;
> -  int block_size_fails;
> +  int block_size_retries;
> +  grub_uint32_t block_size;
>    /* Pointer to shortest available name on nodes representing canonical names,
>       otherwise NULL.  */
>    const char *shortest;
> @@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>        disk->log_sector_size = 9;
>    }
>  
> +  if (last_ihandle)
> +    grub_ieee1275_close (last_ihandle);
> +
> +  last_ihandle = 0;
> +  last_devpath = NULL;
> +  grub_ieee1275_open (devpath, &last_ihandle);
>    grub_free (devpath);
> +
> +  if (! last_ihandle)
> +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
> +
>    return 0;
>  }
>  
> @@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
>        grub_ieee1275_cell_t size2;
>      } args_ieee1275;
>  
> +  if ((op->block_size_retries >= 2) || (op->block_size > 0))
> +    {
> +      *block_size = op->block_size;
> +      return GRUB_ERR_NONE;
> +    }
> +
>    if (last_ihandle)
>      grub_ieee1275_close (last_ihandle);
>  
> @@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
>      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
>  
>    *block_size = 0;
> -
> -  if (op->block_size_fails >= 2)
> -    return GRUB_ERR_NONE;
> +  op->block_size_retries++;
>  
>    INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2);
>    args_ieee1275.method = (grub_ieee1275_cell_t) "block-size";
> @@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
>    if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1)
>      {
>        grub_dprintf ("disk", "can't get block size: failed call-method\n");
> -      op->block_size_fails++;
>      }
>    else if (args_ieee1275.result)
>      {
>        grub_dprintf ("disk", "can't get block size: %lld\n",
>  		    (long long) args_ieee1275.result);
> -      op->block_size_fails++;
>      }
>    else if (args_ieee1275.size1
>  	   && !(args_ieee1275.size1 & (args_ieee1275.size1 - 1))
>  	   && args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384)
>      {
> -      op->block_size_fails = 0;
> +      op->block_size = args_ieee1275.size1;
>        *block_size = args_ieee1275.size1;
>      }
>  
> +  grub_ieee1275_close (last_ihandle);
> +  last_ihandle = 0;
> +
>    return 0;
>  }
> 



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

Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 26.10.2015 22:43, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
> "Don't continue to query block-size if disk doesn't have it.”
> a dangling pointer was introduced.
> 
> Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
> and then used again within the call to grub_ofdisk_get_block_size. This
> solves many memory corruption issues we were seeing.
> 
Committed, thanks
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 331769b..4a5632c 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>      op = ofdisk_hash_find (devpath);
>      if (!op)
>        op = ofdisk_hash_add (devpath, NULL);
> -    else
> -      grub_free (devpath);
>      if (!op)
> -      return grub_errno;
> +      {
> +        grub_free (devpath);
> +        return grub_errno;
> +      }
>      disk->id = (unsigned long) op;
>      disk->data = op->open_path;
>  
> 



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

Re: [PATCH 2/3] ieee1275: ofdisk memory leak

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 26.10.2015 22:43, Eric Snowberg wrote:
> Fix memory leak added within commit:
> 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to
> query block-size if disk doesn't have it.”
> 
Committed, thanks
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 4a5632c..297f058 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>  
>      err = grub_ofdisk_get_block_size (devpath, &block_size, op);
>      if (err)
> -      return err;
> +      {
> +        grub_free (devpath);
> +        return err;
> +      }
>      if (block_size != 0)
>        {
>  	for (disk->log_sector_size = 0;
> @@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>        disk->log_sector_size = 9;
>    }
>  
> +  grub_free (devpath);
>    return 0;
>  }
>  
> 



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

Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer

[Daniel Kiper]

On Mon, Oct 26, 2015 at 05:43:36PM -0400, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
> "Don't continue to query block-size if disk doesn't have it.???
> a dangling pointer was introduced.
>
> Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
> and then used again within the call to grub_ofdisk_get_block_size. This
> solves many memory corruption issues we were seeing.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Please CC me too if you repost patches on grub-devel.

Daniel

Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it

[Andrei Borzenkov]

On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 26.10.2015 22:43, Eric Snowberg wrote:
>> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
>> to query block-size if disk doesn't have it.”  Disks that returned 0 to the
>> block-size query, still get queried every time.
>>
>> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
>> upon each open since it will not change.
>>
> Is it true for removable disks as well? What about someone unplugging
> USB stick and plugging-in 4K USB HDD?

Does OBP alone have hotplug support until OS is running?

Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it

[Eric Snowberg]

> On Nov 10, 2015, at 1:01 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 
> On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> On 26.10.2015 22:43, Eric Snowberg wrote:
>>> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
>>> to query block-size if disk doesn't have it.”  Disks that returned 0 to the
>>> block-size query, still get queried every time.
>>> 
>>> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
>>> upon each open since it will not change.
>>> 
>> Is it true for removable disks as well? What about someone unplugging
>> USB stick and plugging-in 4K USB HDD?
> 
> Does OBP alone have hotplug support until OS is running?

AFAIK, SPARC OBP does not have hot plug support.