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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d67e673.1c69fb81.5f13b.62ee@mx.google.com>
Date:   Thu, 29 Aug 2019 07:51:30 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Hung-Te Lin <hungte@...omium.org>
Cc:     hungte@...omium.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexios Zavras <alexios.zavras@...el.com>,
        Colin Ian King <colin.king@...onical.com>,
        Samuel Holland <samuel@...lland.org>,
        Allison Randal <allison@...utok.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] firmware: google: check if size is valid when decoding VPD data

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@...omium.org>
> ---

Two minor nitpicks, otherwise

Reviewed-by: Stephen Boyd <swboyd@...omium.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.