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

[Stanislaw Gruszka <sgruszka@redhat.com>]

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