Re: [PATCH v3 1/2] init: add root=PARTUUID=UUID/PARTNROFF=%d support

[Will Drewry]

On Tue, Jul 5, 2011 at 3:53 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 30 Jun 2011 16:10:06 -0500
> Will Drewry <wad@xxxxxxxxxxxx> wrote:
>
>> Expand root=PARTUUID=UUID syntax to support selecting a root partition
>> by integer offset from a known, unique partition.  This approach
>> provides similar properties to specifying a device and partition number,
>> but using the UUID as the unique path prior to evaluating the offset.
>>
>> For example,
>>   root=PARTUUID=99DE9194-FC15-4223-9192-FC243948F88B/PARTNROFF=1
>> selects the partition with UUID 99DE.. then select the next
>> partition.
>>
>> This change is motivated by a particular usecase in Chromium OS where
>> the bootloader can easily determine what partition it is on (by UUID)
>> but doesn't perform general partition table walking.
>>
>> That said, support for this model provides a direct mechanism for the
>> user to modify the root partition to boot without specifically needing
>> to extract each UUID or update the bootloader explicitly when the root
>> partition UUID is changed (if it is recreated to be larger, for
>> instance).  Pinning to a /boot-style partition UUID allows the arbitrary
>> root partition reconfiguration/modifications with slightly less
>> ambiguity than just [dev][partition] and less stringency than the
>> specific root partition UUID.
>>
>> ...
>>
>>  static dev_t devt_from_partuuid(char *uuid_str)
>> @@ -98,6 +101,22 @@ static dev_t devt_from_partuuid(char *uuid_str)
>>       dev_t res = 0;
>>       struct device *dev = NULL;
>>       u8 uuid[16];
>> +     struct gendisk *disk;
>> +     struct hd_struct *part;
>> +     int offset = 0;
>> +
>> +     if (strlen(uuid_str) < 36)
>> +             goto done;
>
> I think this secretly changes behaviour?  Previously the code would have
> accepted a less-than-36-byte UUID and would have done <something> with
> it.  Now, it fails.
>
> What was <something>, and what is the reason for this (undocumented!)
> change?

Nice catch.  Initially and currently, the only caller to
devt_from_partuuid is name_to_dev_t.  name_to_dev_t() was forking on
PARTUUID and length == UUID length.  However, if someone had called
directly into devt_from_partuuid, no bounds checking would've occurred
and out of bounds reads may have resulted.

This just moves the check into devt_from_partuuid to unify the length
checking logic with the functional logic.  Now devt_from_partuuid is
safer (kinda) for other init-time callers and allows for detecting
additions.  (E.g., if Kay wants to add more / arguments.)

>> +     /* Check for optional partition number offset attributes. */
>> +     if (uuid_str[36]) {
>> +             /* Explicitly fail on poor PARTUUID syntax. */
>> +             if (sscanf(&uuid_str[36], "/PARTNROFF=%d", &offset) != 1) {
>> +                     printk(KERN_ERR "VFS: PARTUUID= is invalid.\n"
>> +                      "Expected PARTUUID=<valid-uuid-id>[/PARTNROFF=%%d]\n");
>
> The check isn't complete - afacit input of the form PARTNROFF=42foo
> will be treated as PARTNROFF=42?

Completely true.  I can post another version that either pulls a
trailing %c (which should fail) or uses %n.  I have somewhat limited
internet connectivity right now, but I will follow up with a final
clean up when I can (>~week)

Thanks for the close review!
will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/