Re: OLPC regression, ofdisk stops working

[Bean]

Hi,

There is something wrong with r2132, now childtype is a pointer, so
sizeof childtype == 4, the name would be truncated to 4 characters.

diff --git a/kern/ieee1275/openfw.c b/kern/ieee1275/openfw.c
index e7979f4..42d9278 100644
--- a/kern/ieee1275/openfw.c
+++ b/kern/ieee1275/openfw.c
@@ -78,15 +78,15 @@ grub_children_iterate (char *devpath,
       grub_ssize_t actual;

       if (grub_ieee1275_get_property (child, "device_type", childtype,
-                                     sizeof childtype, &actual))
+                                     IEEE1275_MAX_PROP_LEN, &actual))
        continue;

-      if (grub_ieee1275_package_to_path (child, childpath, sizeof childpath,
-                                        &actual))
+      if (grub_ieee1275_package_to_path (child, childpath,
+                                        IEEE1275_MAX_PATH_LEN, &actual))
        continue;

       if (grub_ieee1275_get_property (child, "name", childname,
-                                     sizeof childname, &actual))
+                                     IEEE1275_MAX_PATH_LEN, &actual))
        continue;

       grub_sprintf (fullname, "%s/%s", devpath, childname);


On Fri, Jul 10, 2009 at 12:03 AM, Robert Millan<address@hidden> wrote:
>
> Hi,
>
> I got completely puzzled at this one.   Turns out r2132 broke ofdisk on
> OLPC.  But I don't see anything wrong in this commit.
>
> I isolated the change that causes this breakage, and it's very confusing.
> It turns out that allocating devtype in the heap instead of the stack
> causes its result to be truncated to 4 bytes (+ null terminator).
>
> I'm not sure what can we do about this.  If we're certain it's an OFW
> bug, perhaps we could workaround it by comparing only the first 4 bytes
> of the result.  This worked for me:
>
> -      if (! grub_strcmp (alias->type, "block"))
> +      if (! grub_strncmp (alias->type, "bloc", 4))
>
> (but using the existing "workaround framework")
>
> but it's scary.  I don't know if this 4-byte limit is consistent, or
> will differ arbitrarily.  Maybe we could issue a warning or so, I don't
> know.
>
> Is there someone else (Bean?) who can reproduce this problem?  Does it
> fail in the same way?
>
> On Wed, Apr 22, 2009 at 09:46:55AM +0000, David S. Miller wrote:
>> Revision: 2132
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=grub&revision=2132
>> Author:   davem
>> Date:     2009-04-22 09:46:54 +0000 (Wed, 22 Apr 2009)
>> Log Message:
>> -----------
>>       * include/grub/ieee1275/ieee1275.h (IEEE1275_MAX_PROP_LEN,
>>       IEEE1275_MAX_PATH_LEN): Define.
>>       * kern/ieee1275/openfw.c (grub_children_iterate): Dynamically
>>       allocate 'childtype', 'childpath', 'childname', and 'fullname'.
>>       (grub_devalias_iterate): Dynamically allocate 'aliasname' and
>>       'devtype'.  Explicitly NULL terminate devalias expansion.
>>
>> Modified Paths:
>> --------------
>>     trunk/grub2/ChangeLog
>>     trunk/grub2/include/grub/ieee1275/ieee1275.h
>>     trunk/grub2/kern/ieee1275/openfw.c
>>
>> Modified: trunk/grub2/ChangeLog
>> ===================================================================
>> --- trunk/grub2/ChangeLog     2009-04-22 09:45:43 UTC (rev 2131)
>> +++ trunk/grub2/ChangeLog     2009-04-22 09:46:54 UTC (rev 2132)
>> @@ -3,6 +3,13 @@
>>       * kern/ieee1275/mmap.c (grub_machine_mmap_iterate): If size_cells
>>       is larger than address_cells, use that value for address_cells too.
>>
>> +     * include/grub/ieee1275/ieee1275.h (IEEE1275_MAX_PROP_LEN,
>> +     IEEE1275_MAX_PATH_LEN): Define.
>> +     * kern/ieee1275/openfw.c (grub_children_iterate): Dynamically
>> +     allocate 'childtype', 'childpath', 'childname', and 'fullname'.
>> +     (grub_devalias_iterate): Dynamically allocate 'aliasname' and
>> +     'devtype'.  Explicitly NULL terminate devalias expansion.
>> +
>>  2009-04-19  Vladimir Serbinenko <address@hidden>
>>
>>       Correct GPT definition
>>
>> Modified: trunk/grub2/include/grub/ieee1275/ieee1275.h
>> ===================================================================
>> --- trunk/grub2/include/grub/ieee1275/ieee1275.h      2009-04-22 09:45:43 
>> UTC (rev 2131)
>> +++ trunk/grub2/include/grub/ieee1275/ieee1275.h      2009-04-22 09:46:54 
>> UTC (rev 2132)
>> @@ -39,6 +39,9 @@
>>    unsigned int size;
>>  };
>>
>> +#define IEEE1275_MAX_PROP_LEN        8192
>> +#define IEEE1275_MAX_PATH_LEN        256
>> +
>>  #ifndef IEEE1275_CALL_ENTRY_FN
>>  #define IEEE1275_CALL_ENTRY_FN(args) (*grub_ieee1275_entry_fn) (args)
>>  #endif
>>
>> Modified: trunk/grub2/kern/ieee1275/openfw.c
>> ===================================================================
>> --- trunk/grub2/kern/ieee1275/openfw.c        2009-04-22 09:45:43 UTC (rev 
>> 2131)
>> +++ trunk/grub2/kern/ieee1275/openfw.c        2009-04-22 09:46:54 UTC (rev 
>> 2132)
>> @@ -38,6 +38,8 @@
>>  {
>>    grub_ieee1275_phandle_t dev;
>>    grub_ieee1275_phandle_t child;
>> +  char *childtype, *childpath;
>> +  char *childname, *fullname;
>>
>>    if (grub_ieee1275_finddevice (devpath, &dev))
>>      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
>> @@ -45,13 +47,33 @@
>>    if (grub_ieee1275_child (dev, &child))
>>      return grub_error (GRUB_ERR_BAD_DEVICE, "Device has no children");
>>
>> +  childtype = grub_malloc (IEEE1275_MAX_PROP_LEN);
>> +  if (!childtype)
>> +    return grub_errno;
>> +  childpath = grub_malloc (IEEE1275_MAX_PATH_LEN);
>> +  if (!childpath)
>> +    {
>> +      grub_free (childtype);
>> +      return grub_errno;
>> +    }
>> +  childname = grub_malloc (IEEE1275_MAX_PROP_LEN);
>> +  if (!childname)
>> +    {
>> +      grub_free (childpath);
>> +      grub_free (childtype);
>> +      return grub_errno;
>> +    }
>> +  fullname = grub_malloc (IEEE1275_MAX_PATH_LEN);
>> +  if (!fullname)
>> +    {
>> +      grub_free (childname);
>> +      grub_free (childpath);
>> +      grub_free (childtype);
>> +      return grub_errno;
>> +    }
>> +
>>    do
>>      {
>> -      /* XXX: Don't use hardcoded path lengths.  */
>> -      char childtype[64];
>> -      char childpath[64];
>> -      char childname[64];
>> -      char fullname[64];
>>        struct grub_ieee1275_devalias alias;
>>        grub_ssize_t actual;
>>
>> @@ -76,6 +98,11 @@
>>      }
>>    while (grub_ieee1275_peer (child, &child));
>>
>> +  grub_free (fullname);
>> +  grub_free (childname);
>> +  grub_free (childpath);
>> +  grub_free (childtype);
>> +
>>    return 0;
>>  }
>>
>> @@ -85,13 +112,23 @@
>>  grub_devalias_iterate (int (*hook) (struct grub_ieee1275_devalias *alias))
>>  {
>>    grub_ieee1275_phandle_t aliases;
>> -  char aliasname[32];
>> +  char *aliasname, *devtype;
>>    grub_ssize_t actual;
>>    struct grub_ieee1275_devalias alias;
>>
>>    if (grub_ieee1275_finddevice ("/aliases", &aliases))
>>      return -1;
>>
>> +  aliasname = grub_malloc (IEEE1275_MAX_PROP_LEN);
>> +  if (!aliasname)
>> +    return grub_errno;
>> +  devtype = grub_malloc (IEEE1275_MAX_PROP_LEN);
>> +  if (!devtype)
>> +    {
>> +      grub_free (aliasname);
>> +      return grub_errno;
>> +    }
>> +
>>    /* Find the first property.  */
>>    aliasname[0] = '\0';
>>
>> @@ -100,8 +137,6 @@
>>        grub_ieee1275_phandle_t dev;
>>        grub_ssize_t pathlen;
>>        char *devpath;
>> -      /* XXX: This should be large enough for any possible case.  */
>> -      char devtype[64];
>>
>>        grub_dprintf ("devalias", "devalias name = %s\n", aliasname);
>>
>> @@ -111,9 +146,17 @@
>>        if (!grub_strcmp (aliasname, "name"))
>>       continue;
>>
>> +      /* Sun's OpenBoot often doesn't zero terminate the device alias
>> +      strings, so we will add a NULL byte at the end explicitly.  */
>> +      pathlen += 1;
>> +
>>        devpath = grub_malloc (pathlen);
>>        if (! devpath)
>> -     return grub_errno;
>> +     {
>> +       grub_free (devtype);
>> +       grub_free (aliasname);
>> +       return grub_errno;
>> +     }
>>
>>        if (grub_ieee1275_get_property (aliases, aliasname, devpath, pathlen,
>>                                     &actual))
>> @@ -121,6 +164,7 @@
>>         grub_dprintf ("devalias", "get_property (%s) failed\n", aliasname);
>>         goto nextprop;
>>       }
>> +      devpath [actual] = '\0';
>>
>>        if (grub_ieee1275_finddevice (devpath, &dev))
>>       {
>> @@ -144,6 +188,8 @@
>>        grub_free (devpath);
>>      }
>>
>> +  grub_free (devtype);
>> +  grub_free (aliasname);
>>    return 0;
>>  }
>>
>>
>>
>>
>>
>
> --
> Robert Millan
>
>  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
>  how) you may access your data; but nobody's threatening your freedom: we
>  still allow you to remove your data and not access it at all."
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Bean