| Message ID | 20220116054831.351849-1-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Golle |
| Headers | show |
| Series | [firmware-utils,1/2] ptgen: add Chromium OS kernel partition support | expand |
Hi Brian, thank you for taking care of liberating the Google devices ;) Please see my comments inline below: On Sat, Jan 15, 2022 at 09:48:30PM -0800, Brian Norris wrote: > Chrom{ium,e} OS (shortened as "CrOS") bootloaders use a custom GPT > partition type to locate their kernel(s), with custom attributes for > noting properties around which partition(s) should be active and how > many times they've been tried as part of their A/B in-place upgrade > system. > > OpenWRT doesn't use A/B updates for upgrades (instead, just shutting > things down far enough to reprogram the necessary partitions), so all we > need to do is tell the bootloader which one is the kernel partition, and > how to use it (i.e., set the "successful" and "priority" attributes). > > ptgen already supports some basic GPT partition creation, so just > add support for a '-T <GPT partition type>' argument. Currently, this > only supports '-T cros_kernel', but it could be extended if there are > other GPT partition types needed. > > For GPT attribute and GUID definitions, see the CrOS verified boot > sources: > > https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/lib/cgptlib/include/cgptlib_internal.h > https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/include/gpt.h > > Wikipedia (!!) even notes the GUIDs: > https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_type_GUIDs > > The GUID is also recognized in fdisk, and likely other utilities, but > creation/manipulation is typically done via the 'cgpt' utility, provided > as part of the Chromium vboot_reference project. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > src/ptgen.c | 43 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 38 insertions(+), 5 deletions(-) > > diff --git a/src/ptgen.c b/src/ptgen.c > index 69757c1fc7dc..7220dde42b92 100644 > --- a/src/ptgen.c > +++ b/src/ptgen.c > @@ -70,6 +70,10 @@ typedef struct { > GUID_INIT( 0x21686148, 0x6449, 0x6E6F, \ > 0x74, 0x4E, 0x65, 0x65, 0x64, 0x45, 0x46, 0x49) > > +#define GUID_PARTITION_CHROME_OS_KERNEL \ > + GUID_INIT( 0xFE3A2A5D, 0x4F32, 0x41A7, \ > + 0xB7, 0x25, 0xAC, 0xCC, 0x32, 0x85, 0xA3, 0x09) > + > #define GUID_PARTITION_LINUX_FIT_GUID \ > GUID_INIT( 0xcae9be83, 0xb15f, 0x49cc, \ > 0x86, 0x3f, 0x08, 0x1b, 0x74, 0x4a, 0x2d, 0x93) > @@ -116,7 +120,9 @@ struct partinfo { > int hybrid; > char *name; > short int required; > + bool has_guid; > guid_t guid; > + uint64_t gattr; /* GPT partition attributes */ > }; > > /* GPT Partition table header */ > @@ -256,6 +262,23 @@ static inline int guid_parse(char *buf, guid_t *guid) > return 0; > } > > +/* > + * Map GPT partition types to partition GUIDs. > + * NB: not all GPT partition types have an equivalent MBR type. > + */ > +static inline bool parse_gpt_parttype(const char *type, struct partinfo *part) > +{ > + if (!strcmp(type, "cros_kernel")) { > + part->has_guid = true; > + part->guid = GUID_PARTITION_CHROME_OS_KERNEL; > + /* Default attributes: bootable kernel. */ > + part->gattr = (1ULL << 48) | /* priority=1 */ > + (1ULL << 56); /* success=1 */ > + return true; > + } > + return false; > +} > + > /* init an utf-16 string from utf-8 string */ > static inline void init_utf16(char *str, uint16_t *buf, unsigned bufsize) > { > @@ -416,6 +439,7 @@ static int gen_gptable(uint32_t signature, guid_t guid, unsigned nr) > to_chs(sect - 1, pte[1].chs_end); > pmbr++; > } > + gpte[i].attr = parts[i].gattr; > > if (parts[i].name) > init_utf16(parts[i].name, (uint16_t *)gpte[i].name, GPT_ENTRY_NAME_SIZE / sizeof(uint16_t)); > @@ -523,7 +547,9 @@ fail: > > static void usage(char *prog) > { > - fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile> [-a 0..4] [-l <align kB>] [-G <guid>] [[-t <type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); > + fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile>\n" > + " [-a 0..4] [-l <align kB>] [-G <guid>]\n" > + " [[-t <type> | -T <GPT part type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); > exit(EXIT_FAILURE); > } > > @@ -559,9 +585,8 @@ int main (int argc, char **argv) > uint32_t signature = 0x5452574F; /* 'OWRT' */ > guid_t guid = GUID_INIT( signature, 0x2211, 0x4433, \ > 0x55, 0x66, 0x77, 0x88, 0x99, 0xAA, 0xBB, 0x00); > - guid_t part_guid = GUID_PARTITION_BASIC_DATA; I think we should keep the default GUID, as there are uses for that other than Chrome OS which expect a valid part_guid. > > - while ((ch = getopt(argc, argv, "h:s:p:a:t:o:vnHN:gl:rS:G:")) != -1) { > + while ((ch = getopt(argc, argv, "h:s:p:a:t:T:o:vnHN:gl:rS:G:")) != -1) { > switch (ch) { > case 'o': > filename = optarg; > @@ -594,12 +619,12 @@ int main (int argc, char **argv) > *(p++) = 0; > parts[part].start = to_kbytes(p); > } > - part_guid = type_to_guid_and_name(type, &name); > + if (!parts[part].has_guid) > + parts[part].guid = type_to_guid_and_name(type, &name); > parts[part].size = to_kbytes(optarg); > parts[part].required = required; > parts[part].name = name; > parts[part].hybrid = hybrid; > - parts[part].guid = part_guid; This should probably be part of the conditional if (!parts[part].has_guid) above instead of just removing it as this will break existing non-Chrome OS uses of GPT. > fprintf(stderr, "part %ld %ld\n", parts[part].start, parts[part].size); > parts[part++].type = type; > /* > @@ -630,6 +655,14 @@ int main (int argc, char **argv) > case 'S': > signature = strtoul(optarg, NULL, 0); > break; > + case 'T': > + if (!parse_gpt_parttype(optarg, &parts[part])) { > + fprintf(stderr, > + "Invalid GPT partition type \"%s\"\n", > + optarg); > + exit(EXIT_FAILURE); > + } > + break; > case 'G': > if (guid_parse(optarg, &guid)) { > fputs("Invalid guid string\n", stderr); > -- > 2.34.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Thu, Jan 27, 2022 at 5:34 AM Daniel Golle <daniel@makrotopia.org> wrote: > > Hi Brian, Hi Daniel, > thank you for taking care of liberating the Google devices ;) ;) > Please see my comments inline below: > > On Sat, Jan 15, 2022 at 09:48:30PM -0800, Brian Norris wrote: > > Chrom{ium,e} OS (shortened as "CrOS") bootloaders use a custom GPT > > partition type to locate their kernel(s), with custom attributes for > > noting properties around which partition(s) should be active and how > > many times they've been tried as part of their A/B in-place upgrade > > system. > > > > OpenWRT doesn't use A/B updates for upgrades (instead, just shutting > > things down far enough to reprogram the necessary partitions), so all we > > need to do is tell the bootloader which one is the kernel partition, and > > how to use it (i.e., set the "successful" and "priority" attributes). > > > > ptgen already supports some basic GPT partition creation, so just > > add support for a '-T <GPT partition type>' argument. Currently, this > > only supports '-T cros_kernel', but it could be extended if there are > > other GPT partition types needed. > > > > For GPT attribute and GUID definitions, see the CrOS verified boot > > sources: > > > > https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/lib/cgptlib/include/cgptlib_internal.h > > https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/include/gpt.h > > > > Wikipedia (!!) even notes the GUIDs: > > https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_type_GUIDs > > > > The GUID is also recognized in fdisk, and likely other utilities, but > > creation/manipulation is typically done via the 'cgpt' utility, provided > > as part of the Chromium vboot_reference project. > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > --- > > src/ptgen.c | 43 ++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 38 insertions(+), 5 deletions(-) > > > > diff --git a/src/ptgen.c b/src/ptgen.c > > index 69757c1fc7dc..7220dde42b92 100644 > > --- a/src/ptgen.c > > +++ b/src/ptgen.c > > @@ -70,6 +70,10 @@ typedef struct { > > GUID_INIT( 0x21686148, 0x6449, 0x6E6F, \ > > 0x74, 0x4E, 0x65, 0x65, 0x64, 0x45, 0x46, 0x49) > > > > +#define GUID_PARTITION_CHROME_OS_KERNEL \ > > + GUID_INIT( 0xFE3A2A5D, 0x4F32, 0x41A7, \ > > + 0xB7, 0x25, 0xAC, 0xCC, 0x32, 0x85, 0xA3, 0x09) > > + > > #define GUID_PARTITION_LINUX_FIT_GUID \ > > GUID_INIT( 0xcae9be83, 0xb15f, 0x49cc, \ > > 0x86, 0x3f, 0x08, 0x1b, 0x74, 0x4a, 0x2d, 0x93) > > @@ -116,7 +120,9 @@ struct partinfo { > > int hybrid; > > char *name; > > short int required; > > + bool has_guid; > > guid_t guid; > > + uint64_t gattr; /* GPT partition attributes */ > > }; > > > > /* GPT Partition table header */ > > @@ -256,6 +262,23 @@ static inline int guid_parse(char *buf, guid_t *guid) > > return 0; > > } > > > > +/* > > + * Map GPT partition types to partition GUIDs. > > + * NB: not all GPT partition types have an equivalent MBR type. > > + */ > > +static inline bool parse_gpt_parttype(const char *type, struct partinfo *part) > > +{ > > + if (!strcmp(type, "cros_kernel")) { > > + part->has_guid = true; > > + part->guid = GUID_PARTITION_CHROME_OS_KERNEL; > > + /* Default attributes: bootable kernel. */ > > + part->gattr = (1ULL << 48) | /* priority=1 */ > > + (1ULL << 56); /* success=1 */ > > + return true; > > + } > > + return false; > > +} > > + > > /* init an utf-16 string from utf-8 string */ > > static inline void init_utf16(char *str, uint16_t *buf, unsigned bufsize) > > { > > @@ -416,6 +439,7 @@ static int gen_gptable(uint32_t signature, guid_t guid, unsigned nr) > > to_chs(sect - 1, pte[1].chs_end); > > pmbr++; > > } > > + gpte[i].attr = parts[i].gattr; > > > > if (parts[i].name) > > init_utf16(parts[i].name, (uint16_t *)gpte[i].name, GPT_ENTRY_NAME_SIZE / sizeof(uint16_t)); > > @@ -523,7 +547,9 @@ fail: > > > > static void usage(char *prog) > > { > > - fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile> [-a 0..4] [-l <align kB>] [-G <guid>] [[-t <type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); > > + fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile>\n" > > + " [-a 0..4] [-l <align kB>] [-G <guid>]\n" > > + " [[-t <type> | -T <GPT part type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); > > exit(EXIT_FAILURE); > > } > > > > @@ -559,9 +585,8 @@ int main (int argc, char **argv) > > uint32_t signature = 0x5452574F; /* 'OWRT' */ > > guid_t guid = GUID_INIT( signature, 0x2211, 0x4433, \ > > 0x55, 0x66, 0x77, 0x88, 0x99, 0xAA, 0xBB, 0x00); > > - guid_t part_guid = GUID_PARTITION_BASIC_DATA; > > I think we should keep the default GUID, as there are uses for that > other than Chrome OS which expect a valid part_guid. To be clear, the line you're highlighting was a dead assignment. In the original code, it was overwritten without ever being used. > > > > - while ((ch = getopt(argc, argv, "h:s:p:a:t:o:vnHN:gl:rS:G:")) != -1) { > > + while ((ch = getopt(argc, argv, "h:s:p:a:t:T:o:vnHN:gl:rS:G:")) != -1) { > > switch (ch) { > > case 'o': > > filename = optarg; > > @@ -594,12 +619,12 @@ int main (int argc, char **argv) > > *(p++) = 0; > > parts[part].start = to_kbytes(p); > > } > > - part_guid = type_to_guid_and_name(type, &name); ^^^ Previously, this clobbered the GUID_PARTITION_BASIC_DATA default. > > + if (!parts[part].has_guid) > > + parts[part].guid = type_to_guid_and_name(type, &name); > > parts[part].size = to_kbytes(optarg); > > parts[part].required = required; > > parts[part].name = name; > > parts[part].hybrid = hybrid; > > - parts[part].guid = part_guid; > > This should probably be part of the conditional > if (!parts[part].has_guid) > above instead of just removing it as this will break existing > non-Chrome OS uses of GPT. But it _is_ part of the above conditional? These two versions are equivalent: part_guid = type_to_guid_and_name(type, &name); parts[part].guid = part_guid; vs. parts[part].guid = type_to_guid_and_name(type, &name); But I stuck it beneath a (!parts[part].has_guid) conditional, because for the has_guid=true case, I've already written the guid in parse_gpt_parttype(). So, this might be a little confusing to read, but I don't believe it breaks anything. Apologies if I've missed something. And even if it's correct, I'm open to suggestions on writing it more clearly, within the style of this position-dependent argument list. Regards Brian > > fprintf(stderr, "part %ld %ld\n", parts[part].start, parts[part].size); > > parts[part++].type = type; > > /* > > @@ -630,6 +655,14 @@ int main (int argc, char **argv) > > case 'S': > > signature = strtoul(optarg, NULL, 0); > > break; > > + case 'T': > > + if (!parse_gpt_parttype(optarg, &parts[part])) { > > + fprintf(stderr, > > + "Invalid GPT partition type \"%s\"\n", > > + optarg); > > + exit(EXIT_FAILURE); > > + } > > + break; > > case 'G': > > if (guid_parse(optarg, &guid)) { > > fputs("Invalid guid string\n", stderr); > > -- > > 2.34.1
On Thu, Jan 27, 2022 at 08:56:26PM -0800, Brian Norris wrote: > On Thu, Jan 27, 2022 at 5:34 AM Daniel Golle <daniel@makrotopia.org> wrote: > > Please see my comments inline below: > > > > On Sat, Jan 15, 2022 at 09:48:30PM -0800, Brian Norris wrote: > > > [...] > > > @@ -559,9 +585,8 @@ int main (int argc, char **argv) > > > uint32_t signature = 0x5452574F; /* 'OWRT' */ > > > guid_t guid = GUID_INIT( signature, 0x2211, 0x4433, \ > > > 0x55, 0x66, 0x77, 0x88, 0x99, 0xAA, 0xBB, 0x00); > > > - guid_t part_guid = GUID_PARTITION_BASIC_DATA; > > > > I think we should keep the default GUID, as there are uses for that > > other than Chrome OS which expect a valid part_guid. > > To be clear, the line you're highlighting was a dead assignment. In > the original code, it was overwritten without ever being used. > [...] > So, this might be a little confusing to read, but I don't believe it > breaks anything. It's indeed just a bit confusing (and has been confusing also before you ever touched it). The default to GUID_PARTITION_BASIC_DATA is actually set in type_to_guid_and_name() function, so you are right, it's safe to drop it from the main() function as it was already dead code... I'll apply your patches as-is. Thank you!
On Fri, Jan 28, 2022 at 12:53:04PM +0000, Daniel Golle wrote: > I'll apply your patches as-is. > > > Thank you! Thanks for reviewing and merging! Now to get the rest of the Google WiFi stuff [1] in good enough shape to merge. (Maybe it is already? Although I'm unsure if there are side effects for other ipq40xx firmwares.) And maybe even OnHub for good measure. Brian [1] https://github.com/computersforpeace/openwrt/commits/gale
diff --git a/src/ptgen.c b/src/ptgen.c index 69757c1fc7dc..7220dde42b92 100644 --- a/src/ptgen.c +++ b/src/ptgen.c @@ -70,6 +70,10 @@ typedef struct { GUID_INIT( 0x21686148, 0x6449, 0x6E6F, \ 0x74, 0x4E, 0x65, 0x65, 0x64, 0x45, 0x46, 0x49) +#define GUID_PARTITION_CHROME_OS_KERNEL \ + GUID_INIT( 0xFE3A2A5D, 0x4F32, 0x41A7, \ + 0xB7, 0x25, 0xAC, 0xCC, 0x32, 0x85, 0xA3, 0x09) + #define GUID_PARTITION_LINUX_FIT_GUID \ GUID_INIT( 0xcae9be83, 0xb15f, 0x49cc, \ 0x86, 0x3f, 0x08, 0x1b, 0x74, 0x4a, 0x2d, 0x93) @@ -116,7 +120,9 @@ struct partinfo { int hybrid; char *name; short int required; + bool has_guid; guid_t guid; + uint64_t gattr; /* GPT partition attributes */ }; /* GPT Partition table header */ @@ -256,6 +262,23 @@ static inline int guid_parse(char *buf, guid_t *guid) return 0; } +/* + * Map GPT partition types to partition GUIDs. + * NB: not all GPT partition types have an equivalent MBR type. + */ +static inline bool parse_gpt_parttype(const char *type, struct partinfo *part) +{ + if (!strcmp(type, "cros_kernel")) { + part->has_guid = true; + part->guid = GUID_PARTITION_CHROME_OS_KERNEL; + /* Default attributes: bootable kernel. */ + part->gattr = (1ULL << 48) | /* priority=1 */ + (1ULL << 56); /* success=1 */ + return true; + } + return false; +} + /* init an utf-16 string from utf-8 string */ static inline void init_utf16(char *str, uint16_t *buf, unsigned bufsize) { @@ -416,6 +439,7 @@ static int gen_gptable(uint32_t signature, guid_t guid, unsigned nr) to_chs(sect - 1, pte[1].chs_end); pmbr++; } + gpte[i].attr = parts[i].gattr; if (parts[i].name) init_utf16(parts[i].name, (uint16_t *)gpte[i].name, GPT_ENTRY_NAME_SIZE / sizeof(uint16_t)); @@ -523,7 +547,9 @@ fail: static void usage(char *prog) { - fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile> [-a 0..4] [-l <align kB>] [-G <guid>] [[-t <type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); + fprintf(stderr, "Usage: %s [-v] [-n] [-g] -h <heads> -s <sectors> -o <outputfile>\n" + " [-a 0..4] [-l <align kB>] [-G <guid>]\n" + " [[-t <type> | -T <GPT part type>] [-r] [-N <name>] -p <size>[@<start>]...] \n", prog); exit(EXIT_FAILURE); } @@ -559,9 +585,8 @@ int main (int argc, char **argv) uint32_t signature = 0x5452574F; /* 'OWRT' */ guid_t guid = GUID_INIT( signature, 0x2211, 0x4433, \ 0x55, 0x66, 0x77, 0x88, 0x99, 0xAA, 0xBB, 0x00); - guid_t part_guid = GUID_PARTITION_BASIC_DATA; - while ((ch = getopt(argc, argv, "h:s:p:a:t:o:vnHN:gl:rS:G:")) != -1) { + while ((ch = getopt(argc, argv, "h:s:p:a:t:T:o:vnHN:gl:rS:G:")) != -1) { switch (ch) { case 'o': filename = optarg; @@ -594,12 +619,12 @@ int main (int argc, char **argv) *(p++) = 0; parts[part].start = to_kbytes(p); } - part_guid = type_to_guid_and_name(type, &name); + if (!parts[part].has_guid) + parts[part].guid = type_to_guid_and_name(type, &name); parts[part].size = to_kbytes(optarg); parts[part].required = required; parts[part].name = name; parts[part].hybrid = hybrid; - parts[part].guid = part_guid; fprintf(stderr, "part %ld %ld\n", parts[part].start, parts[part].size); parts[part++].type = type; /* @@ -630,6 +655,14 @@ int main (int argc, char **argv) case 'S': signature = strtoul(optarg, NULL, 0); break; + case 'T': + if (!parse_gpt_parttype(optarg, &parts[part])) { + fprintf(stderr, + "Invalid GPT partition type \"%s\"\n", + optarg); + exit(EXIT_FAILURE); + } + break; case 'G': if (guid_parse(optarg, &guid)) { fputs("Invalid guid string\n", stderr);
Chrom{ium,e} OS (shortened as "CrOS") bootloaders use a custom GPT partition type to locate their kernel(s), with custom attributes for noting properties around which partition(s) should be active and how many times they've been tried as part of their A/B in-place upgrade system. OpenWRT doesn't use A/B updates for upgrades (instead, just shutting things down far enough to reprogram the necessary partitions), so all we need to do is tell the bootloader which one is the kernel partition, and how to use it (i.e., set the "successful" and "priority" attributes). ptgen already supports some basic GPT partition creation, so just add support for a '-T <GPT partition type>' argument. Currently, this only supports '-T cros_kernel', but it could be extended if there are other GPT partition types needed. For GPT attribute and GUID definitions, see the CrOS verified boot sources: https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/lib/cgptlib/include/cgptlib_internal.h https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/refs/heads/master/firmware/include/gpt.h Wikipedia (!!) even notes the GUIDs: https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_type_GUIDs The GUID is also recognized in fdisk, and likely other utilities, but creation/manipulation is typically done via the 'cgpt' utility, provided as part of the Chromium vboot_reference project. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- src/ptgen.c | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-)