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@xxxxxxxxxxxx>

Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

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