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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
>
