linux-kernel - Re: [PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query

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]

Date:   Wed, 11 Jan 2023 10:28:12 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Szymon Heidrich <szymon.heidrich@...il.com>
Cc:     kvalo@...nel.org, jussi.kivilinna@....fi, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        greg@...ah.com, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query_oid

On Wed, Jan 11, 2023 at 9:51 AM Szymon Heidrich
<szymon.heidrich@...il.com> wrote:
>
> Since resplen and respoffs are signed integers sufficiently
> large values of unsigned int len and offset members of RNDIS
> response will result in negative values of prior variables.
> This may be utilized to bypass implemented security checks
> to either extract memory contents by manipulating offset or
> overflow the data buffer via memcpy by manipulating both
> offset and len.
>
> Additionally assure that sum of resplen and respoffs does not
> overflow so buffer boundaries are kept.
>
> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
> Signed-off-by: Szymon Heidrich <szymon.heidrich@...il.com>
> ---
> V1 -> V2: Use size_t and min macro, fix netdev_dbg format
>
>  drivers/net/wireless/rndis_wlan.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> index 82a7458e0..bf72e5fd3 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -696,8 +696,8 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>                 struct rndis_query      *get;
>                 struct rndis_query_c    *get_c;
>         } u;
> -       int ret, buflen;
> -       int resplen, respoffs, copylen;
> +       int ret;
> +       size_t buflen, resplen, respoffs, copylen;
>
>         buflen = *len + sizeof(*u.get);
>         if (buflen < CONTROL_BUFFER_SIZE)
> @@ -732,22 +732,15 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>
>                 if (respoffs > buflen) {
>                         /* Device returned data offset outside buffer, error. */
> -                       netdev_dbg(dev->net, "%s(%s): received invalid "
> -                               "data offset: %d > %d\n", __func__,
> -                               oid_to_string(oid), respoffs, buflen);
> +                       netdev_dbg(dev->net,
> +                                  "%s(%s): received invalid data offset: %zu > %zu\n",
> +                                  __func__, oid_to_string(oid), respoffs, buflen);
>
>                         ret = -EINVAL;
>                         goto exit_unlock;
>                 }
>
> -               if ((resplen + respoffs) > buflen) {
> -                       /* Device would have returned more data if buffer would
> -                        * have been big enough. Copy just the bits that we got.
> -                        */
> -                       copylen = buflen - respoffs;
> -               } else {
> -                       copylen = resplen;
> -               }
> +               copylen = min(resplen, buflen - respoffs);
>
>                 if (copylen > *len)
>                         copylen = *len;

Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>