Messages in this thread | | | Subject | Re: [PATCH] firmware: google: update vpd_decode from upstream | From | Stephen Boyd <> | Date | Fri, 02 Aug 2019 15:27:54 -0700 |
| |
Quoting Hung-Te Lin (2019-08-02 01:20:31) > The VPD implementation from Chromium Vital Product Data project has been > updated so vpd_decode be easily shared by kernel, firmware and the user > space utility programs. Also improved value range checks to prevent > kernel crash due to bad VPD data.
Please add a Fixes: tag here to fix the commit that introduces the problem. It would also be nice to get a description of the problem that this patch is fixing. For example, explaining why the types change from signed to unsigned.
> > Signed-off-by: Hung-Te Lin <hungte@chromium.org> > --- > drivers/firmware/google/vpd.c | 38 +++++++++------ > drivers/firmware/google/vpd_decode.c | 69 +++++++++++++++------------- > drivers/firmware/google/vpd_decode.h | 17 ++++--- > 3 files changed, 71 insertions(+), 53 deletions(-) > > diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c > index 0739f3b70347..ecf217a7db39 100644 > --- a/drivers/firmware/google/vpd.c > +++ b/drivers/firmware/google/vpd.c > @@ -73,7 +73,7 @@ static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp, > * exporting them as sysfs attributes. These keys present in old firmwares are > * ignored. > * > - * Returns VPD_OK for a valid key name, VPD_FAIL otherwise. > + * Returns VPD_DECODE_OK for a valid key name, VPD_DECODE_FAIL otherwise.
Maybe we should convert these things to use linux conventions instead of VPD error codes?
> * > * @key: The key name to check > * @key_len: key name length > @@ -86,14 +86,14 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len) > c = *key++; > > if (!isalnum(c) && c != '_') > - return VPD_FAIL; > + return VPD_DECODE_FAIL; > } > > - return VPD_OK; > + return VPD_DECODE_OK;
Can you split this rename out into it's own patch. That way we can confirm that there are no changes due to the rename of the enum.
> } > > -static int vpd_section_attrib_add(const u8 *key, s32 key_len, > - const u8 *value, s32 value_len, > +static int vpd_section_attrib_add(const u8 *key, u32 key_len, > + const u8 *value, u32 value_len, > void *arg) > { > int ret; > @@ -246,7 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec) > > static int vpd_sections_init(phys_addr_t physaddr) > { > - struct vpd_cbmem *temp; > + struct vpd_cbmem __iomem *temp; > struct vpd_cbmem header; > int ret = 0; > > @@ -254,7 +254,7 @@ static int vpd_sections_init(phys_addr_t physaddr) > if (!temp) > return -ENOMEM; > > - memcpy(&header, temp, sizeof(struct vpd_cbmem)); > + memcpy_fromio(&header, temp, sizeof(struct vpd_cbmem)); > memunmap(temp); > > if (header.magic != VPD_CBMEM_MAGIC) > @@ -316,7 +316,19 @@ static struct coreboot_driver vpd_driver = { > }, > .tag = CB_TAG_VPD, > }; > -module_coreboot_driver(vpd_driver); > + > +static int __init coreboot_vpd_init(void) > +{ > + return coreboot_driver_register(&vpd_driver); > +} > + > +static void __exit coreboot_vpd_exit(void) > +{ > + coreboot_driver_unregister(&vpd_driver); > +} > + > +module_init(coreboot_vpd_init); > +module_exit(coreboot_vpd_exit); > > MODULE_AUTHOR("Google, Inc."); > MODULE_LICENSE("GPL");
The above three hunks should be dropped. They're undoing other patches that have gone upstream.
> diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c > index 92e3258552fc..5531770e3d58 100644 > --- a/drivers/firmware/google/vpd_decode.c > +++ b/drivers/firmware/google/vpd_decode.c > @@ -9,19 +9,19 @@ > > #include "vpd_decode.h" > > -static int vpd_decode_len(const s32 max_len, const u8 *in, > - s32 *length, s32 *decoded_len) > +static int vpd_decode_len(const u32 max_len, const u8 *in, u32 *length,
Is there a reason why max_len and length changes to be unsigned? Presumably to fix something.
> + u32 *decoded_len) > { > u8 more; > int i = 0; > > if (!length || !decoded_len) > - return VPD_FAIL; > + return VPD_DECODE_FAIL; > > *length = 0; > do { > if (i >= max_len) > - return VPD_FAIL; > + return VPD_DECODE_FAIL; > > more = in[i] & 0x80; > *length <<= 7; > @@ -30,24 +30,43 @@ static int vpd_decode_len(const s32 max_len, const u8 *in, > } while (more); > > *decoded_len = i; > + return VPD_DECODE_OK; > +} > + > +static int vpd_decode_entry(const u32 max_len, const u8 *input_buf, > + u32 *consumed, const u8 **entry, u32 *entry_len) > +{ > + u32 decoded_len; > + > + if (vpd_decode_len(max_len - *consumed, &input_buf[*consumed],
Can you add a local variable for *consumed? So _consumed is passed in and then u32 consume = *_consumed.
> + entry_len, &decoded_len) != VPD_DECODE_OK) > + return VPD_DECODE_FAIL; > + if (max_len - *consumed < decoded_len) > + return VPD_DECODE_FAIL; > > - return VPD_OK; > + *consumed += decoded_len; > + *entry = input_buf + *consumed; > + > + /* entry_len is untrusted data and must be checked again. */ > + if (max_len - *consumed < *entry_len) > + return VPD_DECODE_FAIL;
Is consumed supposed to have move forward here on failure? Is entry supposed to point to something, or should it be pointing to NULL on a failure?
> + > + *consumed += *entry_len; > + return VPD_DECODE_OK; > } > > -int vpd_decode_string(const s32 max_len, const u8 *input_buf, s32 *consumed, > +int vpd_decode_string(const u32 max_len, const u8 *input_buf, u32 *consumed, > vpd_decode_callback callback, void *callback_arg) > { > int type; > - int res; > - s32 key_len; > - s32 value_len; > - s32 decoded_len; > + u32 key_len; > + u32 value_len; > const u8 *key; > const u8 *value; > > /* type */ > if (*consumed >= max_len) > - return VPD_FAIL; > + return VPD_DECODE_FAIL; > > type = input_buf[*consumed]; > > @@ -56,25 +75,13 @@ int vpd_decode_string(const s32 max_len, const u8 *input_buf, s32 *consumed, > case VPD_TYPE_STRING: > (*consumed)++; > > - /* key */ > - res = vpd_decode_len(max_len - *consumed, &input_buf[*consumed], > - &key_len, &decoded_len); > - if (res != VPD_OK || *consumed + decoded_len >= max_len) > - return VPD_FAIL; > - > - *consumed += decoded_len; > - key = &input_buf[*consumed]; > - *consumed += key_len; > - > - /* value */ > - res = vpd_decode_len(max_len - *consumed, &input_buf[*consumed], > - &value_len, &decoded_len); > - if (res != VPD_OK || *consumed + decoded_len > max_len) > - return VPD_FAIL; > + if (vpd_decode_entry(max_len, input_buf, consumed, &key, > + &key_len) != VPD_DECODE_OK) > + return VPD_DECODE_FAIL; > > - *consumed += decoded_len; > - value = &input_buf[*consumed]; > - *consumed += value_len; > + if (vpd_decode_entry(max_len, input_buf, consumed, &value, > + &value_len) != VPD_DECODE_OK) > + return VPD_DECODE_FAIL; > > if (type == VPD_TYPE_STRING) > return callback(key, key_len, value, value_len, > @@ -82,8 +89,8 @@ int vpd_decode_string(const s32 max_len, const u8 *input_buf, s32 *consumed, > break; > > default: > - return VPD_FAIL; > + return VPD_DECODE_FAIL; > } > > - return VPD_OK; > + return VPD_DECODE_OK; > } > diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h > index cf8c2ace155a..4113ac2f4a70 100644 > --- a/drivers/firmware/google/vpd_decode.h > +++ b/drivers/firmware/google/vpd_decode.h > @@ -13,28 +13,27 @@ > #include <linux/types.h> > > enum { > - VPD_OK = 0, > - VPD_FAIL, > + VPD_DECODE_OK = 0, > + VPD_DECODE_FAIL = 1,
I wonder why this is an enum vs. just using the typical kernel error codes from errno.h.
> }; > > enum { > VPD_TYPE_TERMINATOR = 0, > VPD_TYPE_STRING, > - VPD_TYPE_INFO = 0xfe, > + VPD_TYPE_INFO = 0xfe, > VPD_TYPE_IMPLICIT_TERMINATOR = 0xff, > };
Please drop this change, it's just unaligning things.
> > /* Callback for vpd_decode_string to invoke. */ > -typedef int vpd_decode_callback(const u8 *key, s32 key_len, > - const u8 *value, s32 value_len, > - void *arg); > +typedef int vpd_decode_callback(const u8 *key, u32 key_len, const u8 *value, > + u32 value_len, void *arg); > > /* > * vpd_decode_string > * > * Given the encoded string, this function invokes callback with extracted > - * (key, value). The *consumed will be plused the number of bytes consumed in > - * this function. > + * (key, value). The *consumed will be incremented by the number of bytes
Use @consumed to refer to the argument. Split this out into a different patch as it's just updating the documentation to say the same thing.
> + * consumed in this function. > * > * The input_buf points to the first byte of the input buffer. > *
| |