Re: [U-Boot] [PATCH v2 4/5] gpt: part: Definition and declaration of GPT verification functions

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,