Change in ...coreboot[master]: src/driver/vpd: Check decoded content length before invoking callback

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33016

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

src/driver/vpd: Check decoded content length before invoking callback

When decoding, the content (key or value) part should be also checked
before we jump into callback because the callback function has no idea
if the contents are all valid.

BUG=chromium:967209
TEST=make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
1 file changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/1

diff --git a/src/drivers/vpd/lib_vpd.c b/src/drivers/vpd/lib_vpd.c
index 0744a71..f235a82 100644
--- a/src/drivers/vpd/lib_vpd.c
+++ b/src/drivers/vpd/lib_vpd.c
@@ -79,8 +79,8 @@
    	if (VPD_OK != decodeLen(max_len - *consumed,
    				&input_buf[*consumed], &key_len,
    				&decoded_len) ||
-				*consumed + decoded_len >= max_len) {
-				return VPD_FAIL;
+		    *consumed + decoded_len + key_len >= max_len) {
+			return VPD_FAIL;
    	}

*consumed += decoded_len;
@@ -91,8 +91,8 @@
    	if (VPD_OK != decodeLen(max_len - *consumed,
    				&input_buf[*consumed],
    				&value_len, &decoded_len) ||
-					*consumed + decoded_len > max_len) {
-					return VPD_FAIL;
+		    *consumed + decoded_len + value_len > max_len) {
+			return VPD_FAIL;
    	}
    	*consumed += decoded_len;
    	value = &input_buf[*consumed];

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#2).

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

src/driver/vpd: Check decoded content length before invoking callback

When decoding, the content (key or value) part should be also checked
before we jump into callback because the callback function has no idea
if the contents are all valid.

The key cannot be empty while value can hold empty contents.

BUG=chromium:967209
TEST=make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/2

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c 
File src/drivers/vpd/lib_vpd.c:

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@82 
PS2, Line 82: <=
Just wondering, is there any good reason for key_len to be signed?

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@83 
PS2, Line 83: decoded_len
If decoded_len is signed, we might also want to fail on decoded_len < 0 here?

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@96 
PS2, Line 96: >
Why is this >= above, but > here?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c 
File src/drivers/vpd/lib_vpd.c:

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@82 
PS2, Line 82: <=
> Just wondering, is there any good reason for key_len to be signed?

no, I think it's just because the library was created in that way...

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@83 
PS2, Line 83: decoded_len
> If decoded_len is signed, we might also want to fail on decoded_len < 0 here?

decode_len is almost impossible to be <0 since it's the accumulated value by looping within max_len. Since max_len is signed then decoded_len will never be over range of signed.

https://review.coreboot.org/#/c/33016/2/src/drivers/vpd/lib_vpd.c@96 
PS2, Line 96: >
> Why is this >= above, but > here?

because key_len >= 1 while value_len >= 0.

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

Patch Set 2: Code-Review-1

Actually, I think we should make these changes in platform/vpd, and then push them back upstream into this coreboot driver.  It doesn't make sense for these two pieces of code to diverge.

Alternatively, we could consider relocating platform/vpd into platform/vboot_reference, and then just linking in the appropriate functions for the coreboot driver.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Check decoded content length before invoking callback
......................................................................

Patch Set 2:
> Actually, I think we should make these changes in platform/vpd

I do have same fix in platform/vpd under review.
But yes, we've seen similar comments there for why it's int instead of uint.
I'll finish platform/vpd review first and then upstream here.
> Alternatively, we could consider relocating platform/vpd into platform/vboot_reference

Probably no. vboot is nothing related (and should not) to vpd.

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#3).

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
M src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
3 files changed, 77 insertions(+), 53 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/3

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 3:

(8 comments)

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h 
File src/drivers/vpd/lib_vpd.h:

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h@96 
PS3, Line 96:     const uint32_t max_len,
please, no spaces at the start of a line

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h@98 
PS3, Line 98:     uint32_t *length,
please, no spaces at the start of a line

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h@99 
PS3, Line 99:     uint32_t *decoded_len);
please, no spaces at the start of a line

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h@155 
PS3, Line 155:     const uint32_t max_len,
please, no spaces at the start of a line

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.h@157 
PS3, Line 157:     uint32_t *consumed,
please, no spaces at the start of a line

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.c 
File src/drivers/vpd/lib_vpd.c:

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.c@88 
PS3, Line 88: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.c@104 
PS3, Line 104: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/3/src/drivers/vpd/lib_vpd.c@122 
PS3, Line 122: 		break;
break is not useful after a goto or return

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#4).

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
M src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
3 files changed, 81 insertions(+), 57 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/4

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/#/c/33016/4/src/drivers/vpd/lib_vpd.c 
File src/drivers/vpd/lib_vpd.c:

https://review.coreboot.org/#/c/33016/4/src/drivers/vpd/lib_vpd.c@88 
PS4, Line 88: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/4/src/drivers/vpd/lib_vpd.c@104 
PS4, Line 104: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/4/src/drivers/vpd/lib_vpd.c@122 
PS4, Line 122: 		break;
break is not useful after a goto or return

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#5).

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
M src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
3 files changed, 153 insertions(+), 97 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/5

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/#/c/33016/5/src/drivers/vpd/lib_vpd.c 
File src/drivers/vpd/lib_vpd.c:

https://review.coreboot.org/#/c/33016/5/src/drivers/vpd/lib_vpd.c@88 
PS5, Line 88: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/5/src/drivers/vpd/lib_vpd.c@104 
PS5, Line 104: 		if (VPD_OK != retval)
Comparisons should place the constant on the right side of the test

https://review.coreboot.org/#/c/33016/5/src/drivers/vpd/lib_vpd.c@122 
PS5, Line 122: 		break;
break is not useful after a goto or return

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#6).

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/lib_vpd.c
M src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
3 files changed, 152 insertions(+), 97 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/6

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 6:

I guess we can make the fixes for Jenkins' suggestions back in CrOS repo?

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 6:
> I guess we can make the fixes for Jenkins' suggestions back in CrOS repo?

I thought I did?

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/33016/6/src/drivers/vpd/vpd.c 
File src/drivers/vpd/vpd.c:

https://review.coreboot.org/#/c/33016/6/src/drivers/vpd/vpd.c@163 
PS6, Line 163: int
This should be vpd_err_t

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hello Julius Werner, You-Cheng Syu, build bot (Jenkins), Joel Kitching,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33016

to look at the new patch set (#7).

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
---
M src/drivers/vpd/Makefile.inc
D src/drivers/vpd/lib_vpd.c
D src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
A src/drivers/vpd/vpd_decode.c
A src/drivers/vpd/vpd_decode.h
6 files changed, 184 insertions(+), 360 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33016/7

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 7:

uploaded a new version, with almost zero change (except the inttypes include) to the upstream.

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

src/driver/vpd: Update lib_vpd from upstream

Update lib_vpd.c (only containing vpd_decode.c) to latest version from
https://chromium.googlesource.com/chromiumos/platform/vpd

The called module (vpd.c) has been also corrected for new lib_vpd
types and constants.

BUG=chromium:967209
TEST=select VPD config on kukui; make; boots on at least kukui boards.

Change-Id: I3928e9c43cb87caf93fb44ee10434ce80f0a188a
Signed-off-by: Hung-Te Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33016
Tested-by: build bot (Jenkins) 
Reviewed-by: Joel Kitching 
---
M src/drivers/vpd/Makefile.inc
D src/drivers/vpd/lib_vpd.c
D src/drivers/vpd/lib_vpd.h
M src/drivers/vpd/vpd.c
A src/drivers/vpd/vpd_decode.c
A src/drivers/vpd/vpd_decode.h
6 files changed, 184 insertions(+), 360 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Joel Kitching: Looks good to me, approved

diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc
index 91c069d..17019b5 100644
--- a/src/drivers/vpd/Makefile.inc
+++ b/src/drivers/vpd/Makefile.inc
@@ -1,2 +1,2 @@
-romstage-$(CONFIG_VPD) += lib_vpd.c
-ramstage-$(CONFIG_VPD) += vpd.c lib_vpd.c
+romstage-$(CONFIG_VPD) += vpd_decode.c
+ramstage-$(CONFIG_VPD) += vpd.c vpd_decode.c
diff --git a/src/drivers/vpd/lib_vpd.c b/src/drivers/vpd/lib_vpd.c
deleted file mode 100644
index 0744a71..0000000
--- a/src/drivers/vpd/lib_vpd.c
+++ /dev/null
@@ -1,113 +0,0 @@
-/*
- * Copyright (c) 2014 The Chromium OS Authors. All rights reserved.
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- *
- */
-#include <assert.h>
-#include "lib_vpd.h"
-
-/* Given an encoded string, this functions decodes the length field which varies
- * from 1 byte to many bytes.
- *
- * The in points the actual byte going to be decoded. The *length returns
- * the decoded length field. The number of consumed bytes will be stroed in
- * decoded_len.
- *
- * Returns VPD_FAIL if more bit is 1, but actually reaches the end of string.
- */
-int decodeLen(const int32_t max_len,
-	      const uint8_t *in,
-	      int32_t *length,
-	      int32_t *decoded_len)
-{
-	uint8_t more;
-	int i = 0;
-
-	assert(length);
-	assert(decoded_len);
-
-	*length = 0;
-	do {
-		if (i >= max_len)
-			return VPD_FAIL;
-
-		more = in[i] & 0x80;
-		*length <<= 7;
-		*length |= in[i] & 0x7f;
-		++i;
-	} while (more);
-
-	*decoded_len = i;
-
-	return VPD_OK;
-}
-
-/* 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.
- *
- * The input_buf points to the first byte of the input buffer.
- *
- * The *consumed starts from 0, which is actually the next byte to be decoded.
- * It can be non-zero to be used in multiple calls.
- *
- * If one entry is successfully decoded, sends it to callback and returns the
- * result.
- */
-int decodeVpdString(const int32_t max_len,
-		    const uint8_t *input_buf,
-		    int32_t *consumed,
-		    VpdDecodeCallback callback,
-		    void *callback_arg)
-{
-	int type;
-	int32_t key_len, value_len;
-	int32_t decoded_len;
-	const uint8_t *key, *value;
-
-	/* type */
-	if (*consumed >= max_len)
-		return VPD_FAIL;
-
-	type = input_buf[*consumed];
-	switch (type) {
-	case VPD_TYPE_INFO:
-	case VPD_TYPE_STRING:
-		(*consumed)++;
-		/* key */
-		if (VPD_OK != decodeLen(max_len - *consumed,
-					&input_buf[*consumed], &key_len,
-					&decoded_len) ||
-				*consumed + decoded_len >= max_len) {
-				return VPD_FAIL;
-		}
-
-		*consumed += decoded_len;
-		key = &input_buf[*consumed];
-		*consumed += key_len;
-
-		/* value */
-		if (VPD_OK != decodeLen(max_len - *consumed,
-					&input_buf[*consumed],
-					&value_len, &decoded_len) ||
-					*consumed + decoded_len > max_len) {
-					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);
-
-		return VPD_OK;
-
-	default:
-		return VPD_FAIL;
-	break;
-	}
-
-	return VPD_OK;
-}
diff --git a/src/drivers/vpd/lib_vpd.h b/src/drivers/vpd/lib_vpd.h
deleted file mode 100644
index 156d279..0000000
--- a/src/drivers/vpd/lib_vpd.h
+++ /dev/null
@@ -1,226 +0,0 @@
-/*
- * Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- *
- */
-
-#ifndef __LIB_VPD__
-#define __LIB_VPD__
-
-#include <inttypes.h>
-
-enum {
-	VPD_OK = 0,
-	VPD_FAIL,
-};
-
-enum {
-	VPD_TYPE_TERMINATOR = 0,
-	VPD_TYPE_STRING,
-	VPD_TYPE_INFO                = 0xfe,
-	VPD_TYPE_IMPLICIT_TERMINATOR = 0xff,
-};
-
-enum {
-	VPD_AS_LONG_AS = -1,
-};
-
-enum {  /* export_type */
-	VPD_EXPORT_KEY_VALUE = 1,
-	VPD_EXPORT_VALUE,
-	VPD_EXPORT_AS_PARAMETER,
-};
-
-/* Callback for decodeVpdString to invoke. */
-typedef int VpdDecodeCallback(const uint8_t *key, int32_t key_len,
-			      const uint8_t *value, int32_t value_len,
-			      void *arg);
-
-/* Container data types */
-struct StringPair {
-	uint8_t *key;
-	uint8_t *value;
-	int pad_len;
-	int filter_out;  /* TRUE means not exported. */
-	struct StringPair *next;
-};
-
-struct PairContainer {
-	struct StringPair *first;
-};
-
-
-/***********************************************************************
- * Encode and decode VPD entries
- ***********************************************************************/
-
-/* Encodes the len into multiple bytes with the following format.
- *
- *    7   6 ............ 0
- *  +----+------------------+
- *  |More|      Length      |  ...
- *  +----+------------------+
- *
- * The encode_buf points to the actual position we are going to store.
- * encoded_len will return the exact bytes we encoded in this function.
- * Returns fail if the buffer is not long enough.
- */
-int encodeLen(
-    const int32_t len,
-    uint8_t *encode_buf,
-    const int32_t max_len,
-    int32_t *encoded_len);
-
-/* Given an encoded string, this functions decodes the length field which varies
- * from 1 byte to many bytes.
- *
- * The in points the actual byte going to be decoded. The *length returns
- * the decoded length field. The number of consumed bytes will be stroed in
- * decoded_len.
- *
- * Returns VPD_FAIL if more bit is 1, but actually reaches the end of string.
- */
-int decodeLen(
-    const int32_t max_len,
-    const uint8_t *in,
-    int32_t *length,
-    int32_t *decoded_len);
-
-
-/* Encodes the terminator.
- * When calling, the output_buf should point to the start of buffer while
- * *generated_len should contain how many bytes exist in buffer now.
- * After return, *generated_len would be plused the number of bytes generated
- * in this function.
- */
-int encodeVpdTerminator(
-    const int max_buffer_len,
-    uint8_t *output_buf,
-    int *generated_len);
-
-/* Encodes the given type/key/value pair into buffer.
- *
- * The pad_value_len is used to control the output value length.
- * When pad_value_len > 0, the value is outputted into fixed length (pad \0
- *                         if the value is shorter). Truncated if too long.
- *      pad_value_len == VPD_NO_LIMIT, output the value as long as possible.
- * This is useful when we want to fix the structure layout at beginning.
- *
- * The encoded string will be stored in output_buf. If it is longer than
- * max_buffer_len, this function returns fail. If the buffer is long enough,
- * the generated_len will be updated.
- *
- * When calling, the output_buf should point to the start of buffer while
- * *generated_len should contain how many bytes exist in buffer now.
- * After return, *generated_len would be plused the number of bytes generated
- * in this function.
- *
- * The initial value of *generated_len can be non-zero, so that this value
- * can be used between multiple calls to encodeVpd2Pair().
- */
-int encodeVpdString(
-    const uint8_t *key,
-    const uint8_t *value,
-    const int pad_value_len,
-    const int max_buffer_len,
-    uint8_t *output_buf,
-    int *generated_len);
-
-
-/* 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.
- *
- * The input_buf points to the first byte of the input buffer.
- *
- * The *consumed starts from 0, which is actually the next byte to be decoded.
- * It can be non-zero to be used in multiple calls.
- *
- * If one entry is successfully decoded, sends it to callback and returns the
- * result.
- */
-int decodeVpdString(
-    const int32_t max_len,
-    const uint8_t *input_buf,
-    int32_t *consumed,
-    VpdDecodeCallback callback,
-    void *callback_arg);
-
-/***********************************************************************
- * Container helpers
- ***********************************************************************/
-void initContainer(struct PairContainer *container);
-
-struct StringPair *findString(struct PairContainer *container,
-			      const uint8_t *key,
-			      struct StringPair ***prev_next);
-
-/* If key is already existed in container, its value will be replaced.
- * If not existed, creates new entry in container.
- */
-void setString(struct PairContainer *container,
-	       const uint8_t *key,
-	       const uint8_t *value,
-	       const int pad_len);
-
-/* merge all entries in src into dst. If key is duplicate, overwrite it.
- */
-void mergeContainer(struct PairContainer *dst,
-		    const struct PairContainer *src);
-
-/* subtract src from dst.
-*/
-int subtractContainer(struct PairContainer *dst,
-		      const struct PairContainer *src);
-
-/* Given a container, encode its all entries into the buffer.
- */
-int encodeContainer(const struct PairContainer *container,
-		    const int max_buf_len,
-		    uint8_t *buf,
-		    int *generated);
-
-/* Given a VPD blob, decode its entries and push into container.
- */
-int decodeToContainer(struct PairContainer *container,
-		      const int32_t max_len,
-		      const uint8_t *input_buf,
-		      int32_t *consumed);
-
-/* Set filter for exporting functions.
- * If filter is NULL, resets the filter so that everything can be exported.
- */
-int setContainerFilter(struct PairContainer *container,
-		       const uint8_t *filter);
-
-/*
- * Remove a key.
- * Returns VPD_OK if deleted successfully. Otherwise, VPD_FAIL.
- */
-int deleteKey(struct PairContainer *container,
-	      const uint8_t *key);
-
-/*
- * Returns number of pairs in container.
- */
-int lenOfContainer(const struct PairContainer *container);
-
-/*
- * Export the container content with human-readable text.
- *
- * The buf points to the first byte of buffer and *generated contains the number
- * of bytes already existed in buffer.
- *
- * Afterward, the *generated will be plused on exact bytes this function has
- * generated.
- */
-int exportContainer(const int export_type,
-		    const struct PairContainer *container,
-		    const int max_buf_len,
-		    uint8_t *buf,
-		    int *generated);
-
-void destroyContainer(struct PairContainer *container);
-
-#endif  /* __LIB_VPD__ */
diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c
index e620b58..c6dd339 100644
--- a/src/drivers/vpd/vpd.c
+++ b/src/drivers/vpd/vpd.c
@@ -12,7 +12,7 @@
 #include <timestamp.h>

#include "vpd.h"
-#include "lib_vpd.h"
+#include "vpd_decode.h"
 #include "vpd_tables.h"

/* Currently we only support Google VPD 2.0, which has a fixed offset. */
@@ -160,27 +160,27 @@
    }
 }

-static int vpd_gets_callback(const uint8_t *key, int32_t key_len,
-			       const uint8_t *value, int32_t value_len,
-			       void *arg)
+static int vpd_gets_callback(const uint8_t *key, uint32_t key_len,
+			     const uint8_t *value, uint32_t value_len,
+			     void *arg)
 {
    struct vpd_gets_arg *result = (struct vpd_gets_arg *)arg;
    if (key_len != result->key_len ||
        memcmp(key, result->key, key_len) != 0)
-		/* Returns VPD_OK to continue parsing. */
-		return VPD_OK;
+		/* Returns VPD_DECODE_OK to continue parsing. */
+		return VPD_DECODE_OK;

result->matched = 1;
    result->value = value;
    result->value_len = value_len;
-	/* Returns VPD_FAIL to stop parsing. */
-	return VPD_FAIL;
+	/* Returns VPD_DECODE_FAIL to stop parsing. */
+	return VPD_DECODE_FAIL;
 }

const void *vpd_find(const char *key, int *size, enum vpd_region region)
 {
    struct vpd_gets_arg arg = {0};
-	int consumed = 0;
+	uint32_t consumed = 0;
    const struct vpd_cbmem *vpd;

vpd = cbmem_find(CBMEM_ID_VPD);
@@ -190,18 +190,21 @@
    arg.key = (const uint8_t *)key;
    arg.key_len = strlen(key);

-	if (region == VPD_ANY || region == VPD_RO)
-		while (VPD_OK == decodeVpdString(vpd->ro_size, vpd->blob,
-		       &consumed, vpd_gets_callback, &arg)) {
-		/* Iterate until found or no more entries. */
+	if (region == VPD_ANY || region == VPD_RO) {
+		while (vpd_decode_string(
+				vpd->ro_size, vpd->blob, &consumed,
+				vpd_gets_callback, &arg) == VPD_DECODE_OK) {
+			/* Iterate until found or no more entries. */
    	}
-
-	if (!arg.matched && region != VPD_RO)
-		while (VPD_OK == decodeVpdString(vpd->rw_size,
-		       vpd->blob + vpd->ro_size, &consumed,
-		       vpd_gets_callback, &arg)) {
-		/* Iterate until found or no more entries. */
+	}
+	if (!arg.matched && region != VPD_RO) {
+		while (vpd_decode_string(
+				vpd->rw_size, vpd->blob + vpd->ro_size,
+				&consumed, vpd_gets_callback,
+				&arg) == VPD_DECODE_OK) {
+			/* Iterate until found or no more entries. */
    	}
+	}

if (!arg.matched)
    	return NULL;
diff --git a/src/drivers/vpd/vpd_decode.c b/src/drivers/vpd/vpd_decode.c
new file mode 100644
index 0000000..0eab704
--- /dev/null
+++ b/src/drivers/vpd/vpd_decode.c
@@ -0,0 +1,92 @@
+/*
+ * Copyright 2014 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ *
+ * This is a copy from upstream:
+ * https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/lib/vpd_d...
+ */
+#include "vpd_decode.h"
+
+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_DECODE_FAIL;
+
+	*length = 0;
+	do {
+		if (i >= max_len)
+			return VPD_DECODE_FAIL;
+
+		more = in[i] & 0x80;
+		*length <<= 7;
+		*length |= in[i] & 0x7f;
+		++i;
+	} while (more);
+
+	*decoded_len = i;
+	return VPD_DECODE_OK;
+}
+
+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;
+	u32 key_len;
+	u32 value_len;
+	u32 decoded_len;
+	const u8 *key;
+	const u8 *value;
+
+	/* type */
+	if (*consumed >= max_len)
+		return VPD_DECODE_FAIL;
+
+	type = input_buf[*consumed];
+
+	switch (type) {
+	case VPD_TYPE_INFO:
+	case VPD_TYPE_STRING:
+		(*consumed)++;
+
+		/* key */
+		res = vpd_decode_len(max_len - *consumed, &input_buf[*consumed],
+				     &key_len, &decoded_len);
+		/* key name cannot be empty, and must be followed by value. */
+		if (res != VPD_DECODE_OK || key_len < 1 ||
+		    *consumed + decoded_len + key_len >= max_len)
+			return VPD_DECODE_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);
+		/* value can be empty (value_len = 0). */
+		if (res != VPD_DECODE_OK ||
+		    *consumed + decoded_len + value_len > max_len)
+			return VPD_DECODE_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);
+		break;
+
+	default:
+		return VPD_DECODE_FAIL;
+	}
+
+	return VPD_DECODE_OK;
+}
diff --git a/src/drivers/vpd/vpd_decode.h b/src/drivers/vpd/vpd_decode.h
new file mode 100644
index 0000000..99ca7ef
--- /dev/null
+++ b/src/drivers/vpd/vpd_decode.h
@@ -0,0 +1,68 @@
+/*
+ * Copyright 2019 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ *
+ * This is a copy from upstream:
+ * https://chromium.googlesource.com/chromiumos/platform/vpd/+/master/include/l...
+ */
+
+#ifndef __VPD_DECODE_H
+#define __VPD_DECODE_H
+
+#include <stdint.h>
+
+enum {
+	VPD_DECODE_OK = 0,
+	VPD_DECODE_FAIL = 1,
+};
+
+enum {
+	VPD_TYPE_TERMINATOR = 0,
+	VPD_TYPE_STRING,
+	VPD_TYPE_INFO = 0xfe,
+	VPD_TYPE_IMPLICIT_TERMINATOR = 0xff,
+};
+
+/* Callback for vpd_decode_string to invoke. */
+typedef int vpd_decode_callback(
+		const u8 *key, u32 key_len, const u8 *value, u32 value_len,
+		void *arg);
+
+/*
+ * vpd_decode_len
+ *
+ * Given an encoded string, this function extracts the length of content
+ * (either key or value). The *consumed will contain the number of bytes
+ * consumed.
+ *
+ * The input_buf points to the first byte of the input buffer.
+ *
+ * The *consumed starts from 0, which is actually the next byte to be decoded.
+ * It can be non-zero to be used in multiple calls.
+ *
+ * Returns VPD_DECODE_OK on success, otherwise VPD_DECODE_FAIL.
+ */
+int vpd_decode_len(
+		const u32 max_len, const u8 *in, u32 *length, u32 *decoded_len);
+
+/*
+ * 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.
+ *
+ * The input_buf points to the first byte of the input buffer.
+ *
+ * The *consumed starts from 0, which is actually the next byte to be decoded.
+ * It can be non-zero to be used in multiple calls.
+ *
+ * If one entry is successfully decoded, sends it to callback and returns the
+ * result.
+ */
+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 */

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/#/c/33016/8/src/drivers/vpd/vpd_decode.c 
File src/drivers/vpd/vpd_decode.c:

https://review.coreboot.org/#/c/33016/8/src/drivers/vpd/vpd_decode.c@63 
PS8, Line 63: 		    *consumed + decoded_len + key_len >= max_len)
I think this can overflow? You should cast to u64 to be safe.

https://review.coreboot.org/#/c/33016/8/src/drivers/vpd/vpd_decode.c@75 
PS8, Line 75: 		    *consumed + decoded_len + value_len > max_len)
Same here.

Change in ...coreboot[master]: src/driver/vpd: Update lib_vpd from upstream

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33016 )

Change subject: src/driver/vpd: Update lib_vpd from upstream
......................................................................

Patch Set 8:

will update again when the upstream is fully reviewed.