compat-wireless:rt2800usb: Added rx packet length validity check

PATCH In review Sergei Poselenov 4 participants 15 messages Aug 20, 2012 – Sep 2, 2012 Networking drivers (wireless)

Share & tools

Sergei Poselenov

author

2012-08-20 16:54 UTC #0 lore raw

PATCH 1/1 compat-wireless:rt2800usb: Added rx packet length validity check

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 [email protected]

Signed-off-by Sergei Poselenov <[email protected]>

---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +8 -2

index 001735f..6776ec8 100644
@@ -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.
 	 */

Stanislaw Gruszka

2012-08-21 11:44 UTC #1 lore raw

Quoted 2 lines · 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

Please remove compat-wireless reference here and in the subject.

Quoted 19 lines

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 [email protected]
Signed-off-by Sergei Poselenov <[email protected]>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +0 -0
index 001735f..6776ec8 100644

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 667

Quoted 7 lines

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +1 -1

@@ -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.

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674

Quoted 7 lines

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +4 -1

 	/*
-	 * 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

Ivo Van Doorn

2012-08-21 13:39 UTC #2 lore raw

Quoted 41 lines · Stanislaw Gruszka

Quoted 2 lines · 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
Please remove compat-wireless reference here and in the subject.
Quoted 19 lines
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 [email protected]
Signed-off-by Sergei Poselenov <[email protected]>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +0 -0
index 001735f..6776ec8 100644
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 667
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +1 -1
@@ -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.
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +4 -1
      /*
-      * 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;

Quoted 7 lines

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

Stanislaw Gruszka

2012-08-21 14:19 UTC #3 lore raw

Quoted 44 lines · Ivo Van Doorn

Quoted 43 lines · Stanislaw Gruszka
Quoted 2 lines · 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
Please remove compat-wireless reference here and in the subject.
Quoted 19 lines
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 [email protected]
Signed-off-by Sergei Poselenov <[email protected]>
---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +0 -0
index 001735f..6776ec8 100644
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 667
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +1 -1
@@ -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.
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +4 -1
      /*
-      * 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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 2 lines

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

Gertjan van Wingerde

2012-08-21 20:07 UTC #4 lore raw

Quoted 45 lines · Stanislaw Gruszka

Quoted 44 lines · Ivo Van Doorn
Quoted 43 lines · Stanislaw Gruszka
Quoted 2 lines · 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
Please remove compat-wireless reference here and in the subject.
Quoted 19 lines
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 [email protected]
Signed-off-by Sergei Poselenov <[email protected]>
---
drivers/net/wireless/rt2x00/rt2800usb.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +0 -0
index 001735f..6776ec8 100644
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 667
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +1 -1
@@ -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.
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674
Quoted 7 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +4 -1
     /*
-      * 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

Quoted 8 lines

Quoted 7 lines
Quoted 5 lines
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.

Quoted 9 lines

Quoted 3 lines
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

Stanislaw Gruszka

2012-08-22 09:27 UTC #5 lore raw

Quoted 12 lines · Gertjan van Wingerde

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674
Quoted 3 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +3 -0
+     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;
}

Quoted 6 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 10 lines

Quoted 8 lines
Quoted 2 lines
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

Gertjan van Wingerde

2012-08-22 20:41 UTC #6 lore raw

Hi Stanislaw,

Quoted 14 lines · Stanislaw Gruszka

Quoted 13 lines · Gertjan van Wingerde
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 674
Quoted 3 lines
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +3 -0
+     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?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 1 line
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.

Quoted 6 lines

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

Stanislaw Gruszka

2012-08-22 21:16 UTC #7 lore raw

Hi Gertjan

Quoted 11 lines · Gertjan van Wingerde

Quoted 5 lines
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.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
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.

Quoted 1 line

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

I hope so :-)

Thanks
Stanislaw

Sergei Poselenov

author

2012-08-23 05:46 UTC #8 lore raw

In reply to Gertjan van Wingerde

Hello guys,

Quoted 3 lines · Gertjan van Wingerde

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

Sergei Poselenov

author

2012-08-26 13:19 UTC #9 lore raw

In reply to Gertjan van Wingerde

All,

Quoted 41 lines · Gertjan van Wingerde

Hi Stanislaw,
Quoted 14 lines · Stanislaw Gruszka
Quoted 13 lines · Gertjan van Wingerde
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+     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?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
Quoted 12 lines
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 5 lines
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

Quoted 16 lines

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.
Quoted 6 lines
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

Sergei Poselenov

author

2012-09-02 09:14 UTC #10 lore raw

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

In reply to Gertjan van Wingerde

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 [email protected]

Signed-off-by Sergei Poselenov <[email protected]>

---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 +++++++++-
 drivers/net/wireless/rt2x00/rt2x00dev.c |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

drivers/net/wireless/rt2x00/rt2800usb.c +9 -1

index f8085b2..48df102 100644
@@ -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);
 
 	/*

drivers/net/wireless/rt2x00/rt2x00dev.c +1 -1

index a59048f..10cf672 100644
@@ -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;

Signature · 1 line

1.7.4.4

Ivo Van Doorn

Acked-by

2012-09-02 20:35 UTC #11 lore raw

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

Quoted 11 lines · 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 [email protected]
Signed-off-by Sergei Poselenov <[email protected]>

Acked-by Ivo van Doorn <[email protected]>

Quoted 45 lines

---
 drivers/net/wireless/rt2x00/rt2800usb.c |   10 +++++++++-
 drivers/net/wireless/rt2x00/rt2x00dev.c |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

drivers/net/wireless/rt2x00/rt2800usb.c +9 -1

index f8085b2..48df102 100644
@@ -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);
 
        /*

drivers/net/wireless/rt2x00/rt2x00dev.c +1 -2

index a59048f..10cf672 100644
@@ -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

Sergei Poselenov

author

2012-08-26 13:53 UTC #12 lore raw

In reply to Stanislaw Gruszka

Stanislaw,

Quoted 6 lines · Stanislaw Gruszka

usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c static void rt2800usb_fill_rxdone(struct queue_entry *entry, · line 667
Quoted 5 lines
      /*
usbwifi/compat-wireless-3.4-rc3-1/drivers/net/wireless/rt2x00/rt2800usb.c +1 -1
-      * 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

Sergei Poselenov

author

2012-08-26 13:56 UTC #13 lore raw

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

In reply to Stanislaw Gruszka

Fixed a typo in the comment.

Cc [email protected]

Signed-off-by Sergei Poselenov <[email protected]>

---
 drivers/net/wireless/rt2x00/rt2800usb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/wireless/rt2x00/rt2800usb.c +1 -1

index 6cf3365..f8085b2 100644
@@ -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);

Signature · 1 line

1.7.4.4

Ivo Van Doorn

Acked-by

2012-08-27 08:23 UTC #14 lore raw

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

Quoted 4 lines · Sergei Poselenov

Fixed a typo in the comment.
Cc [email protected]
Signed-off-by Sergei Poselenov <[email protected]>

Acked-by Ivo van Doorn <[email protected]>

Quoted 26 lines

---
 drivers/net/wireless/rt2x00/rt2800usb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/wireless/rt2x00/rt2800usb.c +1 -2

index 6cf3365..f8085b2 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html