Patchwork [natty,CVE,1/1] bonding: Incorrect TX queue offset

login
register
mail settings
Submitter Andy Whitcroft
Date June 6, 2011, 10:35 a.m.
Message ID <1307356523-6615-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/98867/
State New
Headers show

Comments

Andy Whitcroft - June 6, 2011, 10:35 a.m.
From: Phil Oester <kernel@linuxace.com>

When packets come in from a device with >= 16 receive queues
headed out a bonding interface, syslog gets filled with this:

    kernel: bond0 selects TX queue 16, but real number of TX queues is 16

because queue_mapping is offset by 1.  Adjust return value
to account for the offset.

This is a revision of my earlier patch (which did not use the
skb_rx_queue_* helpers - thanks to Ben for the suggestion).
Andy submitted a similar patch which emits a pr_warning on
invalid queue selection, but I believe the log spew is
not useful.  We can revisit that question in the future,
but in the interim I believe fixing the core problem is
worthwhile.

Signed-off-by: Phil Oester <kernel@linuxace.com>
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: David S. Miller <davem@davemloft.net>

(cherry picked from commit fd0e435b0fe85622f167b84432552885a4856ac8)
CVE-2011-1581
BugLink: http://bugs.launchpad.net/bugs/792312
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/net/bonding/bond_main.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)
Stefan Bader - June 6, 2011, 12:06 p.m.
On 06.06.2011 12:35, Andy Whitcroft wrote:
> From: Phil Oester<kernel@linuxace.com>
>
> When packets come in from a device with>= 16 receive queues
> headed out a bonding interface, syslog gets filled with this:
>
>      kernel: bond0 selects TX queue 16, but real number of TX queues is 16
>
> because queue_mapping is offset by 1.  Adjust return value
> to account for the offset.
>
> This is a revision of my earlier patch (which did not use the
> skb_rx_queue_* helpers - thanks to Ben for the suggestion).
> Andy submitted a similar patch which emits a pr_warning on
> invalid queue selection, but I believe the log spew is
> not useful.  We can revisit that question in the future,
> but in the interim I believe fixing the core problem is
> worthwhile.
>
> Signed-off-by: Phil Oester<kernel@linuxace.com>
> Signed-off-by: Andy Gospodarek<andy@greyhouse.net>
> Signed-off-by: David S. Miller<davem@davemloft.net>
>
> (cherry picked from commit fd0e435b0fe85622f167b84432552885a4856ac8)
> CVE-2011-1581
> BugLink: http://bugs.launchpad.net/bugs/792312
> Signed-off-by: Andy Whitcroft<apw@canonical.com>
> ---
>   drivers/net/bonding/bond_main.c |   11 +++++++++--
>   1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 163e0b0..e68eaef 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4537,11 +4537,18 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb)
>   {
>   	/*
>   	 * This helper function exists to help dev_pick_tx get the correct
> -	 * destination queue.  Using a helper function skips the a call to
> +	 * destination queue.  Using a helper function skips a call to
>   	 * skb_tx_hash and will put the skbs in the queue we expect on their
>   	 * way down to the bonding driver.
>   	 */
> -	return skb->queue_mapping;
> +	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> +
> +	if (unlikely(txq>= dev->real_num_tx_queues)) {
> +		do
> +			txq -= dev->real_num_tx_queues;
> +		while (txq>= dev->real_num_tx_queues);
> +	}
> +	return txq;
>   }
>
>   static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)

Acked-by: Stefan Bader <stefan.bader@canonical.com>

(though more for being clean cherry pick, rather than clearness)
Leann Ogasawara - June 6, 2011, 1:15 p.m.
On Mon, 2011-06-06 at 14:06 +0200, Stefan Bader wrote:
> On 06.06.2011 12:35, Andy Whitcroft wrote:
> > From: Phil Oester<kernel@linuxace.com>
> >
> > When packets come in from a device with>= 16 receive queues
> > headed out a bonding interface, syslog gets filled with this:
> >
> >      kernel: bond0 selects TX queue 16, but real number of TX queues is 16
> >
> > because queue_mapping is offset by 1.  Adjust return value
> > to account for the offset.
> >
> > This is a revision of my earlier patch (which did not use the
> > skb_rx_queue_* helpers - thanks to Ben for the suggestion).
> > Andy submitted a similar patch which emits a pr_warning on
> > invalid queue selection, but I believe the log spew is
> > not useful.  We can revisit that question in the future,
> > but in the interim I believe fixing the core problem is
> > worthwhile.
> >
> > Signed-off-by: Phil Oester<kernel@linuxace.com>
> > Signed-off-by: Andy Gospodarek<andy@greyhouse.net>
> > Signed-off-by: David S. Miller<davem@davemloft.net>
> >
> > (cherry picked from commit fd0e435b0fe85622f167b84432552885a4856ac8)
> > CVE-2011-1581
> > BugLink: http://bugs.launchpad.net/bugs/792312
> > Signed-off-by: Andy Whitcroft<apw@canonical.com>
> > ---
> >   drivers/net/bonding/bond_main.c |   11 +++++++++--
> >   1 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 163e0b0..e68eaef 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -4537,11 +4537,18 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb)
> >   {
> >   	/*
> >   	 * This helper function exists to help dev_pick_tx get the correct
> > -	 * destination queue.  Using a helper function skips the a call to
> > +	 * destination queue.  Using a helper function skips a call to
> >   	 * skb_tx_hash and will put the skbs in the queue we expect on their
> >   	 * way down to the bonding driver.
> >   	 */
> > -	return skb->queue_mapping;
> > +	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> > +
> > +	if (unlikely(txq>= dev->real_num_tx_queues)) {
> > +		do
> > +			txq -= dev->real_num_tx_queues;
> > +		while (txq>= dev->real_num_tx_queues);
> > +	}
> > +	return txq;
> >   }
> >
> >   static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> 
> (though more for being clean cherry pick, rather than clearness)

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 163e0b0..e68eaef 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4537,11 +4537,18 @@  static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb)
 {
 	/*
 	 * This helper function exists to help dev_pick_tx get the correct
-	 * destination queue.  Using a helper function skips the a call to
+	 * destination queue.  Using a helper function skips a call to
 	 * skb_tx_hash and will put the skbs in the queue we expect on their
 	 * way down to the bonding driver.
 	 */
-	return skb->queue_mapping;
+	u16 txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
+
+	if (unlikely(txq >= dev->real_num_tx_queues)) {
+		do
+			txq -= dev->real_num_tx_queues;
+		while (txq >= dev->real_num_tx_queues);
+	}
+	return txq;
 }
 
 static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)