Messages in this thread | | | Date | Wed, 6 Jul 2011 13:01:12 -0500 | Subject | Re: [PATCH v3 1/2] init: add root=PARTUUID=UUID/PARTNROFF=%d support | From | Will Drewry <> |
| |
On Tue, Jul 5, 2011 at 3:53 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 30 Jun 2011 16:10:06 -0500 > Will Drewry <wad@chromium.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |