FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 08-23-2012, 04:27 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

The following changes since commit cf52d980c29eade382d95198b7362787038b9ef5:

eCryptfs: Initialize empty lower files when opening them (2012-08-23
12:05:04 -0400)

are available in the git repository at:

git://kernel.ubuntu.com/rtg/ubuntu-lucid.git CVE-2012-3412

for you to fetch changes up to 67438dd33e0bb1e2f120f4f6672b5c43d30ce5fd:

sfc: Fix maximum number of TSO segments and minimum TX queue size
(2012-08-23 10:07:17 -0600)

----------------------------------------------------------------
Ben Hutchings (4):
net: Allow driver to limit number of GSO segments per skb
tcp: Apply device TSO segment limit earlier
sfc: Replace some literal constants with EFX_PAGE_SIZE/EFX_BUF_SIZE
sfc: Fix maximum number of TSO segments and minimum TX queue size

Neal Cardwell (1):
tcp: do not scale TSO segment size with reordering degree

drivers/net/sfc/efx.c | 8 ++++++++
drivers/net/sfc/efx.h | 19 +++++++++++++++++--
drivers/net/sfc/nic.c | 5 +----
drivers/net/sfc/nic.h | 5 +++++
drivers/net/sfc/tx.c | 21 ++++++++++++++++++++-
include/linux/netdevice.h | 2 ++
include/net/sock.h | 2 ++
include/net/tcp.h | 8 ++++++++
net/core/dev.c | 4 ++++
net/core/sock.c | 1 +
net/ipv4/tcp.c | 4 +++-
net/ipv4/tcp_cong.c | 7 ++++---
net/ipv4/tcp_output.c | 27 +++++++++++++++------------
13 files changed, 90 insertions(+), 23 deletions(-)
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-23-2012, 07:24 PM
Herton Ronaldo Krzesinski
 
Default Lucid CVE-2012-3412

* patch "net: Allow driver to limit number of GSO segments per skb"

I think the backport is wrong. On Lucid there is no netif_skb_features,
but the check shouldn't go inside dev_can_checksum. As far as I
understand the code changes, the clearing GSO flags in dev->features
should happen right before netif_needs_gso, and on Lucid netif_needs_gso
is called from two places instead of one (also on xen-netfront), so may
be for Lucid the clearing of the feature flags could be done inside
netif_needs_gso.

* patch "tcp: Apply device TSO segment limit earlier"

I didn't paid attention before, but I noticed now that on backports of
this patch, on tcp_is_cwnd_limited, you are returning a bool on the
backport while the function was still of int type, shouldn't cause
problems but looks ugly. It happened on all backports of this patch
since Precise, so if fixing this here on Lucid, probably should be fixed
on the others as well (Natty->Precise).

Also other thing that was not part of the change in the commit upstream,
and is harmless anyway, is that on tcp_mss_split_point, you change
"struct tcp_sock *tp" declaration to const. I don't think is a problem
anyway, but isn't related to the change. It wasn't const in Oneiric and
earlier.

* patch "sfc: Fix maximum number of TSO segments and minimum TX queue size"

On this last one, I saw you define new macros,
EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the
backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem
needed as well on the backport.

--
[]'s
Herton

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-23-2012, 08:14 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

On 08/23/2012 01:24 PM, Herton Ronaldo Krzesinski wrote:
> * patch "net: Allow driver to limit number of GSO segments per skb"
>
> I think the backport is wrong. On Lucid there is no netif_skb_features,
> but the check shouldn't go inside dev_can_checksum. As far as I
> understand the code changes, the clearing GSO flags in dev->features
> should happen right before netif_needs_gso, and on Lucid netif_needs_gso
> is called from two places instead of one (also on xen-netfront), so may
> be for Lucid the clearing of the feature flags could be done inside
> netif_needs_gso.
>

You are correct. I didn't look at that one close enough.

> * patch "tcp: Apply device TSO segment limit earlier"
>
> I didn't paid attention before, but I noticed now that on backports of
> this patch, on tcp_is_cwnd_limited, you are returning a bool on the
> backport while the function was still of int type, shouldn't cause
> problems but looks ugly. It happened on all backports of this patch
> since Precise, so if fixing this here on Lucid, probably should be fixed
> on the others as well (Natty->Precise).
>

The code is functionally equivalent, and is true to the upstream patch
so I'm inclined to leave it be.

> Also other thing that was not part of the change in the commit upstream,
> and is harmless anyway, is that on tcp_mss_split_point, you change
> "struct tcp_sock *tp" declaration to const. I don't think is a problem
> anyway, but isn't related to the change. It wasn't const in Oneiric and
> earlier.
>

The upstream prototype is 'const struct tcp_sock *tp', but I think its
benign.

> * patch "sfc: Fix maximum number of TSO segments and minimum TX queue size"
>
> On this last one, I saw you define new macros,
> EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the
> backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem
> needed as well on the backport.
>

EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE came in with the patch backport and
are indeed unused (and benign).

I changed both EFX_TXQ_SIZE and EFX_RXQ_SIZE to use
EFX_DEFAULT_DMAQ_SIZE so that neither is a magic number.

I've repushed with an update to "net: Allow driver to limit number of
GSO segments per skb"

rtg
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-24-2012, 12:46 AM
Herton Ronaldo Krzesinski
 
Default Lucid CVE-2012-3412

On Thu, Aug 23, 2012 at 02:14:23PM -0600, Tim Gardner wrote:
[...]
> I've repushed with an update to "net: Allow driver to limit number of
> GSO segments per skb"

This one still doesn't look ok. You clear dev->features, but the
behaviour of the upstream patch is different: they copy dev->features to
a local variable, and clear that, passing to netif_needs_gso, otherwise
after you clear first time, dev->features will have always the flags
cleared, and the checks are per skb, shouldn't be cleared globally in
dev->features. Fixing this, Ack from me for the patch series, the other
things I just wanted to point out but should be benign or style issues.

>
> rtg
> --
> Tim Gardner tim.gardner@canonical.com
>

--
[]'s
Herton

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-24-2012, 01:58 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

This is a multi-part message in MIME format.On 08/23/2012 06:46 PM, Herton Ronaldo Krzesinski wrote:
> On Thu, Aug 23, 2012 at 02:14:23PM -0600, Tim Gardner wrote:
> [...]
>> I've repushed with an update to "net: Allow driver to limit number of
>> GSO segments per skb"
>
> This one still doesn't look ok. You clear dev->features, but the
> behaviour of the upstream patch is different: they copy dev->features to
> a local variable, and clear that, passing to netif_needs_gso, otherwise
> after you clear first time, dev->features will have always the flags
> cleared, and the checks are per skb, shouldn't be cleared globally in
> dev->features. Fixing this, Ack from me for the patch series, the other
> things I just wanted to point out but should be benign or style issues.
>

Doh, how about this? Also repushed.

rtg
--
Tim Gardner tim.gardner@canonical.com
 
Old 08-24-2012, 03:05 PM
Herton Ronaldo Krzesinski
 
Default Lucid CVE-2012-3412

On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
> {
> + if (skb_is_gso(skb) &&
> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
> + return 0;

Shouldn't be return 1 here? If the condition is true, we would clear the
flags from features. If flags are cleared, when calling skb_gso_ok:

net_gso_ok would always return 0
skb_gso_ok would always return 0
netif_needs_gso returns 1 because it does !skb_gso_ok

Unless I'm missing something here. Anyway, hard to read these functions...
I think just copying/clearing the flags and passing through skb_gso_ok
would be better.

--
[]'s
Herton

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-24-2012, 03:11 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
> On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
>> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>> {
>> + if (skb_is_gso(skb) &&
>> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
>> + return 0;
>
> Shouldn't be return 1 here? If the condition is true, we would clear the
> flags from features. If flags are cleared, when calling skb_gso_ok:
>
> net_gso_ok would always return 0
> skb_gso_ok would always return 0
> netif_needs_gso returns 1 because it does !skb_gso_ok
>
> Unless I'm missing something here. Anyway, hard to read these functions...
> I think just copying/clearing the flags and passing through skb_gso_ok
> would be better.
>

I guess I'm confused about when the flag is set. I was assuming if GSO
was set, then the driver handled 'generic segmentation offload'. Aren't
we checking for error conditions, e.g., if there are more segments then
the driver can handle, then _don't_ do GSO ?

rtg
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-24-2012, 07:38 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

On 08/24/2012 09:11 AM, Tim Gardner wrote:
> On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
>> On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
>>> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>>> {
>>> + if (skb_is_gso(skb) &&
>>> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
>>> + return 0;
>>
>> Shouldn't be return 1 here? If the condition is true, we would clear the
>> flags from features. If flags are cleared, when calling skb_gso_ok:
>>
>> net_gso_ok would always return 0
>> skb_gso_ok would always return 0
>> netif_needs_gso returns 1 because it does !skb_gso_ok
>>
>> Unless I'm missing something here. Anyway, hard to read these functions...
>> I think just copying/clearing the flags and passing through skb_gso_ok
>> would be better.
>>
>
> I guess I'm confused about when the flag is set. I was assuming if GSO
> was set, then the driver handled 'generic segmentation offload'. Aren't
> we checking for error conditions, e.g., if there are more segments then
> the driver can handle, then _don't_ do GSO ?
>
> rtg
>

After some online discussion with Herton I think we've agreed to bag
this mess. This is not a broad vulnerability, and the patch is proving
to be more effort then its worth.

rtg
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-24-2012, 07:50 PM
Herton Ronaldo Krzesinski
 
Default Lucid CVE-2012-3412

On Fri, Aug 24, 2012 at 09:11:22AM -0600, Tim Gardner wrote:
> On 08/24/2012 09:05 AM, Herton Ronaldo Krzesinski wrote:
> > On Fri, Aug 24, 2012 at 07:58:34AM -0600, Tim Gardner wrote:
> >> static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
> >> {
> >> + if (skb_is_gso(skb) &&
> >> + skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
> >> + return 0;
> >
> > Shouldn't be return 1 here? If the condition is true, we would clear the
> > flags from features. If flags are cleared, when calling skb_gso_ok:
> >
> > net_gso_ok would always return 0
> > skb_gso_ok would always return 0
> > netif_needs_gso returns 1 because it does !skb_gso_ok
> >
> > Unless I'm missing something here. Anyway, hard to read these functions...
> > I think just copying/clearing the flags and passing through skb_gso_ok
> > would be better.
> >
>
> I guess I'm confused about when the flag is set. I was assuming if GSO
> was set, then the driver handled 'generic segmentation offload'. Aren't
> we checking for error conditions, e.g., if there are more segments then
> the driver can handle, then _don't_ do GSO ?

Ok, forget last thing I said on IRC about sfc, it is the only one
limiting the gso_max_segs, the others which supports GSO continues to do
so and use the default maximum of GSO_MAX_SEGS as defined on the patch.
Looks like only sfc indeed needs this and is affected, as it's my
understanding.

About the patch, if still pursuing this, I expect the following diff
should work for Lucid. I think I handled what's needed. I noticed
skb_gso_ok was used at dev_gso_segment()->skb_gso_segment(), so we
can't patch netif_needs_gso directly, and if we skb_gso_ok is used with
protocol code so we couldn't clear flags there (not sure if there would
be a side effect doing so).

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 1a11d95..422001f 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -486,7 +486,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)

if (unlikely(!netif_carrier_ok(dev) ||
(frags > 1 && !xennet_can_sg(dev)) ||
- netif_needs_gso(dev, skb))) {
+ netif_needs_gso(skb, netif_skb_features(dev, skb)))) {
spin_unlock_irq(&np->tx_lock);
goto drop;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ea6187c..9216ff6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -907,6 +907,8 @@ struct net_device
/* for setting kernel sock attribute on TCP connection setup */
#define GSO_MAX_SIZE 65536
unsigned int gso_max_size;
+#define GSO_MAX_SEGS 65535
+ u16 gso_max_segs;

#ifdef CONFIG_DCB
/* Data Center Bridging netlink ops */
@@ -1933,10 +1935,10 @@ static inline int skb_gso_ok(struct sk_buff *skb, int features)
(!skb_has_frags(skb) || (features & NETIF_F_FRAGLIST));
}

-static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
+static inline int netif_needs_gso(struct sk_buff *skb, int features)
{
return skb_is_gso(skb) &&
- (!skb_gso_ok(skb, dev->features) ||
+ (!skb_gso_ok(skb, features) ||
unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
}

diff --git a/net/core/dev.c b/net/core/dev.c
index f32f98a..5753bfe 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1516,6 +1516,17 @@ static bool dev_can_checksum(struct net_device *dev, struct sk_buff *skb)
return false;
}

+int netif_skb_features(struct net_device *dev, struct sk_buff *skb)
+{
+ int features = dev->features;
+
+ if (skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs)
+ features &= ~NETIF_F_GSO_MASK;
+
+ return features;
+}
+EXPORT_SYMBOL(netif_skb_features);
+
/*
* Invalidate hardware checksum when packet is to be mangled, and
* complete checksum manually on outgoing path.
@@ -1682,13 +1693,12 @@ static void dev_gso_skb_destructor(struct sk_buff *skb)
* This function segments the given skb and stores the list of segments
* in skb->next.
*/
-static int dev_gso_segment(struct sk_buff *skb)
+static int dev_gso_segment(struct sk_buff *skb, int features)
{
struct net_device *dev = skb->dev;
struct sk_buff *segs;
- int features = dev->features & ~(illegal_highdma(dev, skb) ?
- NETIF_F_SG : 0);

+ features &= ~(illegal_highdma(dev, skb) ? NETIF_F_SG : 0);
segs = skb_gso_segment(skb, features);

/* Verifying header integrity only. */
@@ -1712,11 +1722,15 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
int rc;

if (likely(!skb->next)) {
+ int features;
+
if (!list_empty(&ptype_all))
dev_queue_xmit_nit(skb, dev);

- if (netif_needs_gso(dev, skb)) {
- if (unlikely(dev_gso_segment(skb)))
+ features = netif_skb_features(dev, skb);
+
+ if (netif_needs_gso(skb, features)) {
+ if (unlikely(dev_gso_segment(skb, features)))
goto out_kfree_skb;
if (skb->next)
goto gso;
@@ -1887,7 +1901,7 @@ int dev_queue_xmit(struct sk_buff *skb)
int rc = -ENOMEM;

/* GSO will handle the following emulations directly. */
- if (netif_needs_gso(dev, skb))
+ if (netif_needs_gso(skb, netif_skb_features(dev, skb)))
goto gso;

if (skb_has_frags(skb) &&
@@ -5195,6 +5209,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
dev->real_num_tx_queues = queue_count;

dev->gso_max_size = GSO_MAX_SIZE;
+ dev->gso_max_segs = GSO_MAX_SEGS;

netdev_init_queues(dev);

>
> rtg
> --
> Tim Gardner tim.gardner@canonical.com
>

--
[]'s
Herton

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 08-27-2012, 04:55 PM
Tim Gardner
 
Default Lucid CVE-2012-3412

This is a multi-part message in MIME format.OK, fixed some compile issues with his patch and re-pushed.

git://kernel.ubuntu.com/rtg/ubuntu-lucid.git CVE-2012-3412

rtg
--
Tim Gardner tim.gardner@canonical.com
 

Thread Tools




All times are GMT. The time now is 05:41 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org