| Message ID | 20230102182640.2411224-1-jaswinder.singh@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Handle meta-data in common code | expand |
Hi Jassi, On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote: > Instead of each i/f having to implement their own meta-data verification > and storage, move the logic in common code. This simplifies the i/f code > much simpler and compact. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > --- > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++ > include/fwu.h | 41 ++++++++ > lib/fwu_updates/fwu.c | 142 ++++++++++++++++++++++++++- > 3 files changed, 213 insertions(+), 4 deletions(-) > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c > index b477e9603f..e03773c584 100644 > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > @@ -16,6 +16,40 @@ > #include <linux/types.h> > #include <u-boot/crc.h> [...] > + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided > + * @mdata: FWU metadata structure > + * @part: Bitmask of FWU metadata partitions to be written to > + * > + * Return: 0 if OK, -ve on error > + */ > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part) > +{ > + void *buf = &mdata->version; > + int err = 0; > + > + /* > + * Calculate the crc32 for the updated FWU metadata > + * and put the updated value in the FWU metadata crc32 > + * field > + */ > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > + > + if (part & PRIMARY_PART) > + err = fwu_write_mdata(g_dev, mdata, true); > + > + if (err) { > + log_err("Unable to write primary mdata\n"); > + return err; > + } > + > + if (part & SECONDARY_PART) > + err = fwu_write_mdata(g_dev, mdata, false); > + > + if (err) { > + log_err("Unable to write secondary mdata\n"); > + return err; > + } Can we write this err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false); if (err) log_err("Unable to write %s partition\n", part & PRIMARY_PART ? "primary": "secondary" ); .... > + > + /* update the cached copy of meta-data */ > + memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata)); > + > + return 0; > +} > + > +static inline int mdata_crc_check(struct fwu_mdata *mdata) > +{ > + void *buf = &mdata->version; > + u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > + > + return calc_crc32 == mdata->crc32 ? 0 : -EINVAL; > +} > + > +/** > + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata > + * > + * Read both the metadata copies from the storage media, verify their checksum, > + * and ascertain that both copies match. If one of the copies has gone bad, > + * restore it from the good copy. > + * > + * Return: 0 if OK, -ve on error > + */ > +int fwu_get_verified_mdata(struct fwu_mdata *mdata) > +{ > + int err; > + bool pri_ok, sec_ok; > + struct fwu_mdata s, *p_mdata, *s_mdata; > + > + p_mdata = &g_mdata; > + s_mdata = &s; Why are we defining it like this? Readability to have pointers for primary and secondary metadata? > + > + /* if mdata already read and ready */ > + err = mdata_crc_check(p_mdata); > + if (!err) > + goto ret_mdata; Shouldn't we check the secondary metadata ? At least that's what the old fwu_check_mdata_validity() was doing. > + /* else read, verify and, if needed, fix mdata */ > + > + pri_ok = false; > + err = fwu_read_mdata(g_dev, p_mdata, true); > + if (!err) { > + err = mdata_crc_check(p_mdata); > + if (!err) > + pri_ok = true; > + else > + log_debug("primary mdata: crc32 failed\n"); > + } > + > + sec_ok = false; > + err = fwu_read_mdata(g_dev, s_mdata, false); > + if (!err) { > + err = mdata_crc_check(s_mdata); > + if (!err) > + sec_ok = true; > + else > + log_debug("secondary mdata: crc32 failed\n"); > + } > + > + if (pri_ok && sec_ok) { > + /* > + * Before returning, check that both the > + * FWU metadata copies are the same. > + */ > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > + if (!err) > + goto ret_mdata; > + > + /* > + * If not, populate the secondary partition from the > + * primary partition copy. > + */ > + log_info("Both FWU metadata copies are valid but do not match."); > + log_info(" Restoring the secondary partition from the primary\n"); > + sec_ok = false; > + } > + > + if (!pri_ok) { > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART); > + if (err) > + goto ret_mdata; The error print here is a bit misleading. It's a failed write, not a crc32 mismatch > + } > + > + if (!sec_ok) { > + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); > + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); > + if (err) > + goto ret_mdata; > + } > + > +ret_mdata: > + if (err) > + log_debug("mdata : crc32 failed\n"); > + else if (mdata) > + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); > + > + return err; > +} > + > /** > * fwu_verify_mdata() - Verify the FWU metadata > * @mdata: FWU metadata structure > -- > 2.34.1 > Regards /Ilias
On Mon, 9 Jan 2023 at 06:54, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Jassi, > > On Mon, Jan 02, 2023 at 12:26:40PM -0600, Jassi Brar wrote: > > Instead of each i/f having to implement their own meta-data verification > > and storage, move the logic in common code. This simplifies the i/f code > > much simpler and compact. > > > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > > --- > > drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++ > > include/fwu.h | 41 ++++++++ > > lib/fwu_updates/fwu.c | 142 ++++++++++++++++++++++++++- > > 3 files changed, 213 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c > > index b477e9603f..e03773c584 100644 > > --- a/drivers/fwu-mdata/fwu-mdata-uclass.c > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c ..... > > + */ > > +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part) > > +{ > > + void *buf = &mdata->version; > > + int err = 0; > > + > > + /* > > + * Calculate the crc32 for the updated FWU metadata > > + * and put the updated value in the FWU metadata crc32 > > + * field > > + */ > > + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); > > + > > + if (part & PRIMARY_PART) > > + err = fwu_write_mdata(g_dev, mdata, true); > > + > > + if (err) { > > + log_err("Unable to write primary mdata\n"); > > + return err; > > + } > > + > > + if (part & SECONDARY_PART) > > + err = fwu_write_mdata(g_dev, mdata, false); > > + > > + if (err) { > > + log_err("Unable to write secondary mdata\n"); > > + return err; > > + } > > Can we write this > err = fwu_write_mdata(g_dev, mdata, part & PRIMARY_PART ? true: false); > if (err) > log_err("Unable to write %s partition\n", part & PRIMARY_PART ? "primary": "secondary" ); > .... > of course :) > > +int fwu_get_verified_mdata(struct fwu_mdata *mdata) > > +{ > > + int err; > > + bool pri_ok, sec_ok; > > + struct fwu_mdata s, *p_mdata, *s_mdata; > > + > > + p_mdata = &g_mdata; > > + s_mdata = &s; > > Why are we defining it like this? Readability to have pointers for primary > and secondary metadata? > that's the idea. > > + > > + /* if mdata already read and ready */ > > + err = mdata_crc_check(p_mdata); > > + if (!err) > > + goto ret_mdata; > > Shouldn't we check the secondary metadata ? At least that's what the old > fwu_check_mdata_validity() was doing. > During the first run after boot, both copies are checked. Also when we update the mdata. Othwise we have a good primary copy, even if the secondary is corrupted for some mysterious (corrupted in readonly mode) reason maybe we should let that be fixed after reboot and not add crc checking cost to every call? > > + /* else read, verify and, if needed, fix mdata */ > > + > > + pri_ok = false; > > + err = fwu_read_mdata(g_dev, p_mdata, true); > > + if (!err) { > > + err = mdata_crc_check(p_mdata); > > + if (!err) > > + pri_ok = true; > > + else > > + log_debug("primary mdata: crc32 failed\n"); > > + } > > + > > + sec_ok = false; > > + err = fwu_read_mdata(g_dev, s_mdata, false); > > + if (!err) { > > + err = mdata_crc_check(s_mdata); > > + if (!err) > > + sec_ok = true; > > + else > > + log_debug("secondary mdata: crc32 failed\n"); > > + } > > + > > + if (pri_ok && sec_ok) { > > + /* > > + * Before returning, check that both the > > + * FWU metadata copies are the same. > > + */ > > + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > > + if (!err) > > + goto ret_mdata; > > + > > + /* > > + * If not, populate the secondary partition from the > > + * primary partition copy. > > + */ > > + log_info("Both FWU metadata copies are valid but do not match."); > > + log_info(" Restoring the secondary partition from the primary\n"); > > + sec_ok = false; > > + } > > + > > + if (!pri_ok) { > > + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata)); > > + err = fwu_sync_mdata(p_mdata, PRIMARY_PART); > > + if (err) > > + goto ret_mdata; > > The error print here is a bit misleading. It's a failed write, not a crc32 > mismatch > Fixed. Thanks.
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c index b477e9603f..e03773c584 100644 --- a/drivers/fwu-mdata/fwu-mdata-uclass.c +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c @@ -16,6 +16,40 @@ #include <linux/types.h> #include <u-boot/crc.h> +/** + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata() + * + * Return: 0 if OK, -ve on error + */ +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->read_mdata) { + log_debug("read_mdata() method not defined\n"); + return -ENOSYS; + } + + return ops->read_mdata(dev, mdata, primary); +} + +/** + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata() + * + * Return: 0 if OK, -ve on error + */ +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary) +{ + const struct fwu_mdata_ops *ops = device_get_ops(dev); + + if (!ops->write_mdata) { + log_debug("write_mdata() method not defined\n"); + return -ENOSYS; + } + + return ops->write_mdata(dev, mdata, primary); +} + /** * fwu_get_mdata_part_num() - Get the FWU metadata partition numbers * @dev: FWU metadata device diff --git a/include/fwu.h b/include/fwu.h index 0919ced812..1a700c9e6a 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -24,6 +24,26 @@ struct fwu_mdata_gpt_blk_priv { * @update_mdata() - Update the FWU metadata copy */ struct fwu_mdata_ops { + /** + * read_mdata() - Populate the asked FWU metadata copy + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @primary: If primary or secondary copy of meta-data is to be read + * + * Return: 0 if OK, -ve on error + */ + int (*read_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary); + + /** + * write_mdata() - Write the given FWU metadata copy + * @dev: FWU metadata device + * @mdata: Copy of the FWU metadata + * @primary: If primary or secondary copy of meta-data is to be written + * + * Return: 0 if OK, -ve on error + */ + int (*write_mdata)(struct udevice *dev, struct fwu_mdata *mdata, bool primary); + /** * check_mdata() - Check if the FWU metadata is valid * @dev: FWU device @@ -126,6 +146,27 @@ struct fwu_mdata_ops { EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) +/** + * fwu_read_mdata() - Wrapper around fwu_mdata_ops.read_mdata() + */ +int fwu_read_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary); + +/** + * fwu_write_mdata() - Wrapper around fwu_mdata_ops.write_mdata() + */ +int fwu_write_mdata(struct udevice *dev, struct fwu_mdata *mdata, bool primary); + +/** + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata + * + * Read both the metadata copies from the storage media, verify their checksum, + * and ascertain that both copies match. If one of the copies has gone bad, + * restore it from the good copy. + * + * Return: 0 if OK, -ve on error +*/ +int fwu_get_verified_mdata(struct fwu_mdata *mdata); + /** * fwu_check_mdata_validity() - Check for validity of the FWU metadata copies * diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index 5313d07302..4554654727 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -15,13 +15,13 @@ #include <linux/errno.h> #include <linux/types.h> +#include <u-boot/crc.h> + +static struct fwu_mdata g_mdata; /* = {0} makes uninit crc32 always invalid */ +static struct udevice *g_dev; static u8 in_trial; static u8 boottime_check; -#include <linux/errno.h> -#include <linux/types.h> -#include <u-boot/crc.h> - enum { IMAGE_ACCEPT_SET = 1, IMAGE_ACCEPT_CLEAR, @@ -161,6 +161,140 @@ static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) return -ENOENT; } +/** + * fwu_sync_mdata() - Update given meta-data partition(s) with the copy provided + * @mdata: FWU metadata structure + * @part: Bitmask of FWU metadata partitions to be written to + * + * Return: 0 if OK, -ve on error + */ +static int fwu_sync_mdata(struct fwu_mdata *mdata, int part) +{ + void *buf = &mdata->version; + int err = 0; + + /* + * Calculate the crc32 for the updated FWU metadata + * and put the updated value in the FWU metadata crc32 + * field + */ + mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + if (part & PRIMARY_PART) + err = fwu_write_mdata(g_dev, mdata, true); + + if (err) { + log_err("Unable to write primary mdata\n"); + return err; + } + + if (part & SECONDARY_PART) + err = fwu_write_mdata(g_dev, mdata, false); + + if (err) { + log_err("Unable to write secondary mdata\n"); + return err; + } + + /* update the cached copy of meta-data */ + memcpy(&g_mdata, mdata, sizeof(struct fwu_mdata)); + + return 0; +} + +static inline int mdata_crc_check(struct fwu_mdata *mdata) +{ + void *buf = &mdata->version; + u32 calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32)); + + return calc_crc32 == mdata->crc32 ? 0 : -EINVAL; +} + +/** + * fwu_get_verified_mdata() - Read, verify and return the FWU metadata + * + * Read both the metadata copies from the storage media, verify their checksum, + * and ascertain that both copies match. If one of the copies has gone bad, + * restore it from the good copy. + * + * Return: 0 if OK, -ve on error + */ +int fwu_get_verified_mdata(struct fwu_mdata *mdata) +{ + int err; + bool pri_ok, sec_ok; + struct fwu_mdata s, *p_mdata, *s_mdata; + + p_mdata = &g_mdata; + s_mdata = &s; + + /* if mdata already read and ready */ + err = mdata_crc_check(p_mdata); + if (!err) + goto ret_mdata; + /* else read, verify and, if needed, fix mdata */ + + pri_ok = false; + err = fwu_read_mdata(g_dev, p_mdata, true); + if (!err) { + err = mdata_crc_check(p_mdata); + if (!err) + pri_ok = true; + else + log_debug("primary mdata: crc32 failed\n"); + } + + sec_ok = false; + err = fwu_read_mdata(g_dev, s_mdata, false); + if (!err) { + err = mdata_crc_check(s_mdata); + if (!err) + sec_ok = true; + else + log_debug("secondary mdata: crc32 failed\n"); + } + + if (pri_ok && sec_ok) { + /* + * Before returning, check that both the + * FWU metadata copies are the same. + */ + err = memcmp(p_mdata, s_mdata, sizeof(struct fwu_mdata)); + if (!err) + goto ret_mdata; + + /* + * If not, populate the secondary partition from the + * primary partition copy. + */ + log_info("Both FWU metadata copies are valid but do not match."); + log_info(" Restoring the secondary partition from the primary\n"); + sec_ok = false; + } + + if (!pri_ok) { + memcpy(p_mdata, s_mdata, sizeof(struct fwu_mdata)); + err = fwu_sync_mdata(p_mdata, PRIMARY_PART); + if (err) + goto ret_mdata; + } + + if (!sec_ok) { + memcpy(s_mdata, p_mdata, sizeof(struct fwu_mdata)); + err = fwu_sync_mdata(s_mdata, SECONDARY_PART); + if (err) + goto ret_mdata; + } + +ret_mdata: + if (err) + log_debug("mdata : crc32 failed\n"); + else if (mdata) + memcpy(mdata, p_mdata, sizeof(struct fwu_mdata)); + + return err; +} + /** * fwu_verify_mdata() - Verify the FWU metadata * @mdata: FWU metadata structure
Instead of each i/f having to implement their own meta-data verification and storage, move the logic in common code. This simplifies the i/f code much simpler and compact. Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- drivers/fwu-mdata/fwu-mdata-uclass.c | 34 +++++++ include/fwu.h | 41 ++++++++ lib/fwu_updates/fwu.c | 142 ++++++++++++++++++++++++++- 3 files changed, 213 insertions(+), 4 deletions(-)