| Message ID | 1297892656-2190-4-git-send-email-seth.forshee@canonical.com |
|---|---|
| State | Accepted |
| Commit | 87bca89963656a274828d3fa54a379374bc6d24a |
| Headers | show |
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, ®); > + orig_reg = reg; > rt2x00_set_field32(®, 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\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 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, ®); > + orig_reg = reg; > rt2x00_set_field32(®, 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\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);
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.
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
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, ®); + orig_reg = reg; rt2x00_set_field32(®, 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\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 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, ®); + orig_reg = reg; rt2x00_set_field32(®, 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\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);
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(-)