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.000000000 +0100
> +++ wireless-testing/drivers/net/wireless/b43/dma.c 2009-03-27
> 22:22:21.000000000 +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.
-------
Francesco Gringoli, PhD - Assistant Professor
Dept. of Electrical Engineering for Automation
University of Brescia
via Branze, 38
25123 Brescia
ITALY
Ph: ++39.030.3715843
FAX: ++39.030.380014
WWW: http://www.ing.unibs.it/~gringoli
_______________________________________________
Bcm43xx-dev mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
