Re: [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

Re: [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Luis R. Rodriguez]

On Fri, Aug 17, 2012 at 9:20 PM, Sergei Poselenov
<sposelenov@emcraft.com> wrote:
>
> On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
> compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
> crashes were observed in rt2800usb module because of the invalid
> length of the received packet (3392, 46920...). This patch adds
> the sanity check on the packet legth. In case of the bad length,
> mark the packet as with CRC error.

Nice find! However this is a *real* patch that must go upstream, so
you should submit it to the rt2800usb maintainers and also mark the
patch as a stable fix. You mark the patch as a stable fix by appending
at the bottom of the commit log message: Cc: stable@vger.kernel.org.
For example:

commit e19f15ac6437624b6214b2f0ec0d69fb7eb205fa
Author: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
Date:   Thu Aug 9 12:37:26 2012 +0530

    ath9k: stop btcoex on device suspend

    During suspend, the device will be moved to FULLSLEEP state.
    As btcoex is never been stopped, the btcoex timer is running
    and tries to access hw on fullsleep state. Fix that.

    Cc: stable@vger.kernel.org
    Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

Once you submit if you want it merged into a stable compat-wireless
release even before Greg Kroah-Hartman can apply it and make a new
3.4.10 release (by the time you submit maybe 3.4.11 maybe) you can
send the patch to be applied to the driver in compat-wirelss by
submitting it to be part of the linux-next-pending/ directory. In
there are patches that are at least posted publicly. The format of the
patch there is to be in a full patch. If you have doubts check out the
compat-wireless-3.5.1-1-snpc.tar.bz2 release which has patches in all
of these directories:

  * pending-stable/
  * linux-next-pending/
  * linux-next-cherry-picks/
  * crap/

At this point you may also want to consider upgrading to
compat-wireless-3.5.1-1-snpc.tar.bz2. I should note that to send
patches for the 3.4 stable release of compat-wireless you should use
the linux-3.4.y branch. The same applies to the 3.5 release, you'd use
the linux-3.5.y branch. Except for patches for files in the above
directories I also require all patches merged into the stable branches
to be in the master branch first -- this follows the kernel's on
policy on stable releases.

The compat-wireless project is also now renamed to "compat-drivers"
and we have a new mailing list and home page for the project. New
releases based under the new name will start as of the v3.7 releases.
For more details please see:

https://backports.wiki.kernel.org

Please have a read here with regards for documentation of the
additional patches for compat-drivers.

https://backports.wiki.kernel.org/index.php/Documentation/compat-drivers/additional-patches

  Luis

Re: [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

Hello Luis,

Thanks for the feedback. One simple question below.

On Mon, 20 Aug 2012 11:35:06 +0500
"Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:

> On Fri, Aug 17, 2012 at 9:20 PM, Sergei Poselenov
> <sposelenov@emcraft.com> wrote:
> >
> > On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
> > compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
> > crashes were observed in rt2800usb module because of the invalid
> > length of the received packet (3392, 46920...). This patch adds
> > the sanity check on the packet legth. In case of the bad length,
> > mark the packet as with CRC error.
> 
> Nice find! However this is a *real* patch that must go upstream, so
> you should submit it to the rt2800usb maintainers and also mark the
> patch as a stable fix. You mark the patch as a stable fix by appending
> at the bottom of the commit log message: Cc: stable@vger.kernel.org.
> For example:
> 

Before I did something stupid with my patch, here is what I'm going to
do to make it reviewed in upsteam:

 - Reapply and retest on compat-wireless-3.5.1-1-snpc.tar.bz2
 - According to the http://linuxwireless.org/en/developers/maintainers
   send the patch to
M:	Ivo van Doorn <IvDoorn@gmail.com>
M:	Gertjan van Wingerde <gwingerde@gmail.com>
M:	Helmut Schaa <helmut.schaa@googlemail.com>
and CC to linux-wireless@vger.kernel.org

 - Add the "Cc: stable@vger.kernel.org" before my "Signed-off-by" tag,
   as suggested in the example below.

Is that correct?

Thanks is advance.

After I will submit it to the mainline, I will try to provide the patch
to the linux-next-pending.

Regards,
Sergei Poselenov, Emcraft Systems


> commit e19f15ac6437624b6214b2f0ec0d69fb7eb205fa
> Author: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
> Date:   Thu Aug 9 12:37:26 2012 +0530
> 
>     ath9k: stop btcoex on device suspend
> 
>     During suspend, the device will be moved to FULLSLEEP state.
>     As btcoex is never been stopped, the btcoex timer is running
>     and tries to access hw on fullsleep state. Fix that.
> 
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Rajkumar Manoharan <rmanohar@qca.qualcomm.com>
>     Signed-off-by: John W. Linville <linville@tuxdriver.com>
> 
> Once you submit if you want it merged into a stable compat-wireless
> release even before Greg Kroah-Hartman can apply it and make a new
> 3.4.10 release (by the time you submit maybe 3.4.11 maybe) you can
> send the patch to be applied to the driver in compat-wirelss by
> submitting it to be part of the linux-next-pending/ directory. In
> there are patches that are at least posted publicly. The format of the
> patch there is to be in a full patch. If you have doubts check out the
> compat-wireless-3.5.1-1-snpc.tar.bz2 release which has patches in all
> of these directories:
> 
>   * pending-stable/
>   * linux-next-pending/
>   * linux-next-cherry-picks/
>   * crap/
> 
> At this point you may also want to consider upgrading to
> compat-wireless-3.5.1-1-snpc.tar.bz2. I should note that to send
> patches for the 3.4 stable release of compat-wireless you should use
> the linux-3.4.y branch. The same applies to the 3.5 release, you'd use
> the linux-3.5.y branch. Except for patches for files in the above
> directories I also require all patches merged into the stable branches
> to be in the master branch first -- this follows the kernel's on
> policy on stable releases.
> 
> The compat-wireless project is also now renamed to "compat-drivers"
> and we have a new mailing list and home page for the project. New
> releases based under the new name will start as of the v3.7 releases.
> For more details please see:
> 
> https://backports.wiki.kernel.org
> 
> Please have a read here with regards for documentation of the
> additional patches for compat-drivers.
> 
> https://backports.wiki.kernel.org/index.php/Documentation/compat-drivers/additional-patches
> 
>   Luis

Re: [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Stanislaw Gruszka]

On Mon, Aug 20, 2012 at 03:40:16PM +0400, Sergei Poselenov wrote:
>  - Reapply and retest on compat-wireless-3.5.1-1-snpc.tar.bz2
>  - According to the http://linuxwireless.org/en/developers/maintainers
>    send the patch to
> M:	Ivo van Doorn <IvDoorn@gmail.com>
> M:	Gertjan van Wingerde <gwingerde@gmail.com>
> M:	Helmut Schaa <helmut.schaa@googlemail.com>

Preferably post it to users@rt2x00.serialmonkey.com instead to Ivo,
Gertjan and Helmut directly.

Thanks
Stanislaw

[PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
crashes were observed in rt2800usb module because of the invalid
length of the received packet (3392, 46920...). This patch adds
the sanity check on the packet legth. In case of the bad length,
mark the packet as with CRC error.

The fix was also tested on the latest
compat-wireless-3.5.1-1-snpc.tar.bz2, applies cleanly.

Cc: stable@vger.kernel.org
Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
index 001735f..6776ec8 100644
--- a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -662,13 +662,18 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 	rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
 
 	/*
-	 * Remove the RXINFO structure from the sbk.
+	 * Remove the RXINFO structure from the skb.
 	 */
 	skb_pull(entry->skb, RXINFO_DESC_SIZE);
 
 	/*
-	 * FIXME: we need to check for rx_pkt_len validity
+	 * Check for rx_pkt_len validity, mark as failed.
 	 */
+	if (rx_pkt_len > entry->skb->len) {
+		rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
+		goto procrxwi;
+	}
+
 	rxd = (__le32 *)(entry->skb->data + rx_pkt_len);
 
 	/*
@@ -713,6 +718,7 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 	 */
 	skb_trim(entry->skb, rx_pkt_len);
 
+procrxwi:
 	/*
 	 * Process the RXWI structure.
 	 */

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Stanislaw Gruszka]

On Mon, Aug 20, 2012 at 08:53:55PM +0400, Sergei Poselenov wrote:
> On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
> compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
Please remove compat-wireless reference here and in the subject.

> crashes were observed in rt2800usb module because of the invalid
> length of the received packet (3392, 46920...). This patch adds
> the sanity check on the packet legth. In case of the bad length,
> mark the packet as with CRC error.
> 
> The fix was also tested on the latest
> compat-wireless-3.5.1-1-snpc.tar.bz2, applies cleanly.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> index 001735f..6776ec8 100644
> --- a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -662,13 +662,18 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  	rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
>  
>  	/*
> -	 * Remove the RXINFO structure from the sbk.
> +	 * Remove the RXINFO structure from the skb.
>  	 */
>  	skb_pull(entry->skb, RXINFO_DESC_SIZE);
Would be great if you could post this as separate patch.

>  	/*
> -	 * FIXME: we need to check for rx_pkt_len validity
> +	 * Check for rx_pkt_len validity, mark as failed.
>  	 */
> +	if (rx_pkt_len > entry->skb->len) {
> +		rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
> +		goto procrxwi;

I would rather prefer something like 

if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
 	/* Process error in rt2x00lib_rxdone() */
	rxdesc->size = rx_pkt_len;
	return;
}

Thanks
Stanislaw

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Ivo Van Doorn]

On Tue, Aug 21, 2012 at 1:43 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Mon, Aug 20, 2012 at 08:53:55PM +0400, Sergei Poselenov wrote:
>> On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
>> compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
> Please remove compat-wireless reference here and in the subject.
>
>> crashes were observed in rt2800usb module because of the invalid
>> length of the received packet (3392, 46920...). This patch adds
>> the sanity check on the packet legth. In case of the bad length,
>> mark the packet as with CRC error.
>>
>> The fix was also tested on the latest
>> compat-wireless-3.5.1-1-snpc.tar.bz2, applies cleanly.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 001735f..6776ec8 100644
>> --- a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -662,13 +662,18 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>>       rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
>>
>>       /*
>> -      * Remove the RXINFO structure from the sbk.
>> +      * Remove the RXINFO structure from the skb.
>>        */
>>       skb_pull(entry->skb, RXINFO_DESC_SIZE);
> Would be great if you could post this as separate patch.
>
>>       /*
>> -      * FIXME: we need to check for rx_pkt_len validity
>> +      * Check for rx_pkt_len validity, mark as failed.
>>        */
>> +     if (rx_pkt_len > entry->skb->len) {
>> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
>> +             goto procrxwi;
>
> I would rather prefer something like
>
> if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
>         /* Process error in rt2x00lib_rxdone() */
>         rxdesc->size = rx_pkt_len;
>         return;
> }

But how do you know the packet is correct then? Obviously something is wrong,
so just resetting the rxdesc->size wouldn't be a solution right?

Ivo

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Stanislaw Gruszka]

On Tue, Aug 21, 2012 at 03:39:41PM +0200, Ivo Van Doorn wrote:
> On Tue, Aug 21, 2012 at 1:43 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > On Mon, Aug 20, 2012 at 08:53:55PM +0400, Sergei Poselenov wrote:
> >> On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
> >> compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
> > Please remove compat-wireless reference here and in the subject.
> >
> >> crashes were observed in rt2800usb module because of the invalid
> >> length of the received packet (3392, 46920...). This patch adds
> >> the sanity check on the packet legth. In case of the bad length,
> >> mark the packet as with CRC error.
> >>
> >> The fix was also tested on the latest
> >> compat-wireless-3.5.1-1-snpc.tar.bz2, applies cleanly.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
> >> ---
> >>  drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
> >>  1 files changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> >> index 001735f..6776ec8 100644
> >> --- a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> >> +++ b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
> >> @@ -662,13 +662,18 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
> >>       rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
> >>
> >>       /*
> >> -      * Remove the RXINFO structure from the sbk.
> >> +      * Remove the RXINFO structure from the skb.
> >>        */
> >>       skb_pull(entry->skb, RXINFO_DESC_SIZE);
> > Would be great if you could post this as separate patch.
> >
> >>       /*
> >> -      * FIXME: we need to check for rx_pkt_len validity
> >> +      * Check for rx_pkt_len validity, mark as failed.
> >>        */
> >> +     if (rx_pkt_len > entry->skb->len) {
> >> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
> >> +             goto procrxwi;
> >
> > I would rather prefer something like
> >
> > if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
> >         /* Process error in rt2x00lib_rxdone() */
> >         rxdesc->size = rx_pkt_len;
> >         return;
> > }
> 
> But how do you know the packet is correct then?
Non zero rx_pkt_len smaller than data_size indicate correct package.

> Obviously something is wrong,
> so just resetting the rxdesc->size wouldn't be a solution right?

rt2x00lib_rxdone has rxdesc->size check too, if ->size is bad it 
prints warning, and requeue skb. 

Perhaps this could be coded in some cleaner way (avoid double check),
but basically this should do the job. 

Stanislaw

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Gertjan van Wingerde]

On 21 aug. 2012, at 16:18, Stanislaw Gruszka <sgruszka@redhat.com> wrote:

> On Tue, Aug 21, 2012 at 03:39:41PM +0200, Ivo Van Doorn wrote:
>> On Tue, Aug 21, 2012 at 1:43 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>>> On Mon, Aug 20, 2012 at 08:53:55PM +0400, Sergei Poselenov wrote:
>>>> On our system (ARM Cortex-M3 SOC running linux-2.6.33 with
>>>> compat-wireless-3.4-rc3-1 modules configured for rt2x00) frequent
>>> Please remove compat-wireless reference here and in the subject.
>>> 
>>>> crashes were observed in rt2800usb module because of the invalid
>>>> length of the received packet (3392, 46920...). This patch adds
>>>> the sanity check on the packet legth. In case of the bad length,
>>>> mark the packet as with CRC error.
>>>> 
>>>> The fix was also tested on the latest
>>>> compat-wireless-3.5.1-1-snpc.tar.bz2, applies cleanly.
>>>> 
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
>>>> ---
>>>> drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
>>>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>>>> index 001735f..6776ec8 100644
>>>> --- a/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>>>> +++ b/usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c
>>>> @@ -662,13 +662,18 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>>>>      rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
>>>> 
>>>>      /*
>>>> -      * Remove the RXINFO structure from the sbk.
>>>> +      * Remove the RXINFO structure from the skb.
>>>>       */
>>>>      skb_pull(entry->skb, RXINFO_DESC_SIZE);
>>> Would be great if you could post this as separate patch.
>>> 
>>>>      /*
>>>> -      * FIXME: we need to check for rx_pkt_len validity
>>>> +      * Check for rx_pkt_len validity, mark as failed.
>>>>       */
>>>> +     if (rx_pkt_len > entry->skb->len) {
>>>> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
>>>> +             goto procrxwi;
>>> 
>>> I would rather prefer something like
>>> 
>>> if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
>>>        /* Process error in rt2x00lib_rxdone() */
>>>        rxdesc->size = rx_pkt_len;
>>>        return;
>>> }
>> 
>> But how do you know the packet is correct then?
> Non zero rx_pkt_len smaller than data_size indicate correct package.

To be honest, I think the original approach of Sergei is better. Not touching rxdesc beyond setting the flag will ensure that rt2x00lib_rxdone will simply bounce the skb without handing an invalid packet over to mac80211. That said, it isn't necessary to set the flag. Just returning from the function is good enough.

However, the check that Sergei does is not correct either. The real check that should be done is checking whether the skb has enough data to hold both rx_pkt_len bytes + the size of the rxd, which is 1 word (4 bytes). If only rx_pkt_len are left we don't have an rxd, and is the usb packet invalid as well.

> 
>> Obviously something is wrong,
>> so just resetting the rxdesc->size wouldn't be a solution right?
> 
> rt2x00lib_rxdone has rxdesc->size check too, if ->size is bad it 
> prints warning, and requeue skb. 
> 
> Perhaps this could be coded in some cleaner way (avoid double check),
> but basically this should do the job. 

As I mentioned above, simply bailing out if rt2800usb_fill_rxdone without doing anything (not even setting a flag) should do the trick and IMHO is the cleanest approach.

---
Gertjan

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Stanislaw Gruszka]

On Tue, Aug 21, 2012 at 10:07:03PM +0200, Gertjan van Wingerde wrote:
> >>>> +     if (rx_pkt_len > entry->skb->len) {
> >>>> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
> >>>> +             goto procrxwi;
> >>> 
> >>> I would rather prefer something like
> >>> 
> >>> if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
> >>>        /* Process error in rt2x00lib_rxdone() */
> >>>        rxdesc->size = rx_pkt_len;
> >>>        return;
> >>> }
> >> 
> >> But how do you know the packet is correct then?
> > Non zero rx_pkt_len smaller than data_size indicate correct package.
> 
> To be honest, I think the original approach of Sergei is better. Not touching rxdesc beyond setting the flag will ensure that rt2x00lib_rxdone will simply bounce the skb without handing an invalid packet over to mac80211. That said, it isn't necessary to set the flag. Just returning from the function is good enough.
> 
> However, the check that Sergei does is not correct either. The real check that should be done is checking whether the skb has enough data to hold both rx_pkt_len bytes + the size of the rxd, which is 1 word (4 bytes). If only rx_pkt_len are left we don't have an rxd, and is the usb packet invalid as well.

Yes, but there is also usb alignment on skb->len, so is better to use
queue->data_size to validate rx_pkt (data) length IMHO.

> >> Obviously something is wrong,
> >> so just resetting the rxdesc->size wouldn't be a solution right?
> > 
> > rt2x00lib_rxdone has rxdesc->size check too, if ->size is bad it 
> > prints warning, and requeue skb. 
> > 
> > Perhaps this could be coded in some cleaner way (avoid double check),
> > but basically this should do the job. 
> 
> As I mentioned above, simply bailing out if rt2800usb_fill_rxdone without doing anything (not even setting a flag) should do the trick and IMHO is the cleanest approach.

IIRC this is basically what I proposed, except without setting
rxdesc->size, unlikely() and rx_pkt_len == 0 check. It will work as
rxdesc->size will be 0. But I think it would be better, if WARNING on
rt2x00lib_rxdone() will print actual corrupted size instead of 0.
Having unlikely is good too - this must be unlikely situation.

BTW: would be good to fix reason of that corruption if possible
(as long this is not a H/W or F/W bug). But for now, let just
stop kernel crashing. Printing WARNING on this situation will
help to identify there is something wrong if someone will observe
performance problems or similar.

Thanks
Stanislaw

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Gertjan van Wingerde]

Hi Stanislaw,

On 22 aug. 2012, at 11:27, Stanislaw Gruszka <sgruszka@redhat.com> wrote:

> On Tue, Aug 21, 2012 at 10:07:03PM +0200, Gertjan van Wingerde wrote:
>>>>>> +     if (rx_pkt_len > entry->skb->len) {
>>>>>> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
>>>>>> +             goto procrxwi;
>>>>> 
>>>>> I would rather prefer something like
>>>>> 
>>>>> if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
>>>>>       /* Process error in rt2x00lib_rxdone() */
>>>>>       rxdesc->size = rx_pkt_len;
>>>>>       return;
>>>>> }
>>>> 
>>>> But how do you know the packet is correct then?
>>> Non zero rx_pkt_len smaller than data_size indicate correct package.
>> 
>> To be honest, I think the original approach of Sergei is better. Not touching rxdesc beyond setting the flag will ensure that rt2x00lib_rxdone will simply bounce the skb without handing an invalid packet over to mac80211. That said, it isn't necessary to set the flag. Just returning from the function is good enough.
>> 
>> However, the check that Sergei does is not correct either. The real check that should be done is checking whether the skb has enough data to hold both rx_pkt_len bytes + the size of the rxd, which is 1 word (4 bytes). If only rx_pkt_len are left we don't have an rxd, and is the usb packet invalid as well.
> 
> Yes, but there is also usb alignment on skb->len, so is better to use
> queue->data_size to validate rx_pkt (data) length IMHO.

Oh, well it's an (almost) equivalent test anyway, so I guess I don't care.

> 
>>>> Obviously something is wrong,
>>>> so just resetting the rxdesc->size wouldn't be a solution right?
>>> 
>>> rt2x00lib_rxdone has rxdesc->size check too, if ->size is bad it 
>>> prints warning, and requeue skb. 
>>> 
>>> Perhaps this could be coded in some cleaner way (avoid double check),
>>> but basically this should do the job. 
>> 
>> As I mentioned above, simply bailing out if rt2800usb_fill_rxdone without doing anything (not even setting a flag) should do the trick and IMHO is the cleanest approach.
> 
> IIRC this is basically what I proposed, except without setting
> rxdesc->size, unlikely() and rx_pkt_len == 0 check. It will work as
> rxdesc->size will be 0. But I think it would be better, if WARNING on
> rt2x00lib_rxdone() will print actual corrupted size instead of 0.
> Having unlikely is good too - this must be unlikely situation.

OK, I agree with the use of unlikely(). An rx_pkt_len of 0 doesn't seem to happen in practice, so testing for it seems superfluous, but may be providing some extra safety. Don't really care about that. I really don't think we should set rxdesc->size, as we cannot determine it. If we want to print the value, then do it in this function. Adding a hack to having it printed somewhere else doesn't seem right to me.
Also, it should be an error log message, not a WARNING. There's no need to have a stack trace as the buffer is filled by HW, not by some other function. So, the use of WARNING, with its stack dumping functionality is overkill.


> 
> BTW: would be good to fix reason of that corruption if possible
> (as long this is not a H/W or F/W bug). But for now, let just
> stop kernel crashing. Printing WARNING on this situation will
> help to identify there is something wrong if someone will observe
> performance problems or similar.

I think this is a HW issue. As mentioned above, I believe a WARNING here is overkill, as we don't need the stack trace.

Anyway, Sergei, would you be able to modify the patch along the lines of the discussion?

---
Gertjan

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Stanislaw Gruszka]

Hi Gertjan
 
On Wed, Aug 22, 2012 at 10:41:42PM +0200, Gertjan van Wingerde wrote:
> > IIRC this is basically what I proposed, except without setting
> > rxdesc->size, unlikely() and rx_pkt_len == 0 check. It will work as
> > rxdesc->size will be 0. But I think it would be better, if WARNING on
> > rt2x00lib_rxdone() will print actual corrupted size instead of 0.
> > Having unlikely is good too - this must be unlikely situation.
> 
> OK, I agree with the use of unlikely(). An rx_pkt_len of 0 doesn't seem to happen in practice, so testing for it seems superfluous, but may be providing some extra safety. Don't really care about that. I really don't think we should set rxdesc->size, as we cannot determine it. If we want to print the value, then do it in this function. Adding a hack to having it printed somewhere else doesn't seem right to me.
> Also, it should be an error log message, not a WARNING. There's no need to have a stack trace as the buffer is filled by HW, not by some other function. So, the use of WARNING, with its stack dumping functionality is overkill.
> 
> 
> > 
> > BTW: would be good to fix reason of that corruption if possible
> > (as long this is not a H/W or F/W bug). But for now, let just
> > stop kernel crashing. Printing WARNING on this situation will
> > help to identify there is something wrong if someone will observe
> > performance problems or similar.
> 
> I think this is a HW issue. As mentioned above, I believe a WARNING here is overkill, as we don't need the stack trace.

I was talking about this WARNING in rt2x00lib_rxdone() (which is not
WARN_ON() or WARN() - so no stactrace):

	/*
	 * Check for valid size in case we get corrupted descriptor from
	 * hardware.
	 */
	if (unlikely(rxdesc.size == 0 ||
		     rxdesc.size > entry->queue->data_size)) {
		WARNING(rt2x00dev, "Wrong frame size %d max %d.\n",
			rxdesc.size, entry->queue->data_size);
		dev_kfree_skb(entry->skb);
		goto renew_skb;
	}


I agree that using ERROR is better.
 
> Anyway, Sergei, would you be able to modify the patch along the lines of the discussion?

I hope so :-)

Thanks
Stanislaw

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

Hello guys,

On Wed, 2012-08-22 at 22:41 +0200, Gertjan van Wingerde wrote:
> 
> Anyway, Sergei, would you be able to modify the patch along the lines
> of the discussion? 

Yes, I've been watching your discussion. I hope I'll be able to come up
with a new version shortly.

Regards,
Sergei

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

All, 

On Wed, 2012-08-22 at 22:41 +0200, Gertjan van Wingerde wrote:
> Hi Stanislaw,
> 
> On 22 aug. 2012, at 11:27, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> 
> > On Tue, Aug 21, 2012 at 10:07:03PM +0200, Gertjan van Wingerde wrote:
> >>>>>> +     if (rx_pkt_len > entry->skb->len) {
> >>>>>> +             rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
> >>>>>> +             goto procrxwi;
> >>>>> 
> >>>>> I would rather prefer something like
> >>>>> 
> >>>>> if (unlikely(rx_pkt_len == 0 || rx_pkt_len > entry->queue->data_size)) {
> >>>>>       /* Process error in rt2x00lib_rxdone() */
> >>>>>       rxdesc->size = rx_pkt_len;
> >>>>>       return;
> >>>>> }
> >>>> 
> >>>> But how do you know the packet is correct then?
> >>> Non zero rx_pkt_len smaller than data_size indicate correct package.
> >> 
> >> To be honest, I think the original approach of Sergei is better. Not touching rxdesc beyond setting the flag will ensure that rt2x00lib_rxdone will simply bounce the skb without handing an invalid packet over to mac80211. That said, it isn't necessary to set the flag. Just returning from the function is good enough.
> >> 
> >> However, the check that Sergei does is not correct either. The real check that should be done is checking whether the skb has enough data to hold both rx_pkt_len bytes + the size of the rxd, which is 1 word (4 bytes). If only rx_pkt_len are left we don't have an rxd, and is the usb packet invalid as well.
> > 
> > Yes, but there is also usb alignment on skb->len, so is better to use
> > queue->data_size to validate rx_pkt (data) length IMHO.
> 
> Oh, well it's an (almost) equivalent test anyway, so I guess I don't care.
> 
> > 
> >>>> Obviously something is wrong,
> >>>> so just resetting the rxdesc->size wouldn't be a solution right?
> >>> 
> >>> rt2x00lib_rxdone has rxdesc->size check too, if ->size is bad it 
> >>> prints warning, and requeue skb. 
> >>> 
> >>> Perhaps this could be coded in some cleaner way (avoid double check),
> >>> but basically this should do the job. 
> >> 
> >> As I mentioned above, simply bailing out if rt2800usb_fill_rxdone without doing anything (not even setting a flag) should do the trick and IMHO is the cleanest approach.
> > 
> > IIRC this is basically what I proposed, except without setting
> > rxdesc->size, unlikely() and rx_pkt_len == 0 check. It will work as
> > rxdesc->size will be 0. But I think it would be better, if WARNING on
> > rt2x00lib_rxdone() will print actual corrupted size instead of 0.
> > Having unlikely is good too - this must be unlikely situation.
> 
> OK, I agree with the use of unlikely(). An rx_pkt_len of 0 doesn't seem to happen in practice, so testing for it seems superfluous, but may be providing some extra safety. Don't really care about that. I really don't think we should set rxdesc->size, as we cannot determine it. If we want to print the value, then do it in this function. Adding a hack to having it printed somewhere else doesn't seem right to me.

OK, I modified the check and changed WARNING() to ERROR() in
rt2x00lib_rxdone() as Stanislaw suggested.

Now the error messages looks like follows:
...
phy0 -> rt2800usb_fill_rxdone: Error - Bad frame length 12354, forcing to 0
phy0 -> rt2x00lib_rxdone: Error - Wrong frame size 0 max 3840.
...

OK to submit the final patch?

Regards,
Sergei

> Also, it should be an error log message, not a WARNING. There's no need to have a stack trace as the buffer is filled by HW, not by some other function. So, the use of WARNING, with its stack dumping functionality is overkill.
> 
> 
> > 
> > BTW: would be good to fix reason of that corruption if possible
> > (as long this is not a H/W or F/W bug). But for now, let just
> > stop kernel crashing. Printing WARNING on this situation will
> > help to identify there is something wrong if someone will observe
> > performance problems or similar.
> 
> I think this is a HW issue. As mentioned above, I believe a WARNING here is overkill, as we don't need the stack trace.
> 
> Anyway, Sergei, would you be able to modify the patch along the lines of the discussion?
> 
> ---
> Gertjan

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

Stanislaw,

On Tue, 2012-08-21 at 13:43 +0200, Stanislaw Gruszka wrote:
> >       /*
> > -      * Remove the RXINFO structure from the sbk.
> > +      * Remove the RXINFO structure from the skb.
> >        */
> >       skb_pull(entry->skb, RXINFO_DESC_SIZE);
> Would be great if you could post this as separate patch.

I'm sending the patch for this typo in the follow-up email.

Regards,
Sergei

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Fixed a typo

[Sergei Poselenov]

Fixed a typo in the comment.

Cc: stable@vger.kernel.org
Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 6cf3365..f8085b2 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -662,7 +662,7 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 	rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
 
 	/*
-	 * Remove the RXINFO structure from the sbk.
+	 * Remove the RXINFO structure from the skb.
 	 */
 	skb_pull(entry->skb, RXINFO_DESC_SIZE);
 
-- 
1.7.4.4

Re: [rt2x00-users] [PATCH] compat-wireless:rt2800usb: Fixed a typo

[Ivo Van Doorn]

On Sun, Aug 26, 2012 at 3:56 PM, Sergei Poselenov
<sposelenov@emcraft.com> wrote:
> Fixed a typo in the comment.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 6cf3365..f8085b2 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -662,7 +662,7 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>         rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
>
>         /*
> -        * Remove the RXINFO structure from the sbk.
> +        * Remove the RXINFO structure from the skb.
>          */
>         skb_pull(entry->skb, RXINFO_DESC_SIZE);
>
> --
> 1.7.4.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rt2x00-users] [PATCH V2]: rt2800usb: Added rx packet length validity check

[Sergei Poselenov]

On our system (ARM Cortex-M3 SOC running linux-2.6.33)
frequent crashes were observed in the rt2800usb module
because of the invalid length of the received packet (3392,
46920...). This patch adds the sanity check on the packet
legth. Also, changed WARNING to ERROR in rt2x00lib_rxdone()
so that the bad packet condition would be noticed.

The fix was tested on the latest compat-wireless-3.5.1-1-snpc.

Cc: stable@vger.kernel.org
Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 +++++++++-
 drivers/net/wireless/rt2x00/rt2x00dev.c |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index f8085b2..48df102 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -667,8 +667,16 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 	skb_pull(entry->skb, RXINFO_DESC_SIZE);
 
 	/*
-	 * FIXME: we need to check for rx_pkt_len validity
+	 * Check for rx_pkt_len validity. Return if invalid, leaving
+	 * rxdesc->size zeroed out by the upper level.
 	 */
+	if (unlikely(rx_pkt_len == 0 ||
+			rx_pkt_len > entry->queue->data_size)) {
+		ERROR(entry->queue->rt2x00dev,
+			"Bad frame size %d, forcing to 0\n", rx_pkt_len);
+		return;
+	}
+
 	rxd = (__le32 *)(entry->skb->data + rx_pkt_len);
 
 	/*
diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index a59048f..10cf672 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -629,7 +629,7 @@ void rt2x00lib_rxdone(struct queue_entry *entry, gfp_t gfp)
 	 */
 	if (unlikely(rxdesc.size == 0 ||
 		     rxdesc.size > entry->queue->data_size)) {
-		WARNING(rt2x00dev, "Wrong frame size %d max %d.\n",
+		ERROR(rt2x00dev, "Wrong frame size %d max %d.\n",
 			rxdesc.size, entry->queue->data_size);
 		dev_kfree_skb(entry->skb);
 		goto renew_skb;
-- 
1.7.4.4

Re: [rt2x00-users] [PATCH V2]: rt2800usb: Added rx packet length validity check

[Ivo Van Doorn]

On Sun, Sep 2, 2012 at 11:14 AM, Sergei Poselenov
<sposelenov@emcraft.com> wrote:
> On our system (ARM Cortex-M3 SOC running linux-2.6.33)
> frequent crashes were observed in the rt2800usb module
> because of the invalid length of the received packet (3392,
> 46920...). This patch adds the sanity check on the packet
> legth. Also, changed WARNING to ERROR in rt2x00lib_rxdone()
> so that the bad packet condition would be noticed.
>
> The fix was tested on the latest compat-wireless-3.5.1-1-snpc.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergei Poselenov <sposelenov@emcraft.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |   10 +++++++++-
>  drivers/net/wireless/rt2x00/rt2x00dev.c |    2 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index f8085b2..48df102 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -667,8 +667,16 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>         skb_pull(entry->skb, RXINFO_DESC_SIZE);
>
>         /*
> -        * FIXME: we need to check for rx_pkt_len validity
> +        * Check for rx_pkt_len validity. Return if invalid, leaving
> +        * rxdesc->size zeroed out by the upper level.
>          */
> +       if (unlikely(rx_pkt_len == 0 ||
> +                       rx_pkt_len > entry->queue->data_size)) {
> +               ERROR(entry->queue->rt2x00dev,
> +                       "Bad frame size %d, forcing to 0\n", rx_pkt_len);
> +               return;
> +       }
> +
>         rxd = (__le32 *)(entry->skb->data + rx_pkt_len);
>
>         /*
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index a59048f..10cf672 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -629,7 +629,7 @@ void rt2x00lib_rxdone(struct queue_entry *entry, gfp_t gfp)
>          */
>         if (unlikely(rxdesc.size == 0 ||
>                      rxdesc.size > entry->queue->data_size)) {
> -               WARNING(rt2x00dev, "Wrong frame size %d max %d.\n",
> +               ERROR(rt2x00dev, "Wrong frame size %d max %d.\n",
>                         rxdesc.size, entry->queue->data_size);
>                 dev_kfree_skb(entry->skb);
>                 goto renew_skb;
> --
> 1.7.4.4
>
>
>