2015-05-19 05:47:21

by Haggai Eran

[permalink] [raw]
Subject: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
packet are set, but the packet length is too large. Truncate the packet
to prevent memory corruption.

Signed-off-by: Haggai Eran <[email protected]>
---

Hi,

I think this solves the issue for me. I'll test it more thoroughly later. I
still don't know why a fragmented packet has such a large pkt_len value though.

Thanks,
Haggai

drivers/staging/rtl8712/rtl8712_recv.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index cd8b444..d7ea9c1 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -1055,8 +1055,12 @@ static int recvbuf2recvframe(struct _adapter *padapter, struct sk_buff *pskb)
pkt_offset = (u16)round_up(tmp_len, 128);
/* for first fragment packet, driver need allocate 1536 +
* drvinfo_sz + RXDESC_SIZE to defrag packet. */
- if ((mf == 1) && (frag == 0))
+ if ((mf == 1) && (frag == 0)) {
alloc_sz = 1658;/*1658+6=1664, 1664 is 128 alignment.*/
+ if (tmp_len > alloc_sz) {
+ tmp_len = alloc_sz;
+ }
+ }
else
alloc_sz = tmp_len;
/* 2 is for IP header 4 bytes alignment in QoS packet case.
--
1.9.1



2015-05-19 15:51:56

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 05/19/2015 12:47 AM, Haggai Eran wrote:
> With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
> packet are set, but the packet length is too large. Truncate the packet
> to prevent memory corruption.
>
> Signed-off-by: Haggai Eran <[email protected]>
> ---
>
> Hi,
>
> I think this solves the issue for me. I'll test it more thoroughly later. I
> still don't know why a fragmented packet has such a large pkt_len value though.
>
> Thanks,
> Haggai
>
> drivers/staging/rtl8712/rtl8712_recv.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)

I added a printout to your patch to log the values for tmp_len and alloc_sz when
tmp_len > alloc_sz. In about 15 minutes of running, that print has not
triggered. The condition only seems to happen on your system.

Please replace your patch with my modified version and report the printed values.

I have to go out today, thus there is no hurry.

Thanks,

Larry

>
> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
> index cd8b444..d7ea9c1 100644
> --- a/drivers/staging/rtl8712/rtl8712_recv.c
> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> @@ -1055,8 +1055,12 @@ static int recvbuf2recvframe(struct _adapter *padapter, struct sk_buff *pskb)
> pkt_offset = (u16)round_up(tmp_len, 128);
> /* for first fragment packet, driver need allocate 1536 +
> * drvinfo_sz + RXDESC_SIZE to defrag packet. */
> - if ((mf == 1) && (frag == 0))
> + if ((mf == 1) && (frag == 0)) {
> alloc_sz = 1658;/*1658+6=1664, 1664 is 128 alignment.*/
> + if (tmp_len > alloc_sz) {
> + tmp_len = alloc_sz;
> + }
> + }
> else
> alloc_sz = tmp_len;
> /* 2 is for IP header 4 bytes alignment in QoS packet case.
>


Attachments:
rtl8712_prevent_buffer_overrun (4.78 kB)

2015-05-19 17:23:24

by Haggai Eran

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 19 May 2015 at 18:51, Larry Finger <[email protected]> wrote:
> On 05/19/2015 12:47 AM, Haggai Eran wrote:
>>
>> With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
>> packet are set, but the packet length is too large. Truncate the packet
>> to prevent memory corruption.
>>
>> Signed-off-by: Haggai Eran <[email protected]>
>> ---
>>
>> Hi,
>>
>> I think this solves the issue for me. I'll test it more thoroughly later.
>> I
>> still don't know why a fragmented packet has such a large pkt_len value
>> though.
>>
>> Thanks,
>> Haggai
>>
>> drivers/staging/rtl8712/rtl8712_recv.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>
> I added a printout to your patch to log the values for tmp_len and alloc_sz
> when tmp_len > alloc_sz. In about 15 minutes of running, that print has not
> triggered. The condition only seems to happen on your system.
>
> Please replace your patch with my modified version and report the printed
> values.

I think you attached the original version, and not the one with the
prints. In any case, here are some example values I've seen:

[41727.150644] truncating packet: tmp_len = 3478, alloc_sz = 1658,
pkt_len = 3454, drvinfo_sz = 0
[41732.746346] truncating packet: tmp_len = 13484, alloc_sz = 1658,
pkt_len = 13460, drvinfo_sz = 0
[42044.508326] truncating packet: tmp_len = 9998, alloc_sz = 1658,
pkt_len = 9974, drvinfo_sz = 0
[42044.600013] truncating packet: tmp_len = 1982, alloc_sz = 1658,
pkt_len = 1958, drvinfo_sz = 0
[42044.677548] truncating packet: tmp_len = 11920, alloc_sz = 1658,
pkt_len = 11896, drvinfo_sz = 0

I think this issue may have started when I changed my home router /
access point from a LevelOne WDR-6001 to a TP-Link TL-WR1043ND. Could
these packets be trigerred somehow by a different interaction between
the access point and the wifi adapter?

In addition to these fragmented packets, by the way, I'm still seeing
by the way many packets that are dropped for other reasons, such as:
- invalid ver field
- seq_ctrl doesn't match in recv_decache
- sta2sta_data_frame failing for some reason
- packets with frame type 12.

Thanks,
Haggai