On 11/01/2023 19:28, Alexander Duyck wrote:
> On Wed, Jan 11, 2023 at 9:51 AM Szymon Heidrich
> <szymon.heidrich@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxx>
Awesome, thank you very much.
