Re: [RESEND RFC PATCH 04/10] FWU: Add metadata access functions for GPT partitioned block devices

[Simon Glass <sjg@chromium.org>]

Hi Etienne,

On Wed, 8 Dec 2021 at 07:18, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hi Sughosh, Ilias (and all),
>
> On Thu, 2 Dec 2021 at 08:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Ilias,
> >
> > On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > wrote:
> >
> > > Hi Sughosh,
> > >
> > > On Thu, Nov 25, 2021 at 12:42:56PM +0530, 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>
> > > > ---
> > > >  lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++
> > > >  1 file changed, 716 insertions(+)
> > > >  create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c
> > > >
> > > > +#define SECONDARY_VALID              0x2
> > >
> > >
> > > Change those to BIT(0), BIT(1) etc please
> > >
> >
> > Will change.
> >
> >
> > >
> > > > +
> > > > +#define MDATA_READ           (u8)0x1
> > > > +#define MDATA_WRITE          (u8)0x2
> > > > +
> > > > +#define IMAGE_ACCEPT_SET     (u8)0x1
> > > > +#define IMAGE_ACCEPT_CLEAR   (u8)0x2
> > > > +
> > > > +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool
> > > pri_part)
> > > > +{
> > > > +     u32 calc_crc32;
> > > > +     void *buf;
> > > > +
> > > > +     buf = &metadata->version;
> > > > +     calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32));
> > > > +
> > > > +     if (calc_crc32 != metadata->crc32) {
> > > > +             log_err("crc32 check failed for %s metadata partition\n",
> > > > +                     pri_part ? "primary" : "secondary");
> > >
> > > I think we can rid of the 'bool pri_part'.  It's only used for printing and
> > > you can have more that 2 partitions anyway right?
> > >
> >
> > We will have only two partitions for the metadata. But i think looking at
> > it now, it is a bit fuzzy on which is the primary metadata partition and
> > which is the secondary one. Can we instead print the UniquePartitionGUID of
> > the metadata partition instead. That will help in identifying for which
> > metadata copy the verification failed. Let me know your thoughts on this.
> >
> >
> > > > +             return -1;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int gpt_get_metadata_partitions(struct blk_desc *desc,
> > >
> > >
> > > Please add a function descriptions (on following functions as well)
> > >
> >
> > I have added the function descriptions in the fwu_metadata.c, where the
> > api's are being defined. Do we need to add the descriptions for the
> > functions in this file as well?
> >
> >
> > >
> > > > +                                    u32 *primary_mpart,
> > > > +                                    u32 *secondary_mpart)
> > >
> > > u16 maybe? This is the number of gpt partitions with metadata right?
> >
> >
> > Okay.
> >
> >
> > >
> > >
> > > > +{
> > > > +     int i, ret;
> > > > +     u32 nparts, mparts;
>
> I think the 2 variables labels are too similar, it's a source of confusion.
> One is a number of entries, the second is a counter. It would be nice
> it's a bit more explicit.
>
> > > > +     gpt_entry *gpt_pte = NULL;
> > > > +     const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID;
> > > > +
> > > > +     ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1,
> > > > +                                  desc->blksz);
> > > > +
> > > > +     ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte);
> > > > +     if (ret < 0) {
> > > > +             log_err("Error getting GPT header and partitions\n");
> > > > +             ret = -EIO;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > > +     nparts = gpt_head->num_partition_entries;
> > > > +     for (i = 0, mparts = 0; i < nparts; i++) {
> > > > +             if (!guidcmp(&fwu_metadata_guid,
> > > > +                          &gpt_pte[i].partition_type_guid)) {
> > > > +                     ++mparts;
> > > > +                     if (!*primary_mpart)
> > > > +                             *primary_mpart = i + 1;
> > > > +                     else
> > > > +                             *secondary_mpart = i + 1;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     if (mparts != 2) {
> > > > +             log_err("Expect two copies of the metadata instead of
> > > %d\n",
> > > > +                     mparts);
> > > > +             ret = -EINVAL;
> > > > +     } else {
> > > > +             ret = 0;
> > > > +     }
> > > > +out:
> > > > +     free(gpt_pte);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static int gpt_get_metadata_disk_part(struct blk_desc *desc,
> > > > +                                   struct disk_partition *info,
> > > > +                                   u32 part_num)
> > > > +{
> > > > +     int ret;
> > > > +     char *metadata_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 metadata
> > > part %d",
> > > > +                     part_num);
> > > > +             return -1;
> > > > +     }
> > > > +
> > > > +     /* Check that it is indeed the metadata partition */
> > > > +     if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) {
> > > > +             /* Found the metadata partition */
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return -1;
> > > > +}
> > > > +
> > > > +static int gpt_read_write_metadata(struct blk_desc *desc,
> > > > +                                struct fwu_metadata *metadata,
> > > > +                                u8 access, u32 part_num)
> > > > +{
> > > > +     int ret;
> > > > +     u32 len, blk_start, blkcnt;
> > > > +     struct disk_partition info;
> > > > +
> > > > +     ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1,
> > > desc->blksz);
> > > > +
> > > > +     ret = gpt_get_metadata_disk_part(desc, &info, part_num);
> > > > +     if (ret < 0) {
> > > > +             printf("Unable to get the metadata partition\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     len = sizeof(*metadata);
> > > > +     blkcnt = BLOCK_CNT(len, desc);
> > > > +     if (blkcnt > info.size) {
> > > > +             log_err("Block count exceeds metadata partition size\n");
> > > > +             return -ERANGE;
> > > > +     }
> > > > +
> > > > +     blk_start = info.start;
> > > > +     if (access == MDATA_READ) {
> > > > +             if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) {
> > > > +                     log_err("Error reading metadata from the
> > > device\n");
> > > > +                     return -EIO;
> > > > +             }
> > > > +             memcpy(metadata, mdata, sizeof(struct fwu_metadata));
> > > > +     } else {
> > > > +             if (blk_dwrite(desc, blk_start, blkcnt, metadata) !=
> > > blkcnt) {
> > > > +                     log_err("Error writing metadata to the device\n");
> > > > +                     return -EIO;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int gpt_read_metadata(struct blk_desc *desc,
> > > > +                          struct fwu_metadata *metadata, u32 part_num)
> > > > +{
> > > > +     return gpt_read_write_metadata(desc, metadata, MDATA_READ,
> > > part_num);
> > > > +}
> > > > +
> > > > +static int gpt_write_metadata_partition(struct blk_desc *desc,
> > > > +                                     struct fwu_metadata *metadata,
> > > > +                                     u32 part_num)
> > > > +{
> > > > +     return gpt_read_write_metadata(desc, metadata, MDATA_WRITE,
> > > part_num);
> > > > +}
> > > > +
> > > > +static int gpt_update_metadata(struct fwu_metadata *metadata)
> > > > +{
> > > > +     int ret;
> > > > +     struct blk_desc *desc;
> > > > +     u32 primary_mpart, secondary_mpart;
> > > > +
> > > > +     ret = fwu_plat_get_blk_desc(&desc);
> > > > +     if (ret < 0) {
> > > > +             log_err("Block device not found\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     primary_mpart = secondary_mpart = 0;
> > > > +     ret = gpt_get_metadata_partitions(desc, &primary_mpart,
> > > > +                                       &secondary_mpart);
> > > > +
> > > > +     if (ret < 0) {
> > > > +             log_err("Error getting the metadata partitions\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     /* First write the primary partition*/
> > > > +     ret = gpt_write_metadata_partition(desc, metadata, primary_mpart);
> > > > +     if (ret < 0) {
> > > > +             log_err("Updating primary metadata partition failed\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     /* And now the replica */
> > > > +     ret = gpt_write_metadata_partition(desc, metadata,
> > > secondary_mpart);
> > > > +     if (ret < 0) {
> > > > +             log_err("Updating secondary metadata partition failed\n");
> > > > +             return ret;
> > > > +     }
> > >
> > > So shouldn't we do something about this case?  The first partition was
> > > correctly written and the second failed.  Now if the primary GPT somehow
> > > gets corrupted the device is now unusable.
>
> The replica is not there to overcome bitflips of the storage media.
> It's here to allow updates while reliable info a still available in
> the counter part.
> The scheme could be to rely on only 1 instance of the fwu-metadata
> (sorry Simon) image is valid.
> A first load: load 1st instance, crap the second.
> At update: find the crapped one: write it with new data. Upon success
> crapped the alternate one.
> This is a suggestion. There are many ways to handle that.
>
> For sure, the scheme should be well defined so that the boot stage
> that read fwu-data complies with the scheme used to write them.
>
>
>
> > > I assume that depending on what happened to the firmware rollback counter,
> > > we could either try to rewrite this, or revert the first one as well
> > > (assuming rollback counters allow that).
>
> Rollback counters should protect image version management, not
> metadata updates (imho).
>
> > >
> >
> > Okay, although this might be indicative of some underlying hardware issue
> > with the device, else this scenario should not play out.
> >
> >
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int gpt_get_valid_metadata(struct fwu_metadata **metadata)
>
> Could be renamed gpt_get_metadata(), we don't expect to get invalid data :)

How about gpt_get_mdata() ?

When I read this I thought it was getting the GPT table...'metadata'
is just too generic.

Regards,
Simon