Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Ubuntu Kernel Team (http://www.linux-archive.org/ubuntu-kernel-team/)
-   -   rt2x00: Check for errors from skb_pad() calls (http://www.linux-archive.org/ubuntu-kernel-team/490364-rt2x00-check-errors-skb_pad-calls.html)

Seth Forshee 02-16-2011 08:44 PM

rt2x00: Check for errors from skb_pad() calls
 
BugLink: http://bugs.launchpad.net/bugs/659143

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.

(backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
drivers/net/wireless/rt2x00/rt2800pci.c | 11 +++++++++--
drivers/net/wireless/rt2x00/rt61pci.c | 11 +++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index 9ad676d..e13666e 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -690,13 +690,14 @@ static void rt2800pci_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.
*/
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);

@@ -710,7 +711,13 @@ static void rt2800pci_write_beacon(struct queue_entry *entry,
* 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
");
+ /* 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 23bcb65..f4d1ec9 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -1864,13 +1864,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);

@@ -1878,7 +1879,13 @@ 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
");
+ /* 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);
--
1.7.1


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Stefan Bader 02-17-2011 01:19 PM

rt2x00: Check for errors from skb_pad() calls
 
On 02/16/2011 10:44 PM, Seth Forshee wrote:
> BugLink: http://bugs.launchpad.net/bugs/659143
>
> 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.
>
- (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
+ (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
+ Signed-off-by: Seth ...
?

Generally I am in favour of waiting until everything hits upstream and then play
nice and try to get it into upstream stable (which has been become a bit harder
as one never knows exactly who that is for which kernel). Though this is a
regression and wireless-next should become soonish upstream. So

Acked-by: Stefan Bader <stefan.bader@canonical.com> (for all three)

> ---
> drivers/net/wireless/rt2x00/rt2800pci.c | 11 +++++++++--
> drivers/net/wireless/rt2x00/rt61pci.c | 11 +++++++++--
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 9ad676d..e13666e 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -690,13 +690,14 @@ static void rt2800pci_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.
> */
> 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);
>
> @@ -710,7 +711,13 @@ static void rt2800pci_write_beacon(struct queue_entry *entry,
> * 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
");
> + /* 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 23bcb65..f4d1ec9 100644
> --- a/drivers/net/wireless/rt2x00/rt61pci.c
> +++ b/drivers/net/wireless/rt2x00/rt61pci.c
> @@ -1864,13 +1864,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);
>
> @@ -1878,7 +1879,13 @@ 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
");
> + /* 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);


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Seth Forshee 02-17-2011 02:23 PM

rt2x00: Check for errors from skb_pad() calls
 
On Thu, Feb 17, 2011 at 03:19:35PM +0100, Stefan Bader wrote:
> On 02/16/2011 10:44 PM, Seth Forshee wrote:
> > BugLink: http://bugs.launchpad.net/bugs/659143
> >
> > 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.
> >
> - (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> + (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
> + Signed-off-by: Seth ...
> ?
>
> Generally I am in favour of waiting until everything hits upstream and then play
> nice and try to get it into upstream stable (which has been become a bit harder
> as one never knows exactly who that is for which kernel). Though this is a
> regression and wireless-next should become soonish upstream. So

This isn't a candidate for stable, as the first two patches (without
which this patch isn't needed) first appeared in .38-rc1 and afaik
aren't in stable. And it doesn't look like John Linville plans to submit
it to Linus until .39 since he put it in the wireless-next tree.

And in practice I don't know that anyone has actually seen the skb_pad()
calls failing. I just noticed it when reviewing the patches and fixed
it, and thought that we should include it in maverick with the first two
to avoid the possibility of introducing a new regression. But we can
take the first two and leave this one out if you'd rather.

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Stefan Bader 02-17-2011 02:45 PM

rt2x00: Check for errors from skb_pad() calls
 
On 02/17/2011 04:23 PM, Seth Forshee wrote:
> On Thu, Feb 17, 2011 at 03:19:35PM +0100, Stefan Bader wrote:
>> On 02/16/2011 10:44 PM, Seth Forshee wrote:
>>> BugLink: http://bugs.launchpad.net/bugs/659143
>>>
>>> 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.
>>>
>> - (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>> Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
>>> Signed-off-by: John W. Linville <linville@tuxdriver.com>
>> + (backported from d76dfc612b40b6a9de0a3fe57fe1fa3db7a1ae3b wireless-next-2.6)
>> + Signed-off-by: Seth ...
>> ?
>>
>> Generally I am in favour of waiting until everything hits upstream and then play
>> nice and try to get it into upstream stable (which has been become a bit harder
>> as one never knows exactly who that is for which kernel). Though this is a
>> regression and wireless-next should become soonish upstream. So
>
> This isn't a candidate for stable, as the first two patches (without
> which this patch isn't needed) first appeared in .38-rc1 and afaik
> aren't in stable. And it doesn't look like John Linville plans to submit
> it to Linus until .39 since he put it in the wireless-next tree.
>
> And in practice I don't know that anyone has actually seen the skb_pad()
> calls failing. I just noticed it when reviewing the patches and fixed
> it, and thought that we should include it in maverick with the first two
> to avoid the possibility of introducing a new regression. But we can
> take the first two and leave this one out if you'd rather.

I did not intend to say that. More that it helps to have everything together in
Linus tree, then one can work on the upstream stable task as well as the sru
task. The third patch does make sense. And I think it is ok to sru it with the
rest in this case of an regression.

The main concern about things not being in Linus tree is that they may be found
wrong before they reach and then missing the correct change when it does. This
also helps to prevent problems thought to be fixed to turn up again in the next
release. In theory the way to sru things (in general/other packages) is to have
the fix tested in the development branch and then take it back to older
releases. Of course we cannot always do that.

-Stefan

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team


All times are GMT. The time now is 09:24 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.