Message-ID: <CAKgT0UePq+Gg5mpvD7ag=ern9JN5JyAFv5RPc05Zn9jSh4W+0g@mail.gmail.com>
Date:   Wed, 11 Jan 2023 07:47:39 -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,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Greg KH <greg@...ah.com>
Subject: Re: [PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

On Wed, Jan 11, 2023 at 1:54 AM Szymon Heidrich
<szymon.heidrich@...il.com> wrote:
>
> On 10/01/2023 20:39, Alexander H Duyck wrote:
> > On Tue, 2023-01-10 at 18:30 +0100, Szymon Heidrich 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>
> >> ---
> >>  drivers/net/wireless/rndis_wlan.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> >> index 82a7458e0..d7fc05328 100644
> >> --- a/drivers/net/wireless/rndis_wlan.c
> >> +++ b/drivers/net/wireless/rndis_wlan.c
> >> @@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
> >>              struct rndis_query_c    *get_c;
> >>      } u;
> >>      int ret, buflen;
> >> -    int resplen, respoffs, copylen;
> >> +    u32 resplen, respoffs, copylen;
> >
> > Rather than a u32 why not just make it an size_t? The advantage is that
> > is the native type for all the memory allocation and copying that takes
> > place in the function so it would avoid having to cast between u32 and
> > size_t.
>
> My sole intention with this patch was to address the exploitable overflow
> with minimal chance of introducing any extra issues.
> Sure some things probably could be done differently, but I would stick to
> the choices made by original authors of this driver, especially since Greg
> mentioned that RNDIS support generally should be dropped at some point.

My main concern was that your change will introduce a comparison
between signed and unsigned integer expressions. If you build with W=3
you should find that your changes add new warnings when they trigger
the "-Wsign-compare" check. Based on the comment earlier that you were
concerned about integer roll-over I thought that it might be good to
address that as well.

Basically my initial thought was that buflen should be a u32, but I
had opted to suggest size_t since that is the native type for the size
of memory regions in the kernel.

> > Also why not move buflen over to the unsigned integer category with the
> > other values you stated were at risk of overflow?
> >
> >>
> >>      buflen = *len + sizeof(*u.get);
> >>      if (buflen < CONTROL_BUFFER_SIZE)
> >
> > For example, this line here is comparing buflen to a fixed constant. If
> > we are concerned about overflows this could be triggering an integer
> > overflow resulting in truncation assuming *len is close to the roll-
> > over threshold.
>
> I'm not sure how this would be exploitable since len is controlled by the
> developer rather than potential attacker, at least in existing code. Please
> correct me in case I'm wrong.

The fact that w/ buflen signed and your other variables unsigned it
can lead to mix-ups between the comparisons below as it has to promote
one side or the other so that the types match before making the
comparison.

> > By converting to a size_t we would most likely end up blowing up on the
> > kmalloc and instead returning an -ENOMEM.
> >
> >> @@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
> >
> > Also with any type change such as this I believe you would also need to
> > update the netdev_dbg statement that displays respoffs and the like to
> > account for the fact that you are now using an unsigned value.
> > Otherwise I believe %d will display the value as a signed integer
> > value.
> >
> >>                      goto exit_unlock;
> >>              }
> >>
> >> -            if ((resplen + respoffs) > buflen) {
> >> +            if (resplen > (buflen - respoffs)) {
> >>                      /* Device would have returned more data if buffer would
> >>                       * have been big enough. Copy just the bits that we got.
> >>                       */
> >
> > Actually you should be able to simplfy this further. Assuming resplen,
> > buflen and respoffs all the same type this entire if statement could be
> > broken down into:
> >               copylen = min(resplen, buflen - respoffs);
> >
> >
>
> Agree, yet I would prefer to avoid any non-essential changes to keep the risk
> of introducing errors as low as possible. I intentionally refrained from any
> additional modifications. Is this acceptable?
>
> Thank you for your review, I really appreciate all the suggestions.

What I was getting at is that with this change the use of min should
result in almost exactly the same assembler code. If you look at the
min macro all it is doing is a comparison followed by an assignment,
and in your case you are working with only the two values "resplen"
and "buflen - respoffs" so it just saves space to make use of the
macro.

If you opt to not use the macro at a minimum you can get rid of the
parenthesis around "(buflen - respoffs)" since the order of operations
will complete the subtraction first before the comparison.

Powered by blists - more mailing lists