| Message ID | 20220119185548.16730-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>: > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > +{ > + int ret; > + struct blk_desc *desc; > + u16 primary_mpart = 0, secondary_mpart = 0; > + I think this update_mdata() method (or fwu_update_mdata() wrapper) should always update mdata::crc32. calculate crc32 at each call site is inefficient and easy to introduce bugs. > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + /* First write the primary partition*/ > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > + if (ret < 0) { > + log_err("Updating primary FWU metadata partition failed\n"); > + return ret; > + } > + > + /* And now the replica */ > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > + if (ret < 0) { > + log_err("Updating secondary FWU metadata partition failed\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int gpt_get_mdata(struct fwu_mdata **mdata) > +{ > + int ret; > + struct blk_desc *desc; > + u16 primary_mpart = 0, secondary_mpart = 0; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + *mdata = malloc(sizeof(struct fwu_mdata)); > + if (!*mdata) { > + log_err("Unable to allocate memory for reading FWU metadata\n"); > + return -ENOMEM; > + } > + > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); Also, please release mdata inside the gpt_get_mdata() itself. I think it is not a good design to ask caller to free mdata if get_mdata() operation is failed because mdata may or may not allocated in error case. In success case, user must free it because it is allocated (user accessed it), and in error case, user can ignore it because it should not be allocated. This is simpler mind model and less memory leak chance. Thank you,
On 1/19/22 19:55, Sughosh Ganu wrote: > In the FWU Multi Bank Update feature, the information about the > updatable images is stored as part of the metadata, on a separate > partition. Add functions for reading from and writing to the metadata > when the updatable images and the metadata are stored on a block > device which is formated with GPT based partition scheme. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> Will a GPT partition remain the only place to store that information? Should this be implemented according to the dirver model? > --- > > Changes since V2: > * Move the function definition of fwu_verify_mdata to fwu_mdata.c to > facilitate reuse > * Remove the block device specific desc->devnum parameter for the > fwu_plat_get_alt_num function call > > include/fwu.h | 1 + > include/fwu_mdata.h | 2 + > lib/fwu_updates/fwu_mdata.c | 29 ++ > lib/fwu_updates/fwu_mdata_gpt_blk.c | 520 ++++++++++++++++++++++++++++ > 4 files changed, 552 insertions(+) > create mode 100644 lib/fwu_updates/fwu_mdata_gpt_blk.c > > diff --git a/include/fwu.h b/include/fwu.h > index acba725bc8..12f7eecdb0 100644 > --- a/include/fwu.h > +++ b/include/fwu.h > @@ -53,6 +53,7 @@ int fwu_get_active_index(u32 *active_idx); > int fwu_update_active_index(u32 active_idx); > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > int *alt_num); > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); > int fwu_mdata_check(void); > int fwu_revert_boot_index(void); > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > index d788eb69e7..53e39f9af6 100644 > --- a/include/fwu_mdata.h > +++ b/include/fwu_mdata.h > @@ -64,4 +64,6 @@ struct fwu_mdata { > struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > } __attribute__((__packed__)); > > +extern struct fwu_mdata_ops fwu_gpt_blk_ops; > + > #endif /* _FWU_MDATA_H_ */ > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c > index 58e838fe28..252fcf50f6 100644 > --- a/lib/fwu_updates/fwu_mdata.c > +++ b/lib/fwu_updates/fwu_mdata.c > @@ -25,6 +25,35 @@ static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > return ops; > } > > +/** > + * fwu_verify_mdata() - Verify the FWU metadata > + * @mdata: FWU metadata structure > + * @pri_part: FWU metadata partition is primary or secondary > + * > + * Verify the FWU metadata by computing the CRC32 for the metadata > + * structure and comparing it against the CRC32 value stored as part > + * of the structure. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) > +{ > + u32 calc_crc32; > + void *buf; > + > + buf = &mdata->version; > + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); CRC32 does not offer any security against manipulation. What are the security implications? Best regards Heinrich > + > + if (calc_crc32 != mdata->crc32) { > + log_err("crc32 check failed for %s FWU metadata partition\n", > + pri_part ? "primary" : "secondary"); > + return -1; > + } > + > + return 0; > +} > + > /** > * fwu_get_active_index() - Get active_index from the FWU metadata > * @active_idx: active_index value to be read > diff --git a/lib/fwu_updates/fwu_mdata_gpt_blk.c b/lib/fwu_updates/fwu_mdata_gpt_blk.c > new file mode 100644 > index 0000000000..cb47ddf4a7 > --- /dev/null > +++ b/lib/fwu_updates/fwu_mdata_gpt_blk.c > @@ -0,0 +1,520 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2021, Linaro Limited > + */ > + > +#include <blk.h> > +#include <efi_loader.h> > +#include <fwu.h> > +#include <fwu_mdata.h> > +#include <log.h> > +#include <malloc.h> > +#include <memalign.h> > +#include <part.h> > +#include <part_efi.h> > + > +#include <linux/errno.h> > +#include <linux/types.h> > +#include <u-boot/crc.h> > + > +#define PRIMARY_PART BIT(0) > +#define SECONDARY_PART BIT(1) > +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART) > + > +#define MDATA_READ BIT(0) > +#define MDATA_WRITE BIT(1) > + > +#define IMAGE_ACCEPT_SET BIT(0) > +#define IMAGE_ACCEPT_CLEAR BIT(1) > + > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > + u16 *primary_mpart, > + u16 *secondary_mpart) > +{ > + int i, ret; > + u32 mdata_parts; > + efi_guid_t part_type_guid; > + struct disk_partition info; > + const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID; > + > + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { > + if (part_get_info(desc, i, &info)) > + continue; > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > + UUID_STR_FORMAT_GUID); > + > + if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { > + ++mdata_parts; > + if (!*primary_mpart) > + *primary_mpart = i; > + else > + *secondary_mpart = i; > + } > + } > + > + if (mdata_parts != 2) { > + log_err("Expect two copies of the FWU metadata instead of %d\n", > + mdata_parts); > + ret = -EINVAL; > + } else { > + ret = 0; > + } > + > + return ret; > +} > + > +static int gpt_get_mdata_disk_part(struct blk_desc *desc, > + struct disk_partition *info, > + u32 part_num) > +{ > + int ret; > + char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > + > + ret = part_get_info(desc, part_num, info); > + if (ret < 0) { > + log_err("Unable to get the partition info for the FWU metadata part %d", > + part_num); > + return -1; > + } > + > + /* Check that it is indeed the FWU metadata partition */ > + if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) { > + /* Found the FWU metadata partition */ > + return 0; > + } > + > + return -1; > +} > + > +static int gpt_read_write_mdata(struct blk_desc *desc, > + struct fwu_mdata *mdata, > + u8 access, u32 part_num) > +{ > + int ret; > + u32 len, blk_start, blkcnt; > + struct disk_partition info; > + > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1, > + desc->blksz); > + > + ret = gpt_get_mdata_disk_part(desc, &info, part_num); > + if (ret < 0) { > + printf("Unable to get the FWU metadata partition\n"); > + return -ENODEV; > + } > + > + len = sizeof(*mdata); > + blkcnt = BLOCK_CNT(len, desc); > + if (blkcnt > info.size) { > + log_err("Block count exceeds FWU metadata partition size\n"); > + return -ERANGE; > + } > + > + blk_start = info.start; > + if (access == MDATA_READ) { > + if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) { > + log_err("Error reading FWU metadata from the device\n"); > + return -EIO; > + } > + memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata)); > + } else { > + if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) { > + log_err("Error writing FWU metadata to the device\n"); > + return -EIO; > + } > + } > + > + return 0; > +} > + > +static int gpt_read_mdata(struct blk_desc *desc, > + struct fwu_mdata *mdata, u32 part_num) > +{ > + return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num); > +} > + > +static int gpt_write_mdata_partition(struct blk_desc *desc, > + struct fwu_mdata *mdata, > + u32 part_num) > +{ > + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num); > +} > + > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > +{ > + int ret; > + struct blk_desc *desc; > + u16 primary_mpart = 0, secondary_mpart = 0; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + /* First write the primary partition*/ > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > + if (ret < 0) { > + log_err("Updating primary FWU metadata partition failed\n"); > + return ret; > + } > + > + /* And now the replica */ > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > + if (ret < 0) { > + log_err("Updating secondary FWU metadata partition failed\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int gpt_get_mdata(struct fwu_mdata **mdata) > +{ > + int ret; > + struct blk_desc *desc; > + u16 primary_mpart = 0, secondary_mpart = 0; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + *mdata = malloc(sizeof(struct fwu_mdata)); > + if (!*mdata) { > + log_err("Unable to allocate memory for reading FWU metadata\n"); > + return -ENOMEM; > + } > + > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + return -EIO; > + } > + > + ret = fwu_verify_mdata(*mdata, 1); > + if (!ret) > + return 0; > + > + /* > + * Verification of the primary FWU metadata copy failed. > + * Try to read the replica. > + */ > + memset(*mdata, 0, sizeof(struct fwu_mdata)); > + ret = gpt_read_mdata(desc, *mdata, secondary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + return -EIO; > + } > + > + ret = fwu_verify_mdata(*mdata, 0); > + if (!ret) > + return 0; > + > + /* Both the FWU metadata copies are corrupted. */ > + return -1; > +} > + > +static int gpt_check_mdata_validity(void) > +{ > + int ret; > + struct blk_desc *desc; > + struct fwu_mdata pri_mdata; > + struct fwu_mdata secondary_mdata; > + u16 primary_mpart = 0, secondary_mpart = 0; > + u16 valid_partitions, invalid_partitions; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + /* > + * Two FWU metadata partitions are expected. > + * If we don't have two, user needs to create > + * them first > + */ > + valid_partitions = 0; > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the FWU metadata partitions\n"); > + return -ENODEV; > + } > + > + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + goto secondary_read; > + } > + > + ret = fwu_verify_mdata(&pri_mdata, 1); > + if (!ret) > + valid_partitions |= PRIMARY_PART; > + > +secondary_read: > + /* Now check the secondary partition */ > + ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart); > + if (ret < 0) { > + log_err("Failed to read the FWU metadata from the device\n"); > + goto mdata_restore; > + } > + > + ret = fwu_verify_mdata(&secondary_mdata, 0); > + if (!ret) > + valid_partitions |= SECONDARY_PART; > + > +mdata_restore: > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > + ret = -1; > + /* > + * Before returning, check that both the > + * FWU metadata copies are the same. If not, > + * the FWU metadata copies need to be > + * re-populated. > + */ > + if (!memcmp(&pri_mdata, &secondary_mdata, > + sizeof(struct fwu_mdata))) { > + ret = 0; > + } else { > + log_err("Both FWU metadata copies are valid but do not match. Please check!\n"); > + } > + goto out; > + } > + > + ret = -1; > + if (!(valid_partitions & BOTH_PARTS)) > + goto out; > + > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > + ret = gpt_write_mdata_partition(desc, > + (invalid_partitions == PRIMARY_PART) ? > + &secondary_mdata : &pri_mdata, > + (invalid_partitions == PRIMARY_PART) ? > + primary_mpart : secondary_mpart); > + > + if (ret < 0) > + log_err("Restoring %s FWU metadata partition failed\n", > + (invalid_partitions == PRIMARY_PART) ? > + "primary" : "secondary"); > + > +out: > + return ret; > +} > + > +int fwu_gpt_get_active_index(u32 *active_idx) > +{ > + int ret; > + struct fwu_mdata *mdata; > + > + ret = gpt_get_mdata(&mdata); > + if (ret < 0) { > + log_err("Unable to get valid FWU metadata\n"); > + goto out; > + } > + > + /* > + * Found the FWU metadata partition, now read the active_index > + * value > + */ > + *active_idx = mdata->active_index; > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > + printf("Active index value read is incorrect\n"); > + ret = -EINVAL; > + goto out; > + } > + > +out: > + free(mdata); > + > + return ret; > +} > + > +static int gpt_get_image_alt_num(struct blk_desc *desc, > + efi_guid_t image_type_id, > + u32 update_bank, int *alt_no) > +{ > + int ret, i; > + u32 part; > + struct fwu_mdata *mdata; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + struct disk_partition info; > + efi_guid_t unique_part_guid; > + efi_guid_t image_guid = NULL_GUID; > + > + ret = gpt_get_mdata(&mdata); > + if (ret < 0) { > + log_err("Unable to read valid FWU metadata\n"); > + goto out; > + } > + > + /* > + * The FWU metadata has been read. Now get the image_uuid for the > + * image with the update_bank. > + */ > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > + if (!guidcmp(&image_type_id, > + &mdata->img_entry[i].image_type_uuid)) { > + img_entry = &mdata->img_entry[i]; > + img_bank_info = &img_entry->img_bank_info[update_bank]; > + guidcpy(&image_guid, &img_bank_info->image_uuid); > + break; > + } > + } > + > + /* > + * Now read the GPT Partition Table Entries to find a matching > + * partition with UniquePartitionGuid value. We need to iterate > + * through all the GPT partitions since they might be in any > + * order > + */ > + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { > + if (part_get_info(desc, i, &info)) > + continue; > + uuid_str_to_bin(info.uuid, unique_part_guid.b, > + UUID_STR_FORMAT_GUID); > + > + if (!guidcmp(&unique_part_guid, &image_guid)) { > + /* Found the partition */ > + part = i; > + *alt_no = fwu_plat_get_alt_num(&part); > + if (*alt_no != -1) > + log_info("alt_num %d for partition %pUl\n", > + *alt_no, &image_guid); > + ret = 0; > + break; > + } > + } > + > + if (*alt_no == -1) { > + log_err("alt_num not found for partition with GUID %pUl\n", > + &image_guid); > + ret = -EINVAL; > + } > + > + if (i == MAX_SEARCH_PARTITIONS) { > + log_err("Partition with the image guid not found\n"); > + ret = -EINVAL; > + } > + > +out: > + free(mdata); > + > + return ret; > +} > + > +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > + int *alt_no) > +{ > + int ret; > + struct blk_desc *desc; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + return gpt_get_image_alt_num(desc, image_type_id, update_bank, alt_no); > +} > + > +int fwu_gpt_mdata_check(void) > +{ > + /* > + * Check if both the copies of the FWU metadata are > + * valid. If one has gone bad, restore it from the > + * other good copy. > + */ > + return gpt_check_mdata_validity(); > +} > + > +int fwu_gpt_get_mdata(struct fwu_mdata **mdata) > +{ > + return gpt_get_mdata(mdata); > +} > + > +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, > + u32 bank, u8 action) > +{ > + void *buf; > + int ret, i; > + u32 nimages; > + struct fwu_mdata *mdata; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + > + ret = gpt_get_mdata(&mdata); > + if (ret < 0) { > + log_err("Unable to get valid FWU metadata\n"); > + goto out; > + } > + > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > + img_entry = &mdata->img_entry[0]; > + for (i = 0; i < nimages; i++) { > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > + img_bank_info = &img_entry[i].img_bank_info[bank]; > + if (action == IMAGE_ACCEPT_SET) > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > + else > + img_bank_info->accepted = 0; > + > + buf = &mdata->version; > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - > + sizeof(u32)); > + > + ret = fwu_gpt_update_mdata(mdata); > + goto out; > + } > + } > + > + /* Image not found */ > + ret = -EINVAL; > + > +out: > + free(mdata); > + > + return ret; > +} > + > +static int fwu_gpt_accept_image(efi_guid_t *img_type_id, u32 bank) > +{ > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > + IMAGE_ACCEPT_SET); > +} > + > +static int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > +{ > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > + IMAGE_ACCEPT_CLEAR); > +} > + > +struct fwu_mdata_ops fwu_gpt_blk_ops = { > + .get_active_index = fwu_gpt_get_active_index, > + .get_image_alt_num = fwu_gpt_get_image_alt_num, > + .mdata_check = fwu_gpt_mdata_check, > + .set_accept_image = fwu_gpt_accept_image, > + .clear_accept_image = fwu_gpt_clear_accept_image, > + .get_mdata = fwu_gpt_get_mdata, > + .update_mdata = fwu_gpt_update_mdata, > +};
On Thu, 20 Jan 2022 at 16:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/19/22 19:55, Sughosh Ganu wrote: > > In the FWU Multi Bank Update feature, the information about the > > updatable images is stored as part of the metadata, on a separate > > partition. Add functions for reading from and writing to the metadata > > when the updatable images and the metadata are stored on a block > > device which is formated with GPT based partition scheme. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > Will a GPT partition remain the only place to store that information? > Should this be implemented according to the dirver model? Even the current implementation provides a fwu_mdata_ops structure which contains function pointers for accessing the metadata. So there can be multiple implementations of the access methods. I have implemented the access methods for GPT partitioned block devices. But it should not be very difficult to move the metadata access to driver model. Let me know if you prefer that instead of the current implementation. > > > --- > > > > Changes since V2: > > * Move the function definition of fwu_verify_mdata to fwu_mdata.c to > > facilitate reuse > > * Remove the block device specific desc->devnum parameter for the > > fwu_plat_get_alt_num function call > > > > include/fwu.h | 1 + > > include/fwu_mdata.h | 2 + > > lib/fwu_updates/fwu_mdata.c | 29 ++ > > lib/fwu_updates/fwu_mdata_gpt_blk.c | 520 ++++++++++++++++++++++++++++ > > 4 files changed, 552 insertions(+) > > create mode 100644 lib/fwu_updates/fwu_mdata_gpt_blk.c > > > > diff --git a/include/fwu.h b/include/fwu.h > > index acba725bc8..12f7eecdb0 100644 > > --- a/include/fwu.h > > +++ b/include/fwu.h > > @@ -53,6 +53,7 @@ int fwu_get_active_index(u32 *active_idx); > > int fwu_update_active_index(u32 active_idx); > > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > int *alt_num); > > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); > > int fwu_mdata_check(void); > > int fwu_revert_boot_index(void); > > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > index d788eb69e7..53e39f9af6 100644 > > --- a/include/fwu_mdata.h > > +++ b/include/fwu_mdata.h > > @@ -64,4 +64,6 @@ struct fwu_mdata { > > struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > } __attribute__((__packed__)); > > > > +extern struct fwu_mdata_ops fwu_gpt_blk_ops; > > + > > #endif /* _FWU_MDATA_H_ */ > > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c > > index 58e838fe28..252fcf50f6 100644 > > --- a/lib/fwu_updates/fwu_mdata.c > > +++ b/lib/fwu_updates/fwu_mdata.c > > @@ -25,6 +25,35 @@ static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > return ops; > > } > > > > +/** > > + * fwu_verify_mdata() - Verify the FWU metadata > > + * @mdata: FWU metadata structure > > + * @pri_part: FWU metadata partition is primary or secondary > > + * > > + * Verify the FWU metadata by computing the CRC32 for the metadata > > + * structure and comparing it against the CRC32 value stored as part > > + * of the structure. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) > > +{ > > + u32 calc_crc32; > > + void *buf; > > + > > + buf = &mdata->version; > > + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > > CRC32 does not offer any security against manipulation. > What are the security implications? The CRC32 value I believe is used only for integrity check of the metadata against any kind of bitflip type errors of the hardware device which stores the metadata. The metadata itself is stored on a non-secure storage medium. -sughosh > > Best regards > > Heinrich > > > + > > + if (calc_crc32 != mdata->crc32) { > > + log_err("crc32 check failed for %s FWU metadata partition\n", > > + pri_part ? "primary" : "secondary"); > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > /** > > * fwu_get_active_index() - Get active_index from the FWU metadata > > * @active_idx: active_index value to be read > > diff --git a/lib/fwu_updates/fwu_mdata_gpt_blk.c b/lib/fwu_updates/fwu_mdata_gpt_blk.c > > new file mode 100644 > > index 0000000000..cb47ddf4a7 > > --- /dev/null > > +++ b/lib/fwu_updates/fwu_mdata_gpt_blk.c > > @@ -0,0 +1,520 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2021, Linaro Limited > > + */ > > + > > +#include <blk.h> > > +#include <efi_loader.h> > > +#include <fwu.h> > > +#include <fwu_mdata.h> > > +#include <log.h> > > +#include <malloc.h> > > +#include <memalign.h> > > +#include <part.h> > > +#include <part_efi.h> > > + > > +#include <linux/errno.h> > > +#include <linux/types.h> > > +#include <u-boot/crc.h> > > + > > +#define PRIMARY_PART BIT(0) > > +#define SECONDARY_PART BIT(1) > > +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART) > > + > > +#define MDATA_READ BIT(0) > > +#define MDATA_WRITE BIT(1) > > + > > +#define IMAGE_ACCEPT_SET BIT(0) > > +#define IMAGE_ACCEPT_CLEAR BIT(1) > > + > > +static int gpt_get_mdata_partitions(struct blk_desc *desc, > > + u16 *primary_mpart, > > + u16 *secondary_mpart) > > +{ > > + int i, ret; > > + u32 mdata_parts; > > + efi_guid_t part_type_guid; > > + struct disk_partition info; > > + const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID; > > + > > + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { > > + if (part_get_info(desc, i, &info)) > > + continue; > > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > > + UUID_STR_FORMAT_GUID); > > + > > + if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { > > + ++mdata_parts; > > + if (!*primary_mpart) > > + *primary_mpart = i; > > + else > > + *secondary_mpart = i; > > + } > > + } > > + > > + if (mdata_parts != 2) { > > + log_err("Expect two copies of the FWU metadata instead of %d\n", > > + mdata_parts); > > + ret = -EINVAL; > > + } else { > > + ret = 0; > > + } > > + > > + return ret; > > +} > > + > > +static int gpt_get_mdata_disk_part(struct blk_desc *desc, > > + struct disk_partition *info, > > + u32 part_num) > > +{ > > + int ret; > > + char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > > + > > + ret = part_get_info(desc, part_num, info); > > + if (ret < 0) { > > + log_err("Unable to get the partition info for the FWU metadata part %d", > > + part_num); > > + return -1; > > + } > > + > > + /* Check that it is indeed the FWU metadata partition */ > > + if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) { > > + /* Found the FWU metadata partition */ > > + return 0; > > + } > > + > > + return -1; > > +} > > + > > +static int gpt_read_write_mdata(struct blk_desc *desc, > > + struct fwu_mdata *mdata, > > + u8 access, u32 part_num) > > +{ > > + int ret; > > + u32 len, blk_start, blkcnt; > > + struct disk_partition info; > > + > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1, > > + desc->blksz); > > + > > + ret = gpt_get_mdata_disk_part(desc, &info, part_num); > > + if (ret < 0) { > > + printf("Unable to get the FWU metadata partition\n"); > > + return -ENODEV; > > + } > > + > > + len = sizeof(*mdata); > > + blkcnt = BLOCK_CNT(len, desc); > > + if (blkcnt > info.size) { > > + log_err("Block count exceeds FWU metadata partition size\n"); > > + return -ERANGE; > > + } > > + > > + blk_start = info.start; > > + if (access == MDATA_READ) { > > + if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) { > > + log_err("Error reading FWU metadata from the device\n"); > > + return -EIO; > > + } > > + memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata)); > > + } else { > > + if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) { > > + log_err("Error writing FWU metadata to the device\n"); > > + return -EIO; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int gpt_read_mdata(struct blk_desc *desc, > > + struct fwu_mdata *mdata, u32 part_num) > > +{ > > + return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num); > > +} > > + > > +static int gpt_write_mdata_partition(struct blk_desc *desc, > > + struct fwu_mdata *mdata, > > + u32 part_num) > > +{ > > + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num); > > +} > > + > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + /* First write the primary partition*/ > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Updating primary FWU metadata partition failed\n"); > > + return ret; > > + } > > + > > + /* And now the replica */ > > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Updating secondary FWU metadata partition failed\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int gpt_get_mdata(struct fwu_mdata **mdata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > + if (!*mdata) { > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > + return -ENOMEM; > > + } > > + > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + return -EIO; > > + } > > + > > + ret = fwu_verify_mdata(*mdata, 1); > > + if (!ret) > > + return 0; > > + > > + /* > > + * Verification of the primary FWU metadata copy failed. > > + * Try to read the replica. > > + */ > > + memset(*mdata, 0, sizeof(struct fwu_mdata)); > > + ret = gpt_read_mdata(desc, *mdata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + return -EIO; > > + } > > + > > + ret = fwu_verify_mdata(*mdata, 0); > > + if (!ret) > > + return 0; > > + > > + /* Both the FWU metadata copies are corrupted. */ > > + return -1; > > +} > > + > > +static int gpt_check_mdata_validity(void) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + struct fwu_mdata pri_mdata; > > + struct fwu_mdata secondary_mdata; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + u16 valid_partitions, invalid_partitions; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + /* > > + * Two FWU metadata partitions are expected. > > + * If we don't have two, user needs to create > > + * them first > > + */ > > + valid_partitions = 0; > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + goto secondary_read; > > + } > > + > > + ret = fwu_verify_mdata(&pri_mdata, 1); > > + if (!ret) > > + valid_partitions |= PRIMARY_PART; > > + > > +secondary_read: > > + /* Now check the secondary partition */ > > + ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > + goto mdata_restore; > > + } > > + > > + ret = fwu_verify_mdata(&secondary_mdata, 0); > > + if (!ret) > > + valid_partitions |= SECONDARY_PART; > > + > > +mdata_restore: > > + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { > > + ret = -1; > > + /* > > + * Before returning, check that both the > > + * FWU metadata copies are the same. If not, > > + * the FWU metadata copies need to be > > + * re-populated. > > + */ > > + if (!memcmp(&pri_mdata, &secondary_mdata, > > + sizeof(struct fwu_mdata))) { > > + ret = 0; > > + } else { > > + log_err("Both FWU metadata copies are valid but do not match. Please check!\n"); > > + } > > + goto out; > > + } > > + > > + ret = -1; > > + if (!(valid_partitions & BOTH_PARTS)) > > + goto out; > > + > > + invalid_partitions = valid_partitions ^ BOTH_PARTS; > > + ret = gpt_write_mdata_partition(desc, > > + (invalid_partitions == PRIMARY_PART) ? > > + &secondary_mdata : &pri_mdata, > > + (invalid_partitions == PRIMARY_PART) ? > > + primary_mpart : secondary_mpart); > > + > > + if (ret < 0) > > + log_err("Restoring %s FWU metadata partition failed\n", > > + (invalid_partitions == PRIMARY_PART) ? > > + "primary" : "secondary"); > > + > > +out: > > + return ret; > > +} > > + > > +int fwu_gpt_get_active_index(u32 *active_idx) > > +{ > > + int ret; > > + struct fwu_mdata *mdata; > > + > > + ret = gpt_get_mdata(&mdata); > > + if (ret < 0) { > > + log_err("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * Found the FWU metadata partition, now read the active_index > > + * value > > + */ > > + *active_idx = mdata->active_index; > > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > > + printf("Active index value read is incorrect\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > +out: > > + free(mdata); > > + > > + return ret; > > +} > > + > > +static int gpt_get_image_alt_num(struct blk_desc *desc, > > + efi_guid_t image_type_id, > > + u32 update_bank, int *alt_no) > > +{ > > + int ret, i; > > + u32 part; > > + struct fwu_mdata *mdata; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + struct disk_partition info; > > + efi_guid_t unique_part_guid; > > + efi_guid_t image_guid = NULL_GUID; > > + > > + ret = gpt_get_mdata(&mdata); > > + if (ret < 0) { > > + log_err("Unable to read valid FWU metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * The FWU metadata has been read. Now get the image_uuid for the > > + * image with the update_bank. > > + */ > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&image_type_id, > > + &mdata->img_entry[i].image_type_uuid)) { > > + img_entry = &mdata->img_entry[i]; > > + img_bank_info = &img_entry->img_bank_info[update_bank]; > > + guidcpy(&image_guid, &img_bank_info->image_uuid); > > + break; > > + } > > + } > > + > > + /* > > + * Now read the GPT Partition Table Entries to find a matching > > + * partition with UniquePartitionGuid value. We need to iterate > > + * through all the GPT partitions since they might be in any > > + * order > > + */ > > + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { > > + if (part_get_info(desc, i, &info)) > > + continue; > > + uuid_str_to_bin(info.uuid, unique_part_guid.b, > > + UUID_STR_FORMAT_GUID); > > + > > + if (!guidcmp(&unique_part_guid, &image_guid)) { > > + /* Found the partition */ > > + part = i; > > + *alt_no = fwu_plat_get_alt_num(&part); > > + if (*alt_no != -1) > > + log_info("alt_num %d for partition %pUl\n", > > + *alt_no, &image_guid); > > + ret = 0; > > + break; > > + } > > + } > > + > > + if (*alt_no == -1) { > > + log_err("alt_num not found for partition with GUID %pUl\n", > > + &image_guid); > > + ret = -EINVAL; > > + } > > + > > + if (i == MAX_SEARCH_PARTITIONS) { > > + log_err("Partition with the image guid not found\n"); > > + ret = -EINVAL; > > + } > > + > > +out: > > + free(mdata); > > + > > + return ret; > > +} > > + > > +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > + int *alt_no) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + return gpt_get_image_alt_num(desc, image_type_id, update_bank, alt_no); > > +} > > + > > +int fwu_gpt_mdata_check(void) > > +{ > > + /* > > + * Check if both the copies of the FWU metadata are > > + * valid. If one has gone bad, restore it from the > > + * other good copy. > > + */ > > + return gpt_check_mdata_validity(); > > +} > > + > > +int fwu_gpt_get_mdata(struct fwu_mdata **mdata) > > +{ > > + return gpt_get_mdata(mdata); > > +} > > + > > +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, > > + u32 bank, u8 action) > > +{ > > + void *buf; > > + int ret, i; > > + u32 nimages; > > + struct fwu_mdata *mdata; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = gpt_get_mdata(&mdata); > > + if (ret < 0) { > > + log_err("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > + img_entry = &mdata->img_entry[0]; > > + for (i = 0; i < nimages; i++) { > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > + if (action == IMAGE_ACCEPT_SET) > > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; > > + else > > + img_bank_info->accepted = 0; > > + > > + buf = &mdata->version; > > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - > > + sizeof(u32)); > > + > > + ret = fwu_gpt_update_mdata(mdata); > > + goto out; > > + } > > + } > > + > > + /* Image not found */ > > + ret = -EINVAL; > > + > > +out: > > + free(mdata); > > + > > + return ret; > > +} > > + > > +static int fwu_gpt_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_SET); > > +} > > + > > +static int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_CLEAR); > > +} > > + > > +struct fwu_mdata_ops fwu_gpt_blk_ops = { > > + .get_active_index = fwu_gpt_get_active_index, > > + .get_image_alt_num = fwu_gpt_get_image_alt_num, > > + .mdata_check = fwu_gpt_mdata_check, > > + .set_accept_image = fwu_gpt_accept_image, > > + .clear_accept_image = fwu_gpt_clear_accept_image, > > + .get_mdata = fwu_gpt_get_mdata, > > + .update_mdata = fwu_gpt_update_mdata, > > +}; >
hi Masami, On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Sughosh, > > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + > > I think this update_mdata() method (or fwu_update_mdata() wrapper) > should always update mdata::crc32. calculate crc32 at each call site is > inefficient and easy to introduce bugs. Okay. Will do. > > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + /* First write the primary partition*/ > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Updating primary FWU metadata partition failed\n"); > > + return ret; > > + } > > + > > + /* And now the replica */ > > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Updating secondary FWU metadata partition failed\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int gpt_get_mdata(struct fwu_mdata **mdata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u16 primary_mpart = 0, secondary_mpart = 0; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the FWU metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > + if (!*mdata) { > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > + return -ENOMEM; > > + } > > + > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the FWU metadata from the device\n"); > > Also, please release mdata inside the gpt_get_mdata() itself. > > I think it is not a good design to ask caller to free mdata if get_mdata() > operation is failed because mdata may or may not allocated in error case. > > In success case, user must free it because it is allocated (user accessed it), > and in error case, user can ignore it because it should not be allocated. > This is simpler mind model and less memory leak chance. I think this is confusing. It would be better to be consistent and have the caller free up the allocated/not allocated memory. If you check, the mdata pointer is being initialised to NULL in every instance. Calling free with a NULL pointer is a valid case, which the free call handles. There are multiple places in u-boot where the caller is freeing up the allocated memory. So i think this should not be an issue. -sughosh > > Thank you, > -- > Masami Hiramatsu
Hi Sughosh, 2022年1月24日(月) 15:58 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > Hi Sughosh, > > > > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > > I think this update_mdata() method (or fwu_update_mdata() wrapper) > > should always update mdata::crc32. calculate crc32 at each call site is > > inefficient and easy to introduce bugs. > > Okay. Will do. > > > > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + /* First write the primary partition*/ > > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating primary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + /* And now the replica */ > > > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating secondary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_get_mdata(struct fwu_mdata **mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > > + if (!*mdata) { > > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the FWU metadata from the device\n"); > > > > Also, please release mdata inside the gpt_get_mdata() itself. > > > > I think it is not a good design to ask caller to free mdata if get_mdata() > > operation is failed because mdata may or may not allocated in error case. > > > > In success case, user must free it because it is allocated (user accessed it), > > and in error case, user can ignore it because it should not be allocated. > > This is simpler mind model and less memory leak chance. > > I think this is confusing. It would be better to be consistent and > have the caller free up the allocated/not allocated memory. If you > check, the mdata pointer is being initialised to NULL in every > instance. Calling free with a NULL pointer is a valid case, which the > free call handles. There are multiple places in u-boot where the > caller is freeing up the allocated memory. So i think this should not > be an issue. Of course it is sane. What I was that is easier to introduce mistakes. If it requires the caller to prepare mdata = NULL as an input, it easily causes memory leak or other issues, because it seems not an input but just an output parameter. My concern is that this method is not a local one, but it is directly exported via fwu_mdata_opts. That is a problem because this means other developers can use it. And also, it forces the other backend driver (like my fwu_mdata_sf.c) to do so. Thank you,
On Mon, Jan 24, 2022 at 12:28:24PM +0530, Sughosh Ganu wrote: > hi Masami, > > On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > Hi Sughosh, > > > > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > > I think this update_mdata() method (or fwu_update_mdata() wrapper) > > should always update mdata::crc32. calculate crc32 at each call site is > > inefficient and easy to introduce bugs. > > Okay. Will do. > > > > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + /* First write the primary partition*/ > > > + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating primary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + /* And now the replica */ > > > + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating secondary FWU metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_get_mdata(struct fwu_mdata **mdata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u16 primary_mpart = 0, secondary_mpart = 0; > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = gpt_get_mdata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the FWU metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + *mdata = malloc(sizeof(struct fwu_mdata)); > > > + if (!*mdata) { > > > + log_err("Unable to allocate memory for reading FWU metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = gpt_read_mdata(desc, *mdata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the FWU metadata from the device\n"); > > > > Also, please release mdata inside the gpt_get_mdata() itself. > > > > I think it is not a good design to ask caller to free mdata if get_mdata() > > operation is failed because mdata may or may not allocated in error case. > > > > In success case, user must free it because it is allocated (user accessed it), > > and in error case, user can ignore it because it should not be allocated. > > This is simpler mind model and less memory leak chance. > > I think this is confusing. It would be better to be consistent and > have the caller free up the allocated/not allocated memory. If you > check, the mdata pointer is being initialised to NULL in every > instance. Calling free with a NULL pointer is a valid case, which the > free call handles. There are multiple places in u-boot where the > caller is freeing up the allocated memory. So i think this should not > be an issue. The simple rule should be that, if the function returns 0 (successfully), the area will be allocated. If not (i.e. returns an error), the area will not be allocated. This will be a much more common behavior. -Takahiro Akashi > -sughosh > > > > > Thank you, > > -- > > Masami Hiramatsu
diff --git a/include/fwu.h b/include/fwu.h index acba725bc8..12f7eecdb0 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -53,6 +53,7 @@ int fwu_get_active_index(u32 *active_idx); int fwu_update_active_index(u32 active_idx); int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, int *alt_num); +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); int fwu_mdata_check(void); int fwu_revert_boot_index(void); int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h index d788eb69e7..53e39f9af6 100644 --- a/include/fwu_mdata.h +++ b/include/fwu_mdata.h @@ -64,4 +64,6 @@ struct fwu_mdata { struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; } __attribute__((__packed__)); +extern struct fwu_mdata_ops fwu_gpt_blk_ops; + #endif /* _FWU_MDATA_H_ */ diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c index 58e838fe28..252fcf50f6 100644 --- a/lib/fwu_updates/fwu_mdata.c +++ b/lib/fwu_updates/fwu_mdata.c @@ -25,6 +25,35 @@ static struct fwu_mdata_ops *get_fwu_mdata_ops(void) return ops; } +/** + * fwu_verify_mdata() - Verify the FWU metadata + * @mdata: FWU metadata structure + * @pri_part: FWU metadata partition is primary or secondary + * + * Verify the FWU metadata by computing the CRC32 for the metadata + * structure and comparing it against the CRC32 value stored as part + * of the structure. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part) +{ + u32 calc_crc32; + void *buf; + + buf = &mdata->version; + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + if (calc_crc32 != mdata->crc32) { + log_err("crc32 check failed for %s FWU metadata partition\n", + pri_part ? "primary" : "secondary"); + return -1; + } + + return 0; +} + /** * fwu_get_active_index() - Get active_index from the FWU metadata * @active_idx: active_index value to be read diff --git a/lib/fwu_updates/fwu_mdata_gpt_blk.c b/lib/fwu_updates/fwu_mdata_gpt_blk.c new file mode 100644 index 0000000000..cb47ddf4a7 --- /dev/null +++ b/lib/fwu_updates/fwu_mdata_gpt_blk.c @@ -0,0 +1,520 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <blk.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <malloc.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +#define PRIMARY_PART BIT(0) +#define SECONDARY_PART BIT(1) +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART) + +#define MDATA_READ BIT(0) +#define MDATA_WRITE BIT(1) + +#define IMAGE_ACCEPT_SET BIT(0) +#define IMAGE_ACCEPT_CLEAR BIT(1) + +static int gpt_get_mdata_partitions(struct blk_desc *desc, + u16 *primary_mpart, + u16 *secondary_mpart) +{ + int i, ret; + u32 mdata_parts; + efi_guid_t part_type_guid; + struct disk_partition info; + const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID; + + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { + if (part_get_info(desc, i, &info)) + continue; + uuid_str_to_bin(info.type_guid, part_type_guid.b, + UUID_STR_FORMAT_GUID); + + if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { + ++mdata_parts; + if (!*primary_mpart) + *primary_mpart = i; + else + *secondary_mpart = i; + } + } + + if (mdata_parts != 2) { + log_err("Expect two copies of the FWU metadata instead of %d\n", + mdata_parts); + ret = -EINVAL; + } else { + ret = 0; + } + + return ret; +} + +static int gpt_get_mdata_disk_part(struct blk_desc *desc, + struct disk_partition *info, + u32 part_num) +{ + int ret; + char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; + + ret = part_get_info(desc, part_num, info); + if (ret < 0) { + log_err("Unable to get the partition info for the FWU metadata part %d", + part_num); + return -1; + } + + /* Check that it is indeed the FWU metadata partition */ + if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) { + /* Found the FWU metadata partition */ + return 0; + } + + return -1; +} + +static int gpt_read_write_mdata(struct blk_desc *desc, + struct fwu_mdata *mdata, + u8 access, u32 part_num) +{ + int ret; + u32 len, blk_start, blkcnt; + struct disk_partition info; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1, + desc->blksz); + + ret = gpt_get_mdata_disk_part(desc, &info, part_num); + if (ret < 0) { + printf("Unable to get the FWU metadata partition\n"); + return -ENODEV; + } + + len = sizeof(*mdata); + blkcnt = BLOCK_CNT(len, desc); + if (blkcnt > info.size) { + log_err("Block count exceeds FWU metadata partition size\n"); + return -ERANGE; + } + + blk_start = info.start; + if (access == MDATA_READ) { + if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) { + log_err("Error reading FWU metadata from the device\n"); + return -EIO; + } + memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata)); + } else { + if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) { + log_err("Error writing FWU metadata to the device\n"); + return -EIO; + } + } + + return 0; +} + +static int gpt_read_mdata(struct blk_desc *desc, + struct fwu_mdata *mdata, u32 part_num) +{ + return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num); +} + +static int gpt_write_mdata_partition(struct blk_desc *desc, + struct fwu_mdata *mdata, + u32 part_num) +{ + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num); +} + +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata) +{ + int ret; + struct blk_desc *desc; + u16 primary_mpart = 0, secondary_mpart = 0; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + /* First write the primary partition*/ + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); + if (ret < 0) { + log_err("Updating primary FWU metadata partition failed\n"); + return ret; + } + + /* And now the replica */ + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); + if (ret < 0) { + log_err("Updating secondary FWU metadata partition failed\n"); + return ret; + } + + return 0; +} + +static int gpt_get_mdata(struct fwu_mdata **mdata) +{ + int ret; + struct blk_desc *desc; + u16 primary_mpart = 0, secondary_mpart = 0; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + *mdata = malloc(sizeof(struct fwu_mdata)); + if (!*mdata) { + log_err("Unable to allocate memory for reading FWU metadata\n"); + return -ENOMEM; + } + + ret = gpt_read_mdata(desc, *mdata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + return -EIO; + } + + ret = fwu_verify_mdata(*mdata, 1); + if (!ret) + return 0; + + /* + * Verification of the primary FWU metadata copy failed. + * Try to read the replica. + */ + memset(*mdata, 0, sizeof(struct fwu_mdata)); + ret = gpt_read_mdata(desc, *mdata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + return -EIO; + } + + ret = fwu_verify_mdata(*mdata, 0); + if (!ret) + return 0; + + /* Both the FWU metadata copies are corrupted. */ + return -1; +} + +static int gpt_check_mdata_validity(void) +{ + int ret; + struct blk_desc *desc; + struct fwu_mdata pri_mdata; + struct fwu_mdata secondary_mdata; + u16 primary_mpart = 0, secondary_mpart = 0; + u16 valid_partitions, invalid_partitions; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + /* + * Two FWU metadata partitions are expected. + * If we don't have two, user needs to create + * them first + */ + valid_partitions = 0; + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + goto secondary_read; + } + + ret = fwu_verify_mdata(&pri_mdata, 1); + if (!ret) + valid_partitions |= PRIMARY_PART; + +secondary_read: + /* Now check the secondary partition */ + ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + goto mdata_restore; + } + + ret = fwu_verify_mdata(&secondary_mdata, 0); + if (!ret) + valid_partitions |= SECONDARY_PART; + +mdata_restore: + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { + ret = -1; + /* + * Before returning, check that both the + * FWU metadata copies are the same. If not, + * the FWU metadata copies need to be + * re-populated. + */ + if (!memcmp(&pri_mdata, &secondary_mdata, + sizeof(struct fwu_mdata))) { + ret = 0; + } else { + log_err("Both FWU metadata copies are valid but do not match. Please check!\n"); + } + goto out; + } + + ret = -1; + if (!(valid_partitions & BOTH_PARTS)) + goto out; + + invalid_partitions = valid_partitions ^ BOTH_PARTS; + ret = gpt_write_mdata_partition(desc, + (invalid_partitions == PRIMARY_PART) ? + &secondary_mdata : &pri_mdata, + (invalid_partitions == PRIMARY_PART) ? + primary_mpart : secondary_mpart); + + if (ret < 0) + log_err("Restoring %s FWU metadata partition failed\n", + (invalid_partitions == PRIMARY_PART) ? + "primary" : "secondary"); + +out: + return ret; +} + +int fwu_gpt_get_active_index(u32 *active_idx) +{ + int ret; + struct fwu_mdata *mdata; + + ret = gpt_get_mdata(&mdata); + if (ret < 0) { + log_err("Unable to get valid FWU metadata\n"); + goto out; + } + + /* + * Found the FWU metadata partition, now read the active_index + * value + */ + *active_idx = mdata->active_index; + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { + printf("Active index value read is incorrect\n"); + ret = -EINVAL; + goto out; + } + +out: + free(mdata); + + return ret; +} + +static int gpt_get_image_alt_num(struct blk_desc *desc, + efi_guid_t image_type_id, + u32 update_bank, int *alt_no) +{ + int ret, i; + u32 part; + struct fwu_mdata *mdata; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + struct disk_partition info; + efi_guid_t unique_part_guid; + efi_guid_t image_guid = NULL_GUID; + + ret = gpt_get_mdata(&mdata); + if (ret < 0) { + log_err("Unable to read valid FWU metadata\n"); + goto out; + } + + /* + * The FWU metadata has been read. Now get the image_uuid for the + * image with the update_bank. + */ + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + if (!guidcmp(&image_type_id, + &mdata->img_entry[i].image_type_uuid)) { + img_entry = &mdata->img_entry[i]; + img_bank_info = &img_entry->img_bank_info[update_bank]; + guidcpy(&image_guid, &img_bank_info->image_uuid); + break; + } + } + + /* + * Now read the GPT Partition Table Entries to find a matching + * partition with UniquePartitionGuid value. We need to iterate + * through all the GPT partitions since they might be in any + * order + */ + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { + if (part_get_info(desc, i, &info)) + continue; + uuid_str_to_bin(info.uuid, unique_part_guid.b, + UUID_STR_FORMAT_GUID); + + if (!guidcmp(&unique_part_guid, &image_guid)) { + /* Found the partition */ + part = i; + *alt_no = fwu_plat_get_alt_num(&part); + if (*alt_no != -1) + log_info("alt_num %d for partition %pUl\n", + *alt_no, &image_guid); + ret = 0; + break; + } + } + + if (*alt_no == -1) { + log_err("alt_num not found for partition with GUID %pUl\n", + &image_guid); + ret = -EINVAL; + } + + if (i == MAX_SEARCH_PARTITIONS) { + log_err("Partition with the image guid not found\n"); + ret = -EINVAL; + } + +out: + free(mdata); + + return ret; +} + +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, + int *alt_no) +{ + int ret; + struct blk_desc *desc; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + return gpt_get_image_alt_num(desc, image_type_id, update_bank, alt_no); +} + +int fwu_gpt_mdata_check(void) +{ + /* + * Check if both the copies of the FWU metadata are + * valid. If one has gone bad, restore it from the + * other good copy. + */ + return gpt_check_mdata_validity(); +} + +int fwu_gpt_get_mdata(struct fwu_mdata **mdata) +{ + return gpt_get_mdata(mdata); +} + +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, + u32 bank, u8 action) +{ + void *buf; + int ret, i; + u32 nimages; + struct fwu_mdata *mdata; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + + ret = gpt_get_mdata(&mdata); + if (ret < 0) { + log_err("Unable to get valid FWU metadata\n"); + goto out; + } + + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; + img_entry = &mdata->img_entry[0]; + for (i = 0; i < nimages; i++) { + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { + img_bank_info = &img_entry[i].img_bank_info[bank]; + if (action == IMAGE_ACCEPT_SET) + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; + else + img_bank_info->accepted = 0; + + buf = &mdata->version; + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - + sizeof(u32)); + + ret = fwu_gpt_update_mdata(mdata); + goto out; + } + } + + /* Image not found */ + ret = -EINVAL; + +out: + free(mdata); + + return ret; +} + +static int fwu_gpt_accept_image(efi_guid_t *img_type_id, u32 bank) +{ + return fwu_gpt_set_clear_image_accept(img_type_id, bank, + IMAGE_ACCEPT_SET); +} + +static int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) +{ + return fwu_gpt_set_clear_image_accept(img_type_id, bank, + IMAGE_ACCEPT_CLEAR); +} + +struct fwu_mdata_ops fwu_gpt_blk_ops = { + .get_active_index = fwu_gpt_get_active_index, + .get_image_alt_num = fwu_gpt_get_image_alt_num, + .mdata_check = fwu_gpt_mdata_check, + .set_accept_image = fwu_gpt_accept_image, + .clear_accept_image = fwu_gpt_clear_accept_image, + .get_mdata = fwu_gpt_get_mdata, + .update_mdata = fwu_gpt_update_mdata, +};
In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add functions for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formated with GPT based partition scheme. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V2: * Move the function definition of fwu_verify_mdata to fwu_mdata.c to facilitate reuse * Remove the block device specific desc->devnum parameter for the fwu_plat_get_alt_num function call include/fwu.h | 1 + include/fwu_mdata.h | 2 + lib/fwu_updates/fwu_mdata.c | 29 ++ lib/fwu_updates/fwu_mdata_gpt_blk.c | 520 ++++++++++++++++++++++++++++ 4 files changed, 552 insertions(+) create mode 100644 lib/fwu_updates/fwu_mdata_gpt_blk.c