| Message ID | 1448003177-26917-5-git-send-email-l.majewski@majess.pl |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Lukasz, On 11/20/2015 08:06 AM, Lukasz Majewski wrote: > This commit provides definition and declaration of GPT verification > functions - namely gpt_verify_headers() and gpt_verify_partitions(). > The former is used to only check CRC32 of GPT's header and PTEs. > The latter examines each partition entry and compare attributes such as: > name, start offset and size with ones provided at '$partitions' env > variable. > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > Reviewed-by: Tom Rini <trini@konsulko.com> > > --- > Changes for v2: > - Provide support for situation where "start" attribute at '$partitions' > env variable is not provided. I've got few comments to this code. > --- > disk/part_efi.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/part.h | 35 ++++++++++++++++++ > 2 files changed, 145 insertions(+) > > diff --git a/disk/part_efi.c b/disk/part_efi.c > index ea9c615..40f0b36 100644 > --- a/disk/part_efi.c > +++ b/disk/part_efi.c > @@ -578,6 +578,116 @@ err: > return ret; > } > Probably print_efiname() could meet your requirements. > +static void gpt_convert_efi_name_to_char(char *s, efi_char16_t *es, int n) > +{ > + char *ess = (char *)es; > + int i, j; > + > + memset(s, '\0', n); > + > + for (i = 0, j = 0; j < n; i += 2, j++) { > + s[j] = ess[i]; > + if (!ess[i]) > + return; > + } > +} > + > +int gpt_verify_headers(block_dev_desc_t *dev_desc, gpt_header *gpt_head, > + gpt_entry **gpt_pte) > +{ > + /* > + * This function validates AND > + * fills in the GPT header and PTE > + */ > + if (is_gpt_valid(dev_desc, > + GPT_PRIMARY_PARTITION_TABLE_LBA, > + gpt_head, gpt_pte) != 1) { > + printf("%s: *** ERROR: Invalid GPT ***\n", > + __func__); > + return -1; > + } > + if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), > + gpt_head, gpt_pte) != 1) { > + printf("%s: *** ERROR: Invalid Backup GPT ***\n", > + __func__); > + return -1; > + } > + > + return 0; > +} > + I've got an error for Trats2: -------------------------------------------------------------- Trats2 # print partitions partitions=uuid_disk=${uuid_gpt_disk};name=csa-mmc,start=5MiB,size=8MiB,uuid=${u uid_gpt_csa-mmc};name=boot,size=60MiB,uuid=${uuid_gpt_boot};name=qboot,size=100M iB,uuid=${uuid_gpt_qboot};name=csc,size=150MiB,uuid=${uuid_gpt_csc};name=platfor m,size=1536MiB,uuid=${uuid_gpt_platform};name=data,size=3000MiB,uuid=${uuid_gpt_ data};name=ums,size=-,uuid=${uuid_gpt_ums} Trats2 # gpt write mmc 0 $partitions Writing GPT: success! Trats2 # gpt verify mmc 0 $partitions ERROR: Partition ums size: 20826079 does not match 0! at disk/part_efi.c:662/gpt_verify_partitions() Verify GPT: error! Trats2 # -------------------------------------------------------------- The function set_gpt_info() in file common/cmd_gpt.c doesn't fill the 'partitions' as your code expects. There are some assumptions, respected by gpt_fill_pte(), like empty start, which you fixed, and case for which 'size' is empty (e.g. for the last partition) - which cause printing an error as above. The function gpt_verify_partitions() should actually do the same what gpt_fill_pte() does, but it has no sense to duplicate the code. > +int gpt_verify_partitions(block_dev_desc_t *dev_desc, > + disk_partition_t *partitions, int parts, > + gpt_header *gpt_head, gpt_entry **gpt_pte) > +{ > + char efi_str[PARTNAME_SZ + 1]; > + u64 gpt_part_size; > + gpt_entry *gpt_e; > + int ret, i; > + > + ret = gpt_verify_headers(dev_desc, gpt_head, gpt_pte); > + if (ret) > + return ret; > + > + gpt_e = *gpt_pte; > + To make proper verification in this function it would be better if you remove this loop and call function gpt_fill_pte() to fill some temporary 'gpt_e' array from the parsed env $partitions. Then you can compare if the temporary created gpt array entries are equal to the device's gpt entries. Then you don't need to take care about the assumptions to parsing $partitions - just compare two headers - if $partition will change, then you will see the difference in the gpt headers. > + for (i = 0; i < parts; i++) { > + if (i == gpt_head->num_partition_entries) { > + error("More partitions than allowed!\n"); > + return -1; > + } > + > + /* Check if GPT and ENV partition names match */ > + gpt_convert_efi_name_to_char(efi_str, gpt_e[i].partition_name, > + PARTNAME_SZ + 1); > + > + debug("%s: part: %2d name - GPT: %16s, ENV: %16s ", > + __func__, i, efi_str, partitions[i].name); > + > + if (strncmp(efi_str, (char *)partitions[i].name, > + sizeof(partitions->name))) { > + error("Partition name: %s does not match %s!\n", > + efi_str, (char *)partitions[i].name); > + return -1; > + } > + > + /* Check if GPT and ENV sizes match */ > + gpt_part_size = le64_to_cpu(gpt_e[i].ending_lba) - > + le64_to_cpu(gpt_e[i].starting_lba) + 1; > + debug("size(LBA) - GPT: %8llu, ENV: %8llu ", > + gpt_part_size, (u64) partitions[i].size); > + > + if (le64_to_cpu(gpt_part_size) != partitions[i].size) { > + error("Partition %s size: %llu does not match %llu!\n", > + efi_str, gpt_part_size, (u64) partitions[i].size); > + return -1; > + } > + > + /* > + * Start address is optional - check only if provided > + * in '$partition' variable > + */ > + if (!partitions[i].start) { > + debug("\n"); > + continue; > + } > + > + /* Check if GPT and ENV start LBAs match */ > + debug("start LBA - GPT: %8llu, ENV: %8llu\n", > + le64_to_cpu(gpt_e[i].starting_lba), > + (u64) partitions[i].start); > + > + if (le64_to_cpu(gpt_e[i].starting_lba) != partitions[i].start) { > + error("Partition %s start: %llu does not match %llu!\n", > + efi_str, le64_to_cpu(gpt_e[i].starting_lba), > + (u64) partitions[i].start); > + return -1; > + } > + } > + > + return 0; > +} > + > int is_valid_gpt_buf(block_dev_desc_t *dev_desc, void *buf) > { > gpt_header *gpt_h; > diff --git a/include/part.h b/include/part.h > index 8b5ac12..720a867 100644 > --- a/include/part.h > +++ b/include/part.h > @@ -267,6 +267,41 @@ int is_valid_gpt_buf(block_dev_desc_t *dev_desc, void *buf); > * @return - '0' on success, otherwise error > */ > int write_mbr_and_gpt_partitions(block_dev_desc_t *dev_desc, void *buf); > + > +/** > + * gpt_verify_headers() - Function to read and CRC32 check of the GPT's header > + * and partition table entries (PTE) > + * > + * As a side effect if sets gpt_head and gpt_pte so they point to GPT data. > + * > + * @param dev_desc - block device descriptor > + * @param gpt_head - pointer to GPT header data read from medium > + * @param gpt_pte - pointer to GPT partition table enties read from medium > + * > + * @return - '0' on success, otherwise error > + */ > +int gpt_verify_headers(block_dev_desc_t *dev_desc, gpt_header *gpt_head, > + gpt_entry **gpt_pte); > + > +/** > + * gpt_verify_partitions() - Function to check if partitions' name, start and > + * size correspond to '$partitions' env variable > + * > + * This function checks if on medium stored GPT data is in sync with information > + * provided in '$partitions' environment variable. Specificially, name, start > + * and size of the partition is checked. > + * > + * @param dev_desc - block device descriptor > + * @param partitions - partition data read from '$partitions' env variable > + * @param parts - number of partitions read from '$partitions' env variable > + * @param gpt_head - pointer to GPT header data read from medium > + * @param gpt_pte - pointer to GPT partition table enties read from medium > + * > + * @return - '0' on success, otherwise error > + */ > +int gpt_verify_partitions(block_dev_desc_t *dev_desc, > + disk_partition_t *partitions, int parts, > + gpt_header *gpt_head, gpt_entry **gpt_pte); > #endif > > #endif /* _PART_H */ > Reviewed-by: Przemyslaw Marczak <p.marczak@samsung.com> Best regards,
On Fri, Nov 20, 2015 at 08:06:16AM +0100, Lukasz Majewski wrote: > This commit provides definition and declaration of GPT verification > functions - namely gpt_verify_headers() and gpt_verify_partitions(). > The former is used to only check CRC32 of GPT's header and PTEs. > The latter examines each partition entry and compare attributes such as: > name, start offset and size with ones provided at '$partitions' env > variable. > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > Reviewed-by: Tom Rini <trini@konsulko.com> > Reviewed-by: Przemyslaw Marczak <p.marczak@samsung.com> Applied to u-boot/master, thanks!
Hello Tom, On 11/23/2015 11:44 PM, Tom Rini wrote: > On Fri, Nov 20, 2015 at 08:06:16AM +0100, Lukasz Majewski wrote: > >> This commit provides definition and declaration of GPT verification >> functions - namely gpt_verify_headers() and gpt_verify_partitions(). >> The former is used to only check CRC32 of GPT's header and PTEs. >> The latter examines each partition entry and compare attributes such as: >> name, start offset and size with ones provided at '$partitions' env >> variable. >> >> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> >> Reviewed-by: Tom Rini <trini@konsulko.com> >> Reviewed-by: Przemyslaw Marczak <p.marczak@samsung.com> > > Applied to u-boot/master, thanks! > > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Why merged so quickly? I tested this patchset on my device and posted about the issues. [1] This should be reworked, since the verify assumptions are too simple and doesn't fully match the GPT header creation. So this command will fail for some cases of write/verify sequence, depending on what the $partitions includes. [1] https://www.mail-archive.com/u-boot@lists.denx.de/msg193216.html Best regards,
On Tue, Nov 24, 2015 at 10:56:41AM +0100, Przemyslaw Marczak wrote: > Hello Tom, > > On 11/23/2015 11:44 PM, Tom Rini wrote: > >On Fri, Nov 20, 2015 at 08:06:16AM +0100, Lukasz Majewski wrote: > > > >>This commit provides definition and declaration of GPT verification > >>functions - namely gpt_verify_headers() and gpt_verify_partitions(). > >>The former is used to only check CRC32 of GPT's header and PTEs. > >>The latter examines each partition entry and compare attributes such as: > >>name, start offset and size with ones provided at '$partitions' env > >>variable. > >> > >>Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > >>Reviewed-by: Tom Rini <trini@konsulko.com> > >>Reviewed-by: Przemyslaw Marczak <p.marczak@samsung.com> > > > >Applied to u-boot/master, thanks! > > > > > > > >_______________________________________________ > >U-Boot mailing list > >U-Boot@lists.denx.de > >http://lists.denx.de/mailman/listinfo/u-boot > > > > Why merged so quickly? > > I tested this patchset on my device and posted about the issues. [1] > > This should be reworked, since the verify assumptions are too simple > and doesn't fully match the GPT header creation. > So this command will fail for some cases of write/verify sequence, > depending on what the $partitions includes. > > [1] https://www.mail-archive.com/u-boot@lists.denx.de/msg193216.html Mainly because I skimmed things too quickly, sorry. Also in the future (and this applies to anyone that's a custodian, and people can also manage their own patches if they login) please update patches you're asking for changes on in patchwork, it really does help me keep an eye on things. Thanks!
Hi Tom, On 11/24/2015 07:56 PM, Tom Rini wrote: > On Tue, Nov 24, 2015 at 10:56:41AM +0100, Przemyslaw Marczak wrote: >> Hello Tom, >> >> On 11/23/2015 11:44 PM, Tom Rini wrote: >>> On Fri, Nov 20, 2015 at 08:06:16AM +0100, Lukasz Majewski wrote: >>> >>>> This commit provides definition and declaration of GPT verification >>>> functions - namely gpt_verify_headers() and gpt_verify_partitions(). >>>> The former is used to only check CRC32 of GPT's header and PTEs. >>>> The latter examines each partition entry and compare attributes such as: >>>> name, start offset and size with ones provided at '$partitions' env >>>> variable. >>>> >>>> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> >>>> Reviewed-by: Tom Rini <trini@konsulko.com> >>>> Reviewed-by: Przemyslaw Marczak <p.marczak@samsung.com> >>> >>> Applied to u-boot/master, thanks! >>> >>> >>> >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >>> >> >> Why merged so quickly? >> >> I tested this patchset on my device and posted about the issues. [1] >> >> This should be reworked, since the verify assumptions are too simple >> and doesn't fully match the GPT header creation. >> So this command will fail for some cases of write/verify sequence, >> depending on what the $partitions includes. >> >> [1] https://www.mail-archive.com/u-boot@lists.denx.de/msg193216.html > > Mainly because I skimmed things too quickly, sorry. Also in the future > (and this applies to anyone that's a custodian, and people can also > manage their own patches if they login) please update patches you're > asking for changes on in patchwork, it really does help me keep an eye > on things. Thanks! > OK, that's a good point. Best regards,
diff --git a/disk/part_efi.c b/disk/part_efi.c index ea9c615..40f0b36 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -578,6 +578,116 @@ err: return ret; } +static void gpt_convert_efi_name_to_char(char *s, efi_char16_t *es, int n) +{ + char *ess = (char *)es; + int i, j; + + memset(s, '\0', n); + + for (i = 0, j = 0; j < n; i += 2, j++) { + s[j] = ess[i]; + if (!ess[i]) + return; + } +} + +int gpt_verify_headers(block_dev_desc_t *dev_desc, gpt_header *gpt_head, + gpt_entry **gpt_pte) +{ + /* + * This function validates AND + * fills in the GPT header and PTE + */ + if (is_gpt_valid(dev_desc, + GPT_PRIMARY_PARTITION_TABLE_LBA, + gpt_head, gpt_pte) != 1) { + printf("%s: *** ERROR: Invalid GPT ***\n", + __func__); + return -1; + } + if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), + gpt_head, gpt_pte) != 1) { + printf("%s: *** ERROR: Invalid Backup GPT ***\n", + __func__); + return -1; + } + + return 0; +} + +int gpt_verify_partitions(block_dev_desc_t *dev_desc, + disk_partition_t *partitions, int parts, + gpt_header *gpt_head, gpt_entry **gpt_pte) +{ + char efi_str[PARTNAME_SZ + 1]; + u64 gpt_part_size; + gpt_entry *gpt_e; + int ret, i; + + ret = gpt_verify_headers(dev_desc, gpt_head, gpt_pte); + if (ret) + return ret; + + gpt_e = *gpt_pte; + + for (i = 0; i < parts; i++) { + if (i == gpt_head->num_partition_entries) { + error("More partitions than allowed!\n"); + return -1; + } + + /* Check if GPT and ENV partition names match */ + gpt_convert_efi_name_to_char(efi_str, gpt_e[i].partition_name, + PARTNAME_SZ + 1); + + debug("%s: part: %2d name - GPT: %16s, ENV: %16s ", + __func__, i, efi_str, partitions[i].name); + + if (strncmp(efi_str, (char *)partitions[i].name, + sizeof(partitions->name))) { + error("Partition name: %s does not match %s!\n", + efi_str, (char *)partitions[i].name); + return -1; + } + + /* Check if GPT and ENV sizes match */ + gpt_part_size = le64_to_cpu(gpt_e[i].ending_lba) - + le64_to_cpu(gpt_e[i].starting_lba) + 1; + debug("size(LBA) - GPT: %8llu, ENV: %8llu ", + gpt_part_size, (u64) partitions[i].size); + + if (le64_to_cpu(gpt_part_size) != partitions[i].size) { + error("Partition %s size: %llu does not match %llu!\n", + efi_str, gpt_part_size, (u64) partitions[i].size); + return -1; + } + + /* + * Start address is optional - check only if provided + * in '$partition' variable + */ + if (!partitions[i].start) { + debug("\n"); + continue; + } + + /* Check if GPT and ENV start LBAs match */ + debug("start LBA - GPT: %8llu, ENV: %8llu\n", + le64_to_cpu(gpt_e[i].starting_lba), + (u64) partitions[i].start); + + if (le64_to_cpu(gpt_e[i].starting_lba) != partitions[i].start) { + error("Partition %s start: %llu does not match %llu!\n", + efi_str, le64_to_cpu(gpt_e[i].starting_lba), + (u64) partitions[i].start); + return -1; + } + } + + return 0; +} + int is_valid_gpt_buf(block_dev_desc_t *dev_desc, void *buf) { gpt_header *gpt_h; diff --git a/include/part.h b/include/part.h index 8b5ac12..720a867 100644 --- a/include/part.h +++ b/include/part.h @@ -267,6 +267,41 @@ int is_valid_gpt_buf(block_dev_desc_t *dev_desc, void *buf); * @return - '0' on success, otherwise error */ int write_mbr_and_gpt_partitions(block_dev_desc_t *dev_desc, void *buf); + +/** + * gpt_verify_headers() - Function to read and CRC32 check of the GPT's header + * and partition table entries (PTE) + * + * As a side effect if sets gpt_head and gpt_pte so they point to GPT data. + * + * @param dev_desc - block device descriptor + * @param gpt_head - pointer to GPT header data read from medium + * @param gpt_pte - pointer to GPT partition table enties read from medium + * + * @return - '0' on success, otherwise error + */ +int gpt_verify_headers(block_dev_desc_t *dev_desc, gpt_header *gpt_head, + gpt_entry **gpt_pte); + +/** + * gpt_verify_partitions() - Function to check if partitions' name, start and + * size correspond to '$partitions' env variable + * + * This function checks if on medium stored GPT data is in sync with information + * provided in '$partitions' environment variable. Specificially, name, start + * and size of the partition is checked. + * + * @param dev_desc - block device descriptor + * @param partitions - partition data read from '$partitions' env variable + * @param parts - number of partitions read from '$partitions' env variable + * @param gpt_head - pointer to GPT header data read from medium + * @param gpt_pte - pointer to GPT partition table enties read from medium + * + * @return - '0' on success, otherwise error + */ +int gpt_verify_partitions(block_dev_desc_t *dev_desc, + disk_partition_t *partitions, int parts, + gpt_header *gpt_head, gpt_entry **gpt_pte); #endif #endif /* _PART_H */