Re: [PATCH] rsi: fix oob in rsi_prepare_skb

From: Kalle Valo <kvalo@kernel.org>
To: Zekun Shen <bruceshenzk@gmail.com>
Cc: Amitkumar Karwar <amitkarwar@gmail.com>,
	Siva Rebbagondla <siva8118@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, brendandg@nyu.edu
Subject: Re: [PATCH] rsi: fix oob in rsi_prepare_skb
Date: Tue, 01 Feb 2022 14:10:31 +0200	[thread overview]
Message-ID: <87y22udbyg.fsf@kernel.org> (raw)
In-Reply-To: <YcnFiGzk67p0PSgd@b-10-27-92-143.dynapool.vpn.nyu.edu> (Zekun Shen's message of "Mon, 27 Dec 2021 08:54:16 -0500")

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