Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f8471471-3b9f-526c-b8d0-88bd3de6d409@gmail.com>
Date:   Wed, 11 Jan 2023 20:12:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query_oid
To:     Alexander Duyck <alexander.duyck@gmail.com>
Cc:     kvalo@kernel.org, jussi.kivilinna@iki.fi, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        greg@kroah.com, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <CAKgT0UePq+Gg5mpvD7ag=ern9JN5JyAFv5RPc05Zn9jSh4W+0g@mail.gmail.com>
 <20230111175031.7049-1-szymon.heidrich@gmail.com>
 <CAKgT0UeiFGyttyQg_yWHA5L6ZPy9W8__b6DFSQe0-CNnLEvY7w@mail.gmail.com>
Content-Language: en-US
From:   Szymon Heidrich <szymon.heidrich@gmail.com>
In-Reply-To: <CAKgT0UeiFGyttyQg_yWHA5L6ZPy9W8__b6DFSQe0-CNnLEvY7w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/01/2023 19:28, Alexander Duyck wrote:
> On Wed, Jan 11, 2023 at 9:51 AM Szymon Heidrich
> <szymon.heidrich@gmail.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@gmail.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@fb.com>

Awesome, thank you very much.