| Message ID | YcnFiGzk67p0PSgd@b-10-27-92-143.dynapool.vpn.nyu.edu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rsi: fix oob in rsi_prepare_skb | expand |
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?
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.
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,
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(+)