[PATCH] b43: Poison RX buffers

2009-03-27 Thread Michael Buesch 
This patch adds poisoning and sanity checking to the RX DMA buffers.
This is used for protection against buggy hardware/firmware that raises
RX interrupts without doing an actual DMA transfer.

This mechanism protects against rare bad packets (due to uninitialized skb 
data)
and rare kernel crashes due to uninitialized RX headers.

The poison is selected to not match on valid frames and to be cheap for 
checking.

The poison check mechanism _might_ trigger incorrectly, if we are voluntarily
receiving frames with bad PLCP headers. However, this is nonfatal, because the
chance of such a match is basically zero and in case it happens it just results
in dropping the packet.
Bad-PLCP RX defaults to off, and you should leave it off unless you want to 
listen
to the latest news broadcasted by your microwave oven.

This patch also moves the initialization of the RX-header length field in 
front of
the mapping of the DMA buffer. The CPU should not touch the buffer after we 
mapped it.

Cc: sta...@kernel.org
Reported-by: Francesco Gringoli francesco.gring...@ing.unibs.it
Signed-off-by: Michael Buesch m...@bu3sch.de

---

Francesco, please stresstest this.
John, please queue as bugfix.


Index: wireless-testing/drivers/net/wireless/b43/dma.c
===
--- wireless-testing.orig/drivers/net/wireless/b43/dma.c2009-03-27 
12:29:57.0 +0100
+++ wireless-testing/drivers/net/wireless/b43/dma.c 2009-03-27 
22:22:21.0 +0100
@@ -555,11 +555,32 @@ address_error:
return 1;
 }
 
+static bool b43_rx_buffer_is_poisoned(struct b43_dmaring *ring, struct sk_buff 
*skb)
+{
+   unsigned char *f = skb-data + ring-frameoffset;
+
+   return ((f[0]  f[1]  f[2]  f[3]  f[4]  f[5]  f[6]  f[7]) == 
0xFF);
+}
+
+static void b43_poison_rx_buffer(struct b43_dmaring *ring, struct sk_buff *skb)
+{
+   struct b43_rxhdr_fw4 *rxhdr;
+   unsigned char *frame;
+
+   /* This poisons the RX buffer to detect DMA failures. */
+
+   rxhdr = (struct b43_rxhdr_fw4 *)(skb-data);
+   rxhdr-frame_len = 0;
+
+   B43_WARN_ON(ring-rx_buffersize  ring-frameoffset + sizeof(struct 
b43_plcp_hdr6) + 2);
+   frame = skb-data + ring-frameoffset;
+   memset(frame, 0xFF, sizeof(struct b43_plcp_hdr6) + 2 /* padding */);
+}
+
 static int setup_rx_descbuffer(struct b43_dmaring *ring,
   struct b43_dmadesc_generic *desc,
   struct b43_dmadesc_meta *meta, gfp_t gfp_flags)
 {
-   struct b43_rxhdr_fw4 *rxhdr;
dma_addr_t dmaaddr;
struct sk_buff *skb;
 
@@ -568,6 +589,7 @@ static int setup_rx_descbuffer(struct b4
skb = __dev_alloc_skb(ring-rx_buffersize, gfp_flags);
if (unlikely(!skb))
return -ENOMEM;
+   b43_poison_rx_buffer(ring, skb);
dmaaddr = map_descbuffer(ring, skb-data, ring-rx_buffersize, 0);
if (b43_dma_mapping_error(ring, dmaaddr, ring-rx_buffersize, 0)) {
/* ugh. try to realloc in zone_dma */
@@ -578,6 +600,7 @@ static int setup_rx_descbuffer(struct b4
skb = __dev_alloc_skb(ring-rx_buffersize, gfp_flags);
if (unlikely(!skb))
return -ENOMEM;
+   b43_poison_rx_buffer(ring, skb);
dmaaddr = map_descbuffer(ring, skb-data,
 ring-rx_buffersize, 0);
if (b43_dma_mapping_error(ring, dmaaddr, ring-rx_buffersize, 
0)) {
@@ -592,9 +615,6 @@ static int setup_rx_descbuffer(struct b4
ring-ops-fill_descriptor(ring, desc, dmaaddr,
   ring-rx_buffersize, 0, 0, 0);
 
-   rxhdr = (struct b43_rxhdr_fw4 *)(skb-data);
-   rxhdr-frame_len = 0;
-
return 0;
 }
 
@@ -1489,6 +1509,15 @@ static void dma_rx(struct b43_dmaring *r
goto drop;
}
}
+   if (unlikely(b43_rx_buffer_is_poisoned(ring, skb))) {
+   /* Something went wrong with the DMA.
+* The device did not touch the buffer and did not overwrite 
the poison. */
+   b43dbg(ring-dev-wl, DMA RX: Dropping poisoned buffer.\n);
+   /* recycle the descriptor buffer. */
+   sync_descbuffer_for_device(ring, meta-dmaaddr,
+  ring-rx_buffersize);
+   goto drop;
+   }
if (unlikely(len  ring-rx_buffersize)) {
/* The data did not fit into one descriptor buffer
 * and is split over multiple buffers.


-- 
Greetings, Michael.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: [PATCH] b43: Poison RX buffers

2009-03-27 Thread Michael Buesch 
On Saturday 28 March 2009 00:49:12 Francesco Gringoli wrote:
 On Mar 27, 2009, at 10:51 PM, Michael Buesch wrote:
 
  This patch adds poisoning and sanity checking to the RX DMA buffers.
  This is used for protection against buggy hardware/firmware that  
  raises
  RX interrupts without doing an actual DMA transfer.
 
  This mechanism protects against rare bad packets (due to  
  uninitialized skb data)
  and rare kernel crashes due to uninitialized RX headers.
 
 
  Francesco, please stresstest this.
 Hi Michael,
 
 thank you for the patch, I will test it ASAP. Before testing, however,  
 I would like to have a feedback about sanity checking directly the  
 rxhdr. Two reasons: 1) also with the patch I sent you yesterday, I  
 continued to observe kernel freezing with FCSFAIL set 2) I fear that  
 when dma_rx is triggered with such dma events, also the content of  
 rxhdr can be broken. In particular if the rxhdr-len reports an  
 incredible long frame, dma_rx could begin recycling too many skbs. I  
 was reported (by Bo) that when this happens, kernel would likely freeze.

This should be fixed by this patch as well.

 I did several tests with the open firmware and I found that the  
 maximum framelength that can be observed within firmware space is  
 0x1005 (with plcp). SPR_RXE_FRAMELEN _NEVER_ reports a value greater  
 than this (independently of FCSFAIL, I sampled its value in several  
 parts of the ucode). This does not surprise me as this should be the  
 maximum length possible, at least for OFDM frames (length is encoded  
 inside plcp in a twelve bits field that takes to 0xfff plus 6 bytes  
 from the plcp gives 0x1005). Given this hypothesis, however, we should  
 never observe any frame longer than 0x1005 in dma_rx as the rxhdr-len  
 field is always written by ucode before push_frame_into_fifo by  
 copying the value of SPR_RXE_FRAMELEN. Observing a greater value means  
 that what we think to be a frame sent the ucode, is nothing more than  
 random memory. So we could poison the rxhdr and verify in dma_rx that  
 it is still poisoned, in that case we could simply exit. How do you  
 think?

We already did that, but slightly incorrectly. Should be fixed by the patch
and the other one I just sent.

-- 
Greetings, Michael.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: [PATCH] b43: Poison RX buffers

2009-03-27 Thread Francesco Gringoli 

On Mar 28, 2009, at 12:53 AM, Michael Buesch wrote:

 On Saturday 28 March 2009 00:49:12 Francesco Gringoli wrote:
 On Mar 27, 2009, at 10:51 PM, Michael Buesch wrote:

 This patch adds poisoning and sanity checking to the RX DMA buffers.
 This is used for protection against buggy hardware/firmware that
 raises
 RX interrupts without doing an actual DMA transfer.

 This mechanism protects against rare bad packets (due to
 uninitialized skb data)
 and rare kernel crashes due to uninitialized RX headers.


 Francesco, please stresstest this.
 Hi Michael,

 thank you for the patch, I will test it ASAP. Before testing,  
 however,
 I would like to have a feedback about sanity checking directly the
 rxhdr. Two reasons: 1) also with the patch I sent you yesterday, I
 continued to observe kernel freezing with FCSFAIL set 2) I fear that
 when dma_rx is triggered with such dma events, also the content of
 rxhdr can be broken. In particular if the rxhdr-len reports an
 incredible long frame, dma_rx could begin recycling too many skbs. I
 was reported (by Bo) that when this happens, kernel would likely  
 freeze.

 This should be fixed by this patch as well.
Yes you are right :-) sorry. Testing tomorrow, pc is crashed in my  
office now. I'll let you know.

Cheers,
-FG

___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: [PATCH] b43: Poison RX buffers

2009-03-27 Thread Francesco Gringoli 
On Mar 27, 2009, at 10:51 PM, Michael Buesch wrote:

 This patch adds poisoning and sanity checking to the RX DMA buffers.
 This is used for protection against buggy hardware/firmware that  
 raises
 RX interrupts without doing an actual DMA transfer.

 This mechanism protects against rare bad packets (due to  
 uninitialized skb data)
 and rare kernel crashes due to uninitialized RX headers.


 Francesco, please stresstest this.
Hi Michael,

thank you for the patch, I will test it ASAP. Before testing, however,  
I would like to have a feedback about sanity checking directly the  
rxhdr. Two reasons: 1) also with the patch I sent you yesterday, I  
continued to observe kernel freezing with FCSFAIL set 2) I fear that  
when dma_rx is triggered with such dma events, also the content of  
rxhdr can be broken. In particular if the rxhdr-len reports an  
incredible long frame, dma_rx could begin recycling too many skbs. I  
was reported (by Bo) that when this happens, kernel would likely freeze.

I did several tests with the open firmware and I found that the  
maximum framelength that can be observed within firmware space is  
0x1005 (with plcp). SPR_RXE_FRAMELEN _NEVER_ reports a value greater  
than this (independently of FCSFAIL, I sampled its value in several  
parts of the ucode). This does not surprise me as this should be the  
maximum length possible, at least for OFDM frames (length is encoded  
inside plcp in a twelve bits field that takes to 0xfff plus 6 bytes  
from the plcp gives 0x1005). Given this hypothesis, however, we should  
never observe any frame longer than 0x1005 in dma_rx as the rxhdr-len  
field is always written by ucode before push_frame_into_fifo by  
copying the value of SPR_RXE_FRAMELEN. Observing a greater value means  
that what we think to be a frame sent the ucode, is nothing more than  
random memory. So we could poison the rxhdr and verify in dma_rx that  
it is still poisoned, in that case we could simply exit. How do you  
think?

Cheers,
-FG


 John, please queue as bugfix.


 Index: wireless-testing/drivers/net/wireless/b43/dma.c
 ===
 --- wireless-testing.orig/drivers/net/wireless/b43/dma.c  2009-03-27  
 12:29:57.0 +0100
 +++ wireless-testing/drivers/net/wireless/b43/dma.c   2009-03-27  
 22:22:21.0 +0100
 @@ -555,11 +555,32 @@ address_error:
   return 1;
 }

 +static bool b43_rx_buffer_is_poisoned(struct b43_dmaring *ring,  
 struct sk_buff *skb)
 +{
 + unsigned char *f = skb-data + ring-frameoffset;
 +
 + return ((f[0]  f[1]  f[2]  f[3]  f[4]  f[5]  f[6]  f[7]) ==  
 0xFF);
 +}
 +
 +static void b43_poison_rx_buffer(struct b43_dmaring *ring, struct  
 sk_buff *skb)
 +{
 + struct b43_rxhdr_fw4 *rxhdr;
 + unsigned char *frame;
 +
 + /* This poisons the RX buffer to detect DMA failures. */
 +
 + rxhdr = (struct b43_rxhdr_fw4 *)(skb-data);
 + rxhdr-frame_len = 0;
 +
 + B43_WARN_ON(ring-rx_buffersize  ring-frameoffset +  
 sizeof(struct b43_plcp_hdr6) + 2);
 + frame = skb-data + ring-frameoffset;
 + memset(frame, 0xFF, sizeof(struct b43_plcp_hdr6) + 2 /* padding */);
 +}
 +
 static int setup_rx_descbuffer(struct b43_dmaring *ring,
  struct b43_dmadesc_generic *desc,
  struct b43_dmadesc_meta *meta, gfp_t gfp_flags)
 {
 - struct b43_rxhdr_fw4 *rxhdr;
   dma_addr_t dmaaddr;
   struct sk_buff *skb;

 @@ -568,6 +589,7 @@ static int setup_rx_descbuffer(struct b4
   skb = __dev_alloc_skb(ring-rx_buffersize, gfp_flags);
   if (unlikely(!skb))
   return -ENOMEM;
 + b43_poison_rx_buffer(ring, skb);
   dmaaddr = map_descbuffer(ring, skb-data, ring-rx_buffersize, 0);
   if (b43_dma_mapping_error(ring, dmaaddr, ring-rx_buffersize, 0)) {
   /* ugh. try to realloc in zone_dma */
 @@ -578,6 +600,7 @@ static int setup_rx_descbuffer(struct b4
   skb = __dev_alloc_skb(ring-rx_buffersize, gfp_flags);
   if (unlikely(!skb))
   return -ENOMEM;
 + b43_poison_rx_buffer(ring, skb);
   dmaaddr = map_descbuffer(ring, skb-data,
ring-rx_buffersize, 0);
   if (b43_dma_mapping_error(ring, dmaaddr, ring-rx_buffersize, 
 0)) {
 @@ -592,9 +615,6 @@ static int setup_rx_descbuffer(struct b4
   ring-ops-fill_descriptor(ring, desc, dmaaddr,
  ring-rx_buffersize, 0, 0, 0);

 - rxhdr = (struct b43_rxhdr_fw4 *)(skb-data);
 - rxhdr-frame_len = 0;
 -
   return 0;
 }

 @@ -1489,6 +1509,15 @@ static void dma_rx(struct b43_dmaring *r
   goto drop;
   }
   }
 + if (unlikely(b43_rx_buffer_is_poisoned(ring, skb))) {
 + /* Something went wrong with the DMA.
 +  * The device did not touch the buffer and did not overwrite 
 the  
 poison. */
 +