[PATCH] firmware: google: update vpd_decode from upstream

[PATCH] firmware: google: update vpd_decode from upstream

[Hung-Te Lin]

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.

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.
  *
  * @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;
 }
 
-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;
@@ -101,11 +101,11 @@ static int vpd_section_attrib_add(const u8 *key, s32 key_len,
 	struct vpd_attrib_info *info;
 
 	/*
-	 * Return VPD_OK immediately to decode next entry if the current key
-	 * name contains invalid characters.
+	 * Return VPD_DECODE_OK immediately to decode next entry if the current
+	 * key name contains invalid characters.
 	 */
-	if (vpd_section_check_key_name(key, key_len) != VPD_OK)
-		return VPD_OK;
+	if (vpd_section_check_key_name(key, key_len) != VPD_DECODE_OK)
+		return VPD_DECODE_OK;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -174,7 +174,7 @@ static int vpd_section_create_attribs(struct vpd_section *sec)
 	do {
 		ret = vpd_decode_string(sec->bin_attr.size, sec->baseaddr,
 					&consumed, vpd_section_attrib_add, sec);
-	} while (ret == VPD_OK);
+	} while (ret == VPD_DECODE_OK);
 
 	return 0;
 }
@@ -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");
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,
+			  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],
+			   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;
+
+	*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,
 };
 
 enum {
 	VPD_TYPE_TERMINATOR = 0,
 	VPD_TYPE_STRING,
-	VPD_TYPE_INFO                = 0xfe,
+	VPD_TYPE_INFO = 0xfe,
 	VPD_TYPE_IMPLICIT_TERMINATOR = 0xff,
 };
 
 /* 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
+ * consumed in this function.
  *
  * The input_buf points to the first byte of the input buffer.
  *
@@ -44,7 +43,7 @@ typedef int vpd_decode_callback(const u8 *key, s32 key_len,
  * If one entry is successfully decoded, sends it to callback and returns the
  * result.
  */
-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);
 
 #endif  /* __VPD_DECODE_H */
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH] firmware: google: update vpd_decode from upstream

[Stephen Boyd]

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.
>   *

Re: [PATCH] firmware: google: update vpd_decode from upstream

[Guenter Roeck]

On Fri, Aug 02, 2019 at 03:27:54PM -0700, Stephen Boyd wrote:
> 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;

The change to __iomem should also be a separate patch.

Guenter

Re: [PATCH] firmware: google: update vpd_decode from upstream

[Stephen Boyd]

Quoting Guenter Roeck (2019-08-07 06:58:34)
> On Fri, Aug 02, 2019 at 03:27:54PM -0700, Stephen Boyd wrote:
> > Quoting Hung-Te Lin (2019-08-02 01:20:31)
> > >  
> > > -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;
> 
> The change to __iomem should also be a separate patch.
> 

Please don't change it back to __iomem. See commit ae21f41e1f56
("firmware: vpd: Drop __iomem usage for memremap() memory") for why.

Re: [PATCH] firmware: google: update vpd_decode from upstream

[Guenter Roeck]

On 8/7/19 7:59 AM, Stephen Boyd wrote:
> Quoting Guenter Roeck (2019-08-07 06:58:34)
>> On Fri, Aug 02, 2019 at 03:27:54PM -0700, Stephen Boyd wrote:
>>> Quoting Hung-Te Lin (2019-08-02 01:20:31)
>>>>   
>>>> -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;
>>
>> The change to __iomem should also be a separate patch.
>>
> 
> Please don't change it back to __iomem. See commit ae21f41e1f56
> ("firmware: vpd: Drop __iomem usage for memremap() memory") for why.
> 
> 
Sorry, I didn't notice that part.

Guenter

[PATCH v2] firmware: google: update vpd_decode from upstream

[Hung-Te Lin]

The VPD implementation from Chromium Vital Product Data project used to
parse data from untrusted input without checking if there is invalid
data (for example the if the size becomes negative, or larger than whole
input buffer), which may cause buffer overflow on corrupted data.

To fix that, the upstream driver 'vpd_decode' has changed size
parameters to unsigned integer (u32), and refactored the parsing of
entry header so the size is always checked properly.

Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
Signed-off-by: Hung-Te Lin <hungte@chromium.org>
---
Changes in v2:
- removed renaming of enum
- prevent undoing other patches that have gone upstream.
- dropped cosmetic changes
- changed *consume to local variable

 drivers/firmware/google/vpd.c        |  4 +-
 drivers/firmware/google/vpd_decode.c | 55 ++++++++++++++++------------
 drivers/firmware/google/vpd_decode.h |  9 ++---
 3 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 0739f3b70347..db0812263d46 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -92,8 +92,8 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
 	return VPD_OK;
 }
 
-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;
diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
index 92e3258552fc..7a5b0c72db00 100644
--- a/drivers/firmware/google/vpd_decode.c
+++ b/drivers/firmware/google/vpd_decode.c
@@ -9,8 +9,8 @@
 
 #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,
+			  u32 *decoded_len)
 {
 	u8 more;
 	int i = 0;
@@ -30,18 +30,39 @@ static int vpd_decode_len(const s32 max_len, const u8 *in,
 	} while (more);
 
 	*decoded_len = i;
+	return VPD_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;
+	u32 consumed = *_consumed;
+
+	if (vpd_decode_len(max_len - consumed, &input_buf[consumed],
+			   entry_len, &decoded_len) != VPD_OK)
+		return VPD_FAIL;
+	if (max_len - consumed < decoded_len)
+		return VPD_FAIL;
+
+	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_FAIL;
 
+	consumed += decoded_len;
+	*_consumed = consumed;
 	return VPD_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;
 
@@ -56,26 +77,14 @@ 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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &key,
+				     &key_len) != VPD_OK)
 			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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &value,
+				     &value_len) != VPD_OK)
 			return VPD_FAIL;
 
-		*consumed += decoded_len;
-		value = &input_buf[*consumed];
-		*consumed += value_len;
-
 		if (type == VPD_TYPE_STRING)
 			return callback(key, key_len, value, value_len,
 					callback_arg);
diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
index cf8c2ace155a..b65d246a6804 100644
--- a/drivers/firmware/google/vpd_decode.h
+++ b/drivers/firmware/google/vpd_decode.h
@@ -25,15 +25,14 @@ enum {
 };
 
 /* 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
+ * (key, value). The *consumed will be plused by the number of bytes consumed in
  * this function.
  *
  * The input_buf points to the first byte of the input buffer.
@@ -44,7 +43,7 @@ typedef int vpd_decode_callback(const u8 *key, s32 key_len,
  * If one entry is successfully decoded, sends it to callback and returns the
  * result.
  */
-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);
 
 #endif  /* __VPD_DECODE_H */
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH v2] firmware: google: update vpd_decode from upstream

[Greg Kroah-Hartman]

On Thu, Aug 29, 2019 at 06:19:45PM +0800, Hung-Te Lin wrote:
> The VPD implementation from Chromium Vital Product Data project used to
> parse data from untrusted input without checking if there is invalid
> data (for example the if the size becomes negative, or larger than whole
> input buffer), which may cause buffer overflow on corrupted data.
> 
> To fix that, the upstream driver 'vpd_decode' has changed size
> parameters to unsigned integer (u32), and refactored the parsing of
> entry header so the size is always checked properly.

"the upstream driver"?  That's the code you are touching here.

What do you mean by "upstream"?  Your subject and this paragraph does
not make much sense.

Please describe exactly what you are doing here, we don't care what
anyone else did with this code in any random repo that is not Linus's
tree.

thanks,

greg k-h

[PATCH v3] firmware: google: check if size is valid when decoding VPD data

[Hung-Te Lin]

The VPD implementation from Chromium Vital Product Data project used to
parse data from untrusted input without checking if the meta data is
invalid or corrupted. For example, the size from decoded content may
be negative value, or larger than whole input buffer. Such invalid data
may cause buffer overflow.

To fix that, the size parameters passed to vpd_decode functions should
be changed to unsigned integer (u32) type, and the parsing of entry
header should be refactored so every size field is correctly verified
before starting to decode.

Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
Signed-off-by: Hung-Te Lin <hungte@chromium.org>
---
Changes in v3:
- Revise commit message

 drivers/firmware/google/vpd.c        |  4 +-
 drivers/firmware/google/vpd_decode.c | 55 ++++++++++++++++------------
 drivers/firmware/google/vpd_decode.h |  9 ++---
 3 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 0739f3b70347..db0812263d46 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -92,8 +92,8 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
 	return VPD_OK;
 }
 
-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;
diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
index 92e3258552fc..7a5b0c72db00 100644
--- a/drivers/firmware/google/vpd_decode.c
+++ b/drivers/firmware/google/vpd_decode.c
@@ -9,8 +9,8 @@
 
 #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,
+			  u32 *decoded_len)
 {
 	u8 more;
 	int i = 0;
@@ -30,18 +30,39 @@ static int vpd_decode_len(const s32 max_len, const u8 *in,
 	} while (more);
 
 	*decoded_len = i;
+	return VPD_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;
+	u32 consumed = *_consumed;
+
+	if (vpd_decode_len(max_len - consumed, &input_buf[consumed],
+			   entry_len, &decoded_len) != VPD_OK)
+		return VPD_FAIL;
+	if (max_len - consumed < decoded_len)
+		return VPD_FAIL;
+
+	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_FAIL;
 
+	consumed += decoded_len;
+	*_consumed = consumed;
 	return VPD_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;
 
@@ -56,26 +77,14 @@ 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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &key,
+				     &key_len) != VPD_OK)
 			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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &value,
+				     &value_len) != VPD_OK)
 			return VPD_FAIL;
 
-		*consumed += decoded_len;
-		value = &input_buf[*consumed];
-		*consumed += value_len;
-
 		if (type == VPD_TYPE_STRING)
 			return callback(key, key_len, value, value_len,
 					callback_arg);
diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
index cf8c2ace155a..b65d246a6804 100644
--- a/drivers/firmware/google/vpd_decode.h
+++ b/drivers/firmware/google/vpd_decode.h
@@ -25,15 +25,14 @@ enum {
 };
 
 /* 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
+ * (key, value). The *consumed will be plused by the number of bytes consumed in
  * this function.
  *
  * The input_buf points to the first byte of the input buffer.
@@ -44,7 +43,7 @@ typedef int vpd_decode_callback(const u8 *key, s32 key_len,
  * If one entry is successfully decoded, sends it to callback and returns the
  * result.
  */
-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);
 
 #endif  /* __VPD_DECODE_H */
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH v3] firmware: google: check if size is valid when decoding VPD data

[Stephen Boyd]

Quoting Hung-Te Lin (2019-08-29 04:45:43)
> The VPD implementation from Chromium Vital Product Data project used to
> parse data from untrusted input without checking if the meta data is
> invalid or corrupted. For example, the size from decoded content may
> be negative value, or larger than whole input buffer. Such invalid data
> may cause buffer overflow.
> 
> To fix that, the size parameters passed to vpd_decode functions should
> be changed to unsigned integer (u32) type, and the parsing of entry
> header should be refactored so every size field is correctly verified
> before starting to decode.
> 
> Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
> Signed-off-by: Hung-Te Lin <hungte@chromium.org>
> ---

Two minor nitpicks, otherwise

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

> diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
> index 92e3258552fc..7a5b0c72db00 100644
> --- a/drivers/firmware/google/vpd_decode.c
> +++ b/drivers/firmware/google/vpd_decode.c
> @@ -9,8 +9,8 @@
>  
>  #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,
> +                         u32 *decoded_len)

Nitpick: Can you leave the first line alone? Just change types from s32
to u32 on the same line so that this hunk clearly shows that the
function name and other arguments aren't changing.

>  {
>         u8 more;
>         int i = 0;
> diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
> index cf8c2ace155a..b65d246a6804 100644
> --- a/drivers/firmware/google/vpd_decode.h
> +++ b/drivers/firmware/google/vpd_decode.h
> @@ -25,15 +25,14 @@ enum {
[...]
>  
>  /*
>   * 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
> + * (key, value). The *consumed will be plused by the number of bytes consumed in
>   * this function.
>   *
>   * The input_buf points to the first byte of the input buffer.

This part can be a different patch that also converts this to kernel-doc
style. See Documentation/doc-guide/kernel-doc.rst for more info.

[PATCH v4] firmware: google: check if size is valid when decoding VPD data

[Hung-Te Lin]

The VPD implementation from Chromium Vital Product Data project used to
parse data from untrusted input without checking if the meta data is
invalid or corrupted. For example, the size from decoded content may
be negative value, or larger than whole input buffer. Such invalid data
may cause buffer overflow.

To fix that, the size parameters passed to vpd_decode functions should
be changed to unsigned integer (u32) type, and the parsing of entry
header should be refactored so every size field is correctly verified
before starting to decode.

Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
Signed-off-by: Hung-Te Lin <hungte@chromium.org>
---
Changes in v4:
- Prevent changing indent in function prototype
- Removed changes in function comments

 drivers/firmware/google/vpd.c        |  4 +-
 drivers/firmware/google/vpd_decode.c | 55 ++++++++++++++++------------
 drivers/firmware/google/vpd_decode.h |  6 +--
 3 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 0739f3b70347..db0812263d46 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -92,8 +92,8 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
 	return VPD_OK;
 }
 
-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;
diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
index 92e3258552fc..dda525c0f968 100644
--- a/drivers/firmware/google/vpd_decode.c
+++ b/drivers/firmware/google/vpd_decode.c
@@ -9,8 +9,8 @@
 
 #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, u32 *decoded_len)
 {
 	u8 more;
 	int i = 0;
@@ -30,18 +30,39 @@ static int vpd_decode_len(const s32 max_len, const u8 *in,
 	} while (more);
 
 	*decoded_len = i;
+	return VPD_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;
+	u32 consumed = *_consumed;
+
+	if (vpd_decode_len(max_len - consumed, &input_buf[consumed],
+			   entry_len, &decoded_len) != VPD_OK)
+		return VPD_FAIL;
+	if (max_len - consumed < decoded_len)
+		return VPD_FAIL;
+
+	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_FAIL;
 
+	consumed += decoded_len;
+	*_consumed = consumed;
 	return VPD_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;
 
@@ -56,26 +77,14 @@ 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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &key,
+				     &key_len) != VPD_OK)
 			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)
+		if (vpd_decode_entry(max_len, input_buf, consumed, &value,
+				     &value_len) != VPD_OK)
 			return VPD_FAIL;
 
-		*consumed += decoded_len;
-		value = &input_buf[*consumed];
-		*consumed += value_len;
-
 		if (type == VPD_TYPE_STRING)
 			return callback(key, key_len, value, value_len,
 					callback_arg);
diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
index cf8c2ace155a..8dbe41cac599 100644
--- a/drivers/firmware/google/vpd_decode.h
+++ b/drivers/firmware/google/vpd_decode.h
@@ -25,8 +25,8 @@ enum {
 };
 
 /* Callback for vpd_decode_string to invoke. */
-typedef int vpd_decode_callback(const u8 *key, s32 key_len,
-				const u8 *value, s32 value_len,
+typedef int vpd_decode_callback(const u8 *key, u32 key_len,
+				const u8 *value, u32 value_len,
 				void *arg);
 
 /*
@@ -44,7 +44,7 @@ typedef int vpd_decode_callback(const u8 *key, s32 key_len,
  * If one entry is successfully decoded, sends it to callback and returns the
  * result.
  */
-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);
 
 #endif  /* __VPD_DECODE_H */
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH v4] firmware: google: check if size is valid when decoding VPD data

[Stephen Boyd]

Quoting Hung-Te Lin (2019-08-29 19:23:58)
> The VPD implementation from Chromium Vital Product Data project used to
> parse data from untrusted input without checking if the meta data is
> invalid or corrupted. For example, the size from decoded content may
> be negative value, or larger than whole input buffer. Such invalid data
> may cause buffer overflow.
> 
> To fix that, the size parameters passed to vpd_decode functions should
> be changed to unsigned integer (u32) type, and the parsing of entry
> header should be refactored so every size field is correctly verified
> before starting to decode.
> 
> Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
> Signed-off-by: Hung-Te Lin <hungte@chromium.org>

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4] firmware: google: check if size is valid when decoding VPD data

[Guenter Roeck]

On Fri, Aug 30, 2019 at 10:23:58AM +0800, Hung-Te Lin wrote:
> The VPD implementation from Chromium Vital Product Data project used to
> parse data from untrusted input without checking if the meta data is
> invalid or corrupted. For example, the size from decoded content may
> be negative value, or larger than whole input buffer. Such invalid data
> may cause buffer overflow.
> 
> To fix that, the size parameters passed to vpd_decode functions should
> be changed to unsigned integer (u32) type, and the parsing of entry
> header should be refactored so every size field is correctly verified
> before starting to decode.
> 
> Fixes: ad2ac9d5c5e0 ("firmware: Google VPD: import lib_vpd source files")
> Signed-off-by: Hung-Te Lin <hungte@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Changes in v4:
> - Prevent changing indent in function prototype
> - Removed changes in function comments
> 
>  drivers/firmware/google/vpd.c        |  4 +-
>  drivers/firmware/google/vpd_decode.c | 55 ++++++++++++++++------------
>  drivers/firmware/google/vpd_decode.h |  6 +--
>  3 files changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index 0739f3b70347..db0812263d46 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -92,8 +92,8 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
>  	return VPD_OK;
>  }
>  
> -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;
> diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
> index 92e3258552fc..dda525c0f968 100644
> --- a/drivers/firmware/google/vpd_decode.c
> +++ b/drivers/firmware/google/vpd_decode.c
> @@ -9,8 +9,8 @@
>  
>  #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, u32 *decoded_len)
>  {
>  	u8 more;
>  	int i = 0;
> @@ -30,18 +30,39 @@ static int vpd_decode_len(const s32 max_len, const u8 *in,
>  	} while (more);
>  
>  	*decoded_len = i;
> +	return VPD_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;
> +	u32 consumed = *_consumed;
> +
> +	if (vpd_decode_len(max_len - consumed, &input_buf[consumed],
> +			   entry_len, &decoded_len) != VPD_OK)
> +		return VPD_FAIL;
> +	if (max_len - consumed < decoded_len)
> +		return VPD_FAIL;
> +
> +	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_FAIL;
>  
> +	consumed += decoded_len;
> +	*_consumed = consumed;
>  	return VPD_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;
>  
> @@ -56,26 +77,14 @@ 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)
> +		if (vpd_decode_entry(max_len, input_buf, consumed, &key,
> +				     &key_len) != VPD_OK)
>  			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)
> +		if (vpd_decode_entry(max_len, input_buf, consumed, &value,
> +				     &value_len) != VPD_OK)
>  			return VPD_FAIL;
>  
> -		*consumed += decoded_len;
> -		value = &input_buf[*consumed];
> -		*consumed += value_len;
> -
>  		if (type == VPD_TYPE_STRING)
>  			return callback(key, key_len, value, value_len,
>  					callback_arg);
> diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/google/vpd_decode.h
> index cf8c2ace155a..8dbe41cac599 100644
> --- a/drivers/firmware/google/vpd_decode.h
> +++ b/drivers/firmware/google/vpd_decode.h
> @@ -25,8 +25,8 @@ enum {
>  };
>  
>  /* Callback for vpd_decode_string to invoke. */
> -typedef int vpd_decode_callback(const u8 *key, s32 key_len,
> -				const u8 *value, s32 value_len,
> +typedef int vpd_decode_callback(const u8 *key, u32 key_len,
> +				const u8 *value, u32 value_len,
>  				void *arg);
>  
>  /*
> @@ -44,7 +44,7 @@ typedef int vpd_decode_callback(const u8 *key, s32 key_len,
>   * If one entry is successfully decoded, sends it to callback and returns the
>   * result.
>   */
> -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);
>  
>  #endif  /* __VPD_DECODE_H */
> -- 
> 2.23.0.187.g17f5b7556c-goog
>