Missing skb_pad() return value checks in rt2x00 driver

Missing skb_pad() return value checks in rt2x00 driver

[Seth Forshee]

In looking at the following commit:

  commit 739fd9405416e22732e46a9226a8cac379bd57fc
  Author: Wolfgang Kufner <wolfgang.kufner@gmail.com>
  Date:   Mon Dec 13 12:39:12 2010 +0100
  
      rt2x00: Pad beacon to multiple of 32 bits.

I noticed that the skb_pad() calls it introduces are not being checked
for failure. I don't know whether or not it's possible for these calls
to fail in this context; would it make sense to incorporate the patch
below?

Thanks,
Seth

>From 6cebee1731e158df888e5ec19f5c221ac8a1a213 Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Mon, 31 Jan 2011 15:06:47 -0600
Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls

Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
added calls to skb_pad() without checking the return value,
which could cause problems if any of those calls does happen
to fail. Add checks to prevent this from happening and move
the padding to the tops of the relevant functions so we can
bail out before writing to any registers.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/net/wireless/rt2x00/rt2800lib.c |   13 +++++++++++--
 drivers/net/wireless/rt2x00/rt61pci.c   |   13 +++++++++++--
 drivers/net/wireless/rt2x00/rt73usb.c   |   13 +++++++++++--
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index f8ba01c..79d17da 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -776,6 +776,17 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	u32 reg;
 
 	/*
+	 * Pad out the beacon to a 32-bit boundary
+	 */
+	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		return;
+	}
+
+	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
@@ -809,8 +820,6 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	/*
 	 * Write entire beacon with TXWI and padding to register.
 	 */
-	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				   entry->skb->len + padding_len);
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index 8de44dd..83ac31e 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -1965,6 +1965,17 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	u32 reg;
 
 	/*
+	 * Pad out the beacon to a 32-bit boundary
+	 */
+	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		return;
+	}
+
+	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
@@ -1985,8 +1996,6 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	/*
 	 * Write entire beacon with descriptor and padding to register.
 	 */
-	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
 				      entry_priv->desc, TXINFO_SIZE);
diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
index 029be3c..5b15609 100644
--- a/drivers/net/wireless/rt2x00/rt73usb.c
+++ b/drivers/net/wireless/rt2x00/rt73usb.c
@@ -1550,6 +1550,17 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	u32 reg;
 
 	/*
+	 * Pad out the beacon to a 32-bit boundary
+	 */
+	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		return;
+	}
+
+	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
@@ -1576,8 +1587,6 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	/*
 	 * Write entire beacon with descriptor and padding to register.
 	 */
-	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00usb_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				      entry->skb->len + padding_len);
-- 
1.7.1

Re: Missing skb_pad() return value checks in rt2x00 driver

[Stanislaw Gruszka]

On Mon, Jan 31, 2011 at 03:45:16PM -0600, Seth Forshee wrote:
> Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
> added calls to skb_pad() without checking the return value,
> which could cause problems if any of those calls does happen
> to fail. Add checks to prevent this from happening and move
> the padding to the tops of the relevant functions so we can
> bail out before writing to any registers.

Make sense for me.

Ivo, Gertjan? 

> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>

>  drivers/net/wireless/rt2x00/rt2800lib.c |   13 +++++++++++--
>  drivers/net/wireless/rt2x00/rt61pci.c   |   13 +++++++++++--
>  drivers/net/wireless/rt2x00/rt73usb.c   |   13 +++++++++++--
>  3 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index f8ba01c..79d17da 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -776,6 +776,17 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>  	u32 reg;
>  
>  	/*
> +	 * Pad out the beacon to a 32-bit boundary
> +	 */
> +	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> +	if (padding_len && skb_pad(entry->skb, padding_len)) {
> +		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
> +		/* skb freed by skb_pad() on failure */
> +		entry->skb = NULL;
> +		return;
> +	}
> +
> +	/*
>  	 * Disable beaconing while we are reloading the beacon data,
>  	 * otherwise we might be sending out invalid data.
>  	 */
> @@ -809,8 +820,6 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>  	/*
>  	 * Write entire beacon with TXWI and padding to register.
>  	 */
> -	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -	skb_pad(entry->skb, padding_len);
>  	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>  	rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
>  				   entry->skb->len + padding_len);
> diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
> index 8de44dd..83ac31e 100644
> --- a/drivers/net/wireless/rt2x00/rt61pci.c
> +++ b/drivers/net/wireless/rt2x00/rt61pci.c
> @@ -1965,6 +1965,17 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
>  	u32 reg;
>  
>  	/*
> +	 * Pad out the beacon to a 32-bit boundary
> +	 */
> +	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> +	if (padding_len && skb_pad(entry->skb, padding_len)) {
> +		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
> +		/* skb freed by skb_pad() on failure */
> +		entry->skb = NULL;
> +		return;
> +	}
> +
> +	/*
>  	 * Disable beaconing while we are reloading the beacon data,
>  	 * otherwise we might be sending out invalid data.
>  	 */
> @@ -1985,8 +1996,6 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
>  	/*
>  	 * Write entire beacon with descriptor and padding to register.
>  	 */
> -	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -	skb_pad(entry->skb, padding_len);
>  	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>  	rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
>  				      entry_priv->desc, TXINFO_SIZE);
> diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
> index 029be3c..5b15609 100644
> --- a/drivers/net/wireless/rt2x00/rt73usb.c
> +++ b/drivers/net/wireless/rt2x00/rt73usb.c
> @@ -1550,6 +1550,17 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
>  	u32 reg;
>  
>  	/*
> +	 * Pad out the beacon to a 32-bit boundary
> +	 */
> +	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> +	if (padding_len && skb_pad(entry->skb, padding_len)) {
> +		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
> +		/* skb freed by skb_pad() on failure */
> +		entry->skb = NULL;
> +		return;
> +	}
> +
> +	/*
>  	 * Disable beaconing while we are reloading the beacon data,
>  	 * otherwise we might be sending out invalid data.
>  	 */
> @@ -1576,8 +1587,6 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
>  	/*
>  	 * Write entire beacon with descriptor and padding to register.
>  	 */
> -	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -	skb_pad(entry->skb, padding_len);
>  	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>  	rt2x00usb_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
>  				      entry->skb->len + padding_len);
> -- 
> 1.7.1
> 
> --
> 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: Missing skb_pad() return value checks in rt2x00 driver

[Ivo Van Doorn]

Hi,

> I noticed that the skb_pad() calls it introduces are not being checked
> for failure. I don't know whether or not it's possible for these calls
> to fail in this context; would it make sense to incorporate the patch
> below?

The suggested change sounds good, however I'm a bit worried
about the relocation of the padding.

> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -776,6 +776,17 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>        u32 reg;
>
>        /*
> +        * Pad out the beacon to a 32-bit boundary
> +        */
> +       padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> +               dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
> +               /* skb freed by skb_pad() on failure */
> +               entry->skb = NULL;
> +               return;
> +       }
> +
> +       /*
>         * Disable beaconing while we are reloading the beacon data,
>         * otherwise we might be sending out invalid data.
>         */
> @@ -809,8 +820,6 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>        /*
>         * Write entire beacon with TXWI and padding to register.
>         */
> -       padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -       skb_pad(entry->skb, padding_len);
>        beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>        rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
>                                   entry->skb->len + padding_len);

Between here and where you added the padding are a couple of function
calls which use the skb->len field. So this patch would change the value what
they expect. Have you checked the possible impact?

Ivo

Re: Missing skb_pad() return value checks in rt2x00 driver

[Wolfgang Kufner]

Hi Ivo,

On Sun, Feb 6, 2011 at 5:17 PM, Ivo Van Doorn <ivdoorn@gmail.com> wrote:
> Between here and where you added the padding are a couple of function
> calls which use the skb->len field. So this patch would change the value what
> they expect. Have you checked the possible impact?

I don't think skb_pad() touches skb->len. It writes zeros into the tailroom:
/**
*      skb_pad                 -       zero pad the tail of an skb
*      @skb: buffer to pad
*      @pad: space to pad
*
*      Ensure that a buffer is followed by a padding area that is zero
*      filled. Used by network drivers which may DMA or transfer data
*      beyond the buffer end onto the wire.
*
*      May return error in out of memory cases. The skb is freed on error.
*/

Thanks,
Wolfgang

Re: Missing skb_pad() return value checks in rt2x00 driver

[Seth Forshee]

On Sun, Feb 06, 2011 at 08:37:21PM +0100, Wolfgang Kufner wrote:
> Hi Ivo,
> 
> On Sun, Feb 6, 2011 at 5:17 PM, Ivo Van Doorn <ivdoorn@gmail.com> wrote:
> > Between here and where you added the padding are a couple of function
> > calls which use the skb->len field. So this patch would change the value what
> > they expect. Have you checked the possible impact?
> 
> I don't think skb_pad() touches skb->len. It writes zeros into the tailroom:

Agreed, it doesn't look like skb_pad() affects that field.

One point of concern though would be operations that changed the length
so that the amount of padding applied was no longer correct. That isn't
the case here, but it does make more sense logically that the padding
would follow any adjustments to skb->len.

My reason for moving it was that if the padding failed at the original
location some data would have already been written to the hardware,
potentially leaving it in a bad state (at a minimum it looks like
beaconing would be left disabled). Maybe a better option would be to
leave the padding at the original location and add some cleanup for the
failure case. I don't have this hardware and am not familiar with it,
but I'd be happy to rework the patch if someone who knows the hardware
can advise me on what cleanup should be done.

Re: Missing skb_pad() return value checks in rt2x00 driver

[Seth Forshee]

On Sun, Feb 06, 2011 at 10:10:07PM -0600, Seth Forshee wrote:
> On Sun, Feb 06, 2011 at 08:37:21PM +0100, Wolfgang Kufner wrote:
> > Hi Ivo,
> > 
> > On Sun, Feb 6, 2011 at 5:17 PM, Ivo Van Doorn <ivdoorn@gmail.com> wrote:
> > > Between here and where you added the padding are a couple of function
> > > calls which use the skb->len field. So this patch would change the value what
> > > they expect. Have you checked the possible impact?
> > 
> > I don't think skb_pad() touches skb->len. It writes zeros into the tailroom:
> 
> Agreed, it doesn't look like skb_pad() affects that field.
> 
> One point of concern though would be operations that changed the length
> so that the amount of padding applied was no longer correct. That isn't
> the case here, but it does make more sense logically that the padding
> would follow any adjustments to skb->len.

I reworked the patch to use the clean-up approach, naively assuming that
we should restore the original beaconing state on failure and that other
writes require no clean-up. Please provide feedback as to whether this
general approach is better and whether adjustments to the clean-up are
required.


>From 4a8b3969775cd233750a1b74b1d4f5fc065932bf Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Mon, 7 Feb 2011 13:50:33 -0600
Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls

Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
added calls to skb_pad() without checking the return value,
which could cause problems if any of those calls does happen
to fail. Add checks to prevent this from happening.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/net/wireless/rt2x00/rt2800lib.c |   12 ++++++++++--
 drivers/net/wireless/rt2x00/rt61pci.c   |   12 ++++++++++--
 drivers/net/wireless/rt2x00/rt73usb.c   |   12 ++++++++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index c9bf074..0f439ba 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -773,13 +773,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	struct skb_frame_desc *skbdesc = get_skb_frame_desc(entry->skb);
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2800_register_read(rt2x00dev, BCN_TIME_CFG, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, BCN_TIME_CFG_BEACON_GEN, 0);
 	rt2800_register_write(rt2x00dev, BCN_TIME_CFG, reg);
 
@@ -810,7 +811,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	 * Write entire beacon with TXWI and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2800_register_write(rt2x00dev, BCN_TIME_CFG, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				   entry->skb->len + padding_len);
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index dd2164d..76d9c3a 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -1978,13 +1978,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	struct queue_entry_priv_pci *entry_priv = entry->priv_data;
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2x00pci_register_read(rt2x00dev, TXRX_CSR9, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
 	rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, reg);
 
@@ -2002,7 +2003,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	 * Write entire beacon with descriptor and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
 				      entry_priv->desc, TXINFO_SIZE);
diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
index 5ff72de..79becd7 100644
--- a/drivers/net/wireless/rt2x00/rt73usb.c
+++ b/drivers/net/wireless/rt2x00/rt73usb.c
@@ -1533,13 +1533,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2x00usb_register_read(rt2x00dev, TXRX_CSR9, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
 	rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, reg);
 
@@ -1563,7 +1564,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	 * Write entire beacon with descriptor and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00usb_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				      entry->skb->len + padding_len);
-- 
1.7.1

Re: Missing skb_pad() return value checks in rt2x00 driver

[Ivo Van Doorn]

Hi,

Well this patch managed to slip through my list. :S

> From 4a8b3969775cd233750a1b74b1d4f5fc065932bf Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Mon, 7 Feb 2011 13:50:33 -0600
> Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls
>
> Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
> added calls to skb_pad() without checking the return value,
> which could cause problems if any of those calls does happen
> to fail. Add checks to prevent this from happening.

Thanks for the updated patch. I only have one small request.

> +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> +               dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");

Please change to:
  ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
same for the other changed drivers in this patch.

Thanks,

Ivo

Re: Missing skb_pad() return value checks in rt2x00 driver

[Seth Forshee]

On Mon, Feb 14, 2011 at 11:03:36AM +0100, Ivo Van Doorn wrote:
> Hi,
> 
> Well this patch managed to slip through my list. :S

No problem, glad it finally caught your attention :)

> > From 4a8b3969775cd233750a1b74b1d4f5fc065932bf Mon Sep 17 00:00:00 2001
> > From: Seth Forshee <seth.forshee@canonical.com>
> > Date: Mon, 7 Feb 2011 13:50:33 -0600
> > Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls
> >
> > Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
> > added calls to skb_pad() without checking the return value,
> > which could cause problems if any of those calls does happen
> > to fail. Add checks to prevent this from happening.
> 
> Thanks for the updated patch. I only have one small request.
> 
> > +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> > +               dev_err(rt2x00dev->dev, "Failure padding beacon, aborting\n");
> 
> Please change to:
>   ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
> same for the other changed drivers in this patch.

Done, updated patch follows.


>From 0de24c34fc6f59d1d557a3cb16adcc32414d4c46 Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Mon, 14 Feb 2011 08:52:25 -0600
Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls

Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
added calls to skb_pad() without checking the return value,
which could cause problems if any of those calls does happen
to fail. Add checks to prevent this from happening.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/net/wireless/rt2x00/rt2800lib.c |   12 ++++++++++--
 drivers/net/wireless/rt2x00/rt61pci.c   |   12 ++++++++++--
 drivers/net/wireless/rt2x00/rt73usb.c   |   12 ++++++++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index c9bf074..7a68a67 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -773,13 +773,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	struct skb_frame_desc *skbdesc = get_skb_frame_desc(entry->skb);
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2800_register_read(rt2x00dev, BCN_TIME_CFG, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, BCN_TIME_CFG_BEACON_GEN, 0);
 	rt2800_register_write(rt2x00dev, BCN_TIME_CFG, reg);
 
@@ -810,7 +811,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
 	 * Write entire beacon with TXWI and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2800_register_write(rt2x00dev, BCN_TIME_CFG, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				   entry->skb->len + padding_len);
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index dd2164d..927a4a3 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -1978,13 +1978,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	struct queue_entry_priv_pci *entry_priv = entry->priv_data;
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2x00pci_register_read(rt2x00dev, TXRX_CSR9, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
 	rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, reg);
 
@@ -2002,7 +2003,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
 	 * Write entire beacon with descriptor and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
 				      entry_priv->desc, TXINFO_SIZE);
diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
index 5ff72de..6e9981a 100644
--- a/drivers/net/wireless/rt2x00/rt73usb.c
+++ b/drivers/net/wireless/rt2x00/rt73usb.c
@@ -1533,13 +1533,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
 	unsigned int beacon_base;
 	unsigned int padding_len;
-	u32 reg;
+	u32 orig_reg, reg;
 
 	/*
 	 * Disable beaconing while we are reloading the beacon data,
 	 * otherwise we might be sending out invalid data.
 	 */
 	rt2x00usb_register_read(rt2x00dev, TXRX_CSR9, &reg);
+	orig_reg = reg;
 	rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
 	rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, reg);
 
@@ -1563,7 +1564,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
 	 * Write entire beacon with descriptor and padding to register.
 	 */
 	padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
-	skb_pad(entry->skb, padding_len);
+	if (padding_len && skb_pad(entry->skb, padding_len)) {
+		ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
+		/* skb freed by skb_pad() on failure */
+		entry->skb = NULL;
+		rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
+		return;
+	}
+
 	beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
 	rt2x00usb_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
 				      entry->skb->len + padding_len);
-- 
1.7.1

Re: Missing skb_pad() return value checks in rt2x00 driver

[Ivo Van Doorn]

Hi,

> From 0de24c34fc6f59d1d557a3cb16adcc32414d4c46 Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Mon, 14 Feb 2011 08:52:25 -0600
> Subject: [PATCH] rt2x00: Check for errors from skb_pad() calls
>
> Commit 739fd94 ("rt2x00: Pad beacon to multiple of 32 bits")
> added calls to skb_pad() without checking the return value,
> which could cause problems if any of those calls does happen
> to fail. Add checks to prevent this from happening.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>

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

> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |   12 ++++++++++--
>  drivers/net/wireless/rt2x00/rt61pci.c   |   12 ++++++++++--
>  drivers/net/wireless/rt2x00/rt73usb.c   |   12 ++++++++++--
>  3 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index c9bf074..7a68a67 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -773,13 +773,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>        struct skb_frame_desc *skbdesc = get_skb_frame_desc(entry->skb);
>        unsigned int beacon_base;
>        unsigned int padding_len;
> -       u32 reg;
> +       u32 orig_reg, reg;
>
>        /*
>         * Disable beaconing while we are reloading the beacon data,
>         * otherwise we might be sending out invalid data.
>         */
>        rt2800_register_read(rt2x00dev, BCN_TIME_CFG, &reg);
> +       orig_reg = reg;
>        rt2x00_set_field32(&reg, BCN_TIME_CFG_BEACON_GEN, 0);
>        rt2800_register_write(rt2x00dev, BCN_TIME_CFG, reg);
>
> @@ -810,7 +811,14 @@ void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc *txdesc)
>         * Write entire beacon with TXWI and padding to register.
>         */
>        padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -       skb_pad(entry->skb, padding_len);
> +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> +               ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
> +               /* skb freed by skb_pad() on failure */
> +               entry->skb = NULL;
> +               rt2800_register_write(rt2x00dev, BCN_TIME_CFG, orig_reg);
> +               return;
> +       }
> +
>        beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>        rt2800_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
>                                   entry->skb->len + padding_len);
> diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
> index dd2164d..927a4a3 100644
> --- a/drivers/net/wireless/rt2x00/rt61pci.c
> +++ b/drivers/net/wireless/rt2x00/rt61pci.c
> @@ -1978,13 +1978,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
>        struct queue_entry_priv_pci *entry_priv = entry->priv_data;
>        unsigned int beacon_base;
>        unsigned int padding_len;
> -       u32 reg;
> +       u32 orig_reg, reg;
>
>        /*
>         * Disable beaconing while we are reloading the beacon data,
>         * otherwise we might be sending out invalid data.
>         */
>        rt2x00pci_register_read(rt2x00dev, TXRX_CSR9, &reg);
> +       orig_reg = reg;
>        rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
>        rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, reg);
>
> @@ -2002,7 +2003,14 @@ static void rt61pci_write_beacon(struct queue_entry *entry,
>         * Write entire beacon with descriptor and padding to register.
>         */
>        padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -       skb_pad(entry->skb, padding_len);
> +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> +               ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
> +               /* skb freed by skb_pad() on failure */
> +               entry->skb = NULL;
> +               rt2x00pci_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
> +               return;
> +       }
> +
>        beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>        rt2x00pci_register_multiwrite(rt2x00dev, beacon_base,
>                                      entry_priv->desc, TXINFO_SIZE);
> diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
> index 5ff72de..6e9981a 100644
> --- a/drivers/net/wireless/rt2x00/rt73usb.c
> +++ b/drivers/net/wireless/rt2x00/rt73usb.c
> @@ -1533,13 +1533,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
>        struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>        unsigned int beacon_base;
>        unsigned int padding_len;
> -       u32 reg;
> +       u32 orig_reg, reg;
>
>        /*
>         * Disable beaconing while we are reloading the beacon data,
>         * otherwise we might be sending out invalid data.
>         */
>        rt2x00usb_register_read(rt2x00dev, TXRX_CSR9, &reg);
> +       orig_reg = reg;
>        rt2x00_set_field32(&reg, TXRX_CSR9_BEACON_GEN, 0);
>        rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, reg);
>
> @@ -1563,7 +1564,14 @@ static void rt73usb_write_beacon(struct queue_entry *entry,
>         * Write entire beacon with descriptor and padding to register.
>         */
>        padding_len = roundup(entry->skb->len, 4) - entry->skb->len;
> -       skb_pad(entry->skb, padding_len);
> +       if (padding_len && skb_pad(entry->skb, padding_len)) {
> +               ERROR(rt2x00dev, "Failure padding beacon, aborting\n");
> +               /* skb freed by skb_pad() on failure */
> +               entry->skb = NULL;
> +               rt2x00usb_register_write(rt2x00dev, TXRX_CSR9, orig_reg);
> +               return;
> +       }
> +
>        beacon_base = HW_BEACON_OFFSET(entry->entry_idx);
>        rt2x00usb_register_multiwrite(rt2x00dev, beacon_base, entry->skb->data,
>                                      entry->skb->len + padding_len);
> --
> 1.7.1
>
>