[PATCH] rsi: fix oob in rsi_prepare_skb

[PATCH] rsi: fix oob in rsi_prepare_skb

[Zekun Shen]

We found this bug while fuzzing the rsi_usb driver.
rsi_prepare_skb does not check for OOB memcpy. We
add the check in the caller to fix.

Although rsi_prepare_skb checks if length is larger
than (4 * RSI_RCV_BUFFER_LEN), it really can't because
length is 0xfff maximum. So the check in patch is sufficient.

This patch is created upon ath-next branch. It is
NOT tested with real device, but with QEMU emulator.

Following is the bug report

BUG: KASAN: use-after-free in rsi_read_pkt
(/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
Read of size 3815 at addr ffff888031da736d by task RX-Thread/204

CPU: 0 PID: 204 Comm: RX-Thread Not tainted 5.6.0 #5
Call Trace:
dump_stack (/linux/lib/dump_stack.c:120)
 ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
 print_address_description.constprop.6 (/linux/mm/kasan/report.c:377)
 ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
 ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
 __kasan_report.cold.9 (/linux/mm/kasan/report.c:510)
 ? syscall_return_via_sysret (/linux/arch/x86/entry/entry_64.S:253)
 ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
 kasan_report (/linux/arch/x86/include/asm/smap.h:69 /linux/mm/kasan/common.c:644)
 check_memory_region (/linux/mm/kasan/generic.c:186 /linux/mm/kasan/generic.c:192)
 memcpy (/linux/mm/kasan/common.c:130)
 rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
 ? skb_dequeue (/linux/net/core/skbuff.c:3042)
 rsi_usb_rx_thread (/linux/drivers/net/wireless/rsi/rsi_91x_usb_ops.c:47) rsi_usb

Reported-by: Brendan Dolan-Gavitt <brendandg@nyu.edu>
Signed-off-by: Zekun Shen <bruceshenzk@gmail.com>
---
 drivers/net/wireless/rsi/rsi_91x_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/rsi/rsi_91x_main.c b/drivers/net/wireless/rsi/rsi_91x_main.c
index 5d1490fc3..41d3c12e0 100644
--- a/drivers/net/wireless/rsi/rsi_91x_main.c
+++ b/drivers/net/wireless/rsi/rsi_91x_main.c
@@ -193,6 +193,10 @@ int rsi_read_pkt(struct rsi_common *common, u8 *rx_pkt, s32 rcv_pkt_len)
 			break;
 
 		case RSI_WIFI_DATA_Q:
+			if (!rcv_pkt_len && offset + length >
+				RSI_MAX_RX_USB_PKT_SIZE)
+				goto fail;
+
 			skb = rsi_prepare_skb(common,
 					      (frame_desc + offset),
 					      length,
-- 
2.25.1

Re: [PATCH] rsi: fix oob in rsi_prepare_skb

[Kalle Valo]

Zekun Shen <bruceshenzk@gmail.com> writes:

> We found this bug while fuzzing the rsi_usb driver.
> rsi_prepare_skb does not check for OOB memcpy. We
> add the check in the caller to fix.
>
> Although rsi_prepare_skb checks if length is larger
> than (4 * RSI_RCV_BUFFER_LEN), it really can't because
> length is 0xfff maximum. So the check in patch is sufficient.
>
> This patch is created upon ath-next branch. It is
> NOT tested with real device, but with QEMU emulator.
>
> Following is the bug report
>
> BUG: KASAN: use-after-free in rsi_read_pkt
> (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
> Read of size 3815 at addr ffff888031da736d by task RX-Thread/204
>
> CPU: 0 PID: 204 Comm: RX-Thread Not tainted 5.6.0 #5
> Call Trace:
> dump_stack (/linux/lib/dump_stack.c:120)
>  ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
>  print_address_description.constprop.6 (/linux/mm/kasan/report.c:377)
>  ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
>  ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
>  __kasan_report.cold.9 (/linux/mm/kasan/report.c:510)
>  ? syscall_return_via_sysret (/linux/arch/x86/entry/entry_64.S:253)
>  ? rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
>  kasan_report (/linux/arch/x86/include/asm/smap.h:69 /linux/mm/kasan/common.c:644)
>  check_memory_region (/linux/mm/kasan/generic.c:186 /linux/mm/kasan/generic.c:192)
>  memcpy (/linux/mm/kasan/common.c:130)
>  rsi_read_pkt (/linux/drivers/net/wireless/rsi/rsi_91x_main.c:206) rsi_91x
>  ? skb_dequeue (/linux/net/core/skbuff.c:3042)
>  rsi_usb_rx_thread (/linux/drivers/net/wireless/rsi/rsi_91x_usb_ops.c:47) rsi_usb
>
> Reported-by: Brendan Dolan-Gavitt <brendandg@nyu.edu>
> Signed-off-by: Zekun Shen <bruceshenzk@gmail.com>
> ---
>  drivers/net/wireless/rsi/rsi_91x_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_main.c b/drivers/net/wireless/rsi/rsi_91x_main.c
> index 5d1490fc3..41d3c12e0 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_main.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_main.c
> @@ -193,6 +193,10 @@ int rsi_read_pkt(struct rsi_common *common, u8 *rx_pkt, s32 rcv_pkt_len)
>  			break;
>  
>  		case RSI_WIFI_DATA_Q:
> +			if (!rcv_pkt_len && offset + length >
> +				RSI_MAX_RX_USB_PKT_SIZE)
> +				goto fail;
> +
>  			skb = rsi_prepare_skb(common,
>  					      (frame_desc + offset),
>  					      length,

Why are you doing the check here? In the beginning of the function we
have:

		frame_desc = &rx_pkt[index];
		actual_length = *(u16 *)&frame_desc[0];
		offset = *(u16 *)&frame_desc[2];
		if (!rcv_pkt_len && offset >
			RSI_MAX_RX_USB_PKT_SIZE - FRAME_DESC_SZ)
			goto fail;

Wouldn't it make more sense to fix that check?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] rsi: fix oob in rsi_prepare_skb

[Zekun Shen]

The maximum length allowed (and without overflow) depends on
the queueno in the switch statement. I don't know the exact format
of the inputs, but there could be a universal and stricter length
restriction in the protocol

It is possible to fix the problem at the previous check you propose,
we just need to add input parsing for length and queueno there.

The code here seems prone to overflow, since function arguments
only include a single buffer pointer without a remaining byte count.
Moreover, some of the lengths are dynamic and encoded in the
buffer.

For this reason, I think it's easier and more maintainable to add the
check after existing parsing code and before read/write the buffer.