Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33414
Change subject: src/driver/vpd: Update vpd_decode from upstream ......................................................................
src/driver/vpd: Update vpd_decode from upstream
The upstream vpd_decode.c has been revised to prevent overrun of decoded contents.
BUG=chromium:967209 TEST=select VPD config on kukui; make; boots on at least kukui boards.
Change-Id: I1a50670a66b7b174d2a432c29d90152b86c32982 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h 2 files changed, 31 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/33414/1
diff --git a/src/drivers/vpd/vpd_decode.c b/src/drivers/vpd/vpd_decode.c index 0eab704..e717972 100644 --- a/src/drivers/vpd/vpd_decode.c +++ b/src/drivers/vpd/vpd_decode.c @@ -8,7 +8,7 @@ */ #include "vpd_decode.h"
-int vpd_decode_len( +static int vpd_decode_len( const u32 max_len, const u8 *in, u32 *length, u32 *decoded_len) { u8 more; @@ -32,15 +32,36 @@ 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; + + *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 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;
@@ -55,33 +76,18 @@ 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) + if (vpd_decode_entry(max_len, input_buf, consumed, &key, + &key_len) != VPD_DECODE_OK) 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) + if (vpd_decode_entry(max_len, input_buf, consumed, &value, + &value_len) != VPD_DECODE_OK) 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: diff --git a/src/drivers/vpd/vpd_decode.h b/src/drivers/vpd/vpd_decode.h index 99ca7ef..5d595f3 100644 --- a/src/drivers/vpd/vpd_decode.h +++ b/src/drivers/vpd/vpd_decode.h @@ -30,28 +30,11 @@ 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. + * (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. *
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33414 )
Change subject: src/driver/vpd: Update vpd_decode from upstream ......................................................................
Patch Set 1: Code-Review+1
Hello Julius Werner, Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33414
to look at the new patch set (#2).
Change subject: src/driver/vpd: Update vpd_decode from upstream ......................................................................
src/driver/vpd: Update vpd_decode from upstream
The upstream vpd_decode.c has been revised to prevent overrun of decoded contents.
BUG=chromium:967209 TEST=select VPD config on kukui; make; boots on at least kukui boards.
Change-Id: I1a50670a66b7b174d2a432c29d90152b86c32982 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h 2 files changed, 30 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/33414/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33414 )
Change subject: src/driver/vpd: Update vpd_decode from upstream ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33414 )
Change subject: src/driver/vpd: Update vpd_decode from upstream ......................................................................
src/driver/vpd: Update vpd_decode from upstream
The upstream vpd_decode.c has been revised to prevent overrun of decoded contents.
BUG=chromium:967209 TEST=select VPD config on kukui; make; boots on at least kukui boards.
Change-Id: I1a50670a66b7b174d2a432c29d90152b86c32982 Signed-off-by: Hung-Te Lin hungte@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33414 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/drivers/vpd/vpd_decode.c M src/drivers/vpd/vpd_decode.h 2 files changed, 30 insertions(+), 42 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/drivers/vpd/vpd_decode.c b/src/drivers/vpd/vpd_decode.c index 0eab704..527c508 100644 --- a/src/drivers/vpd/vpd_decode.c +++ b/src/drivers/vpd/vpd_decode.c @@ -8,7 +8,7 @@ */ #include "vpd_decode.h"
-int vpd_decode_len( +static int vpd_decode_len( const u32 max_len, const u8 *in, u32 *length, u32 *decoded_len) { u8 more; @@ -32,15 +32,36 @@ 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; + + *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 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;
@@ -55,30 +76,14 @@ 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) + if (vpd_decode_entry(max_len, input_buf, consumed, &key, + &key_len) != VPD_DECODE_OK) 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) + if (vpd_decode_entry(max_len, input_buf, consumed, &value, + &value_len) != VPD_DECODE_OK) 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); diff --git a/src/drivers/vpd/vpd_decode.h b/src/drivers/vpd/vpd_decode.h index 99ca7ef..5d595f3 100644 --- a/src/drivers/vpd/vpd_decode.h +++ b/src/drivers/vpd/vpd_decode.h @@ -30,28 +30,11 @@ 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. + * (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. *
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)