On Thu, Jan 27, 2022 at 5:34 AM Daniel Golle <dan...@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 <computersforpe...@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
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
