Re: [PATCH net] net: add recursion limit to GRO

[Hannes Frederic Sowa]

Hi,

On Mon, Oct 10, 2016, at 16:03, Eric Dumazet wrote:
> On Mon, 2016-10-10 at 15:43 +0200, Sabrina Dubroca wrote:
> > Currently, GRO can do unlimited recursion through the gro_receive
> > handlers.  This was fixed for tunneling protocols by limiting tunnel GRO
> > to one level with encap_mark, but both VLAN and TEB still have this
> > problem.  Thus, the kernel is vulnerable to a stack overflow, if we
> > receive a packet composed entirely of VLAN headers.
> > 
> > This patch adds a recursion counter to the GRO layer to prevent stack
> > overflow.  When a gro_receive function hits the recursion limit, GRO is
> > aborted for this skb and it is processed normally.
> > 
> > Thanks to Vladimír Beneš  for the initial bug report.
>
> [...]
>
> Have you considered using a per cpu counter ?
> 
> It might be cheaper than using a 4-bit field in skb.
> 
> Really this counter does not need to be stored in skb. GRO already uses
> way too much space in skb->cb[]

The idea was to use some padding space and not bother with another
static per cpu allocation as long as there is space in the cb, which is
certainly more expensive in terms of memory consumption.

We can add a comment to make future users in gro_cb aware that this can
easily be moved to per cpu allocation if necessary in future?

Bye,
Hannes

Re: [PATCH net] net: add recursion limit to GRO

[Sabrina Dubroca]

Hi Eric,

2016-10-10, 07:03:56 -0700, Eric Dumazet wrote:
> On Mon, 2016-10-10 at 15:43 +0200, Sabrina Dubroca wrote:
> > Currently, GRO can do unlimited recursion through the gro_receive
> > handlers.  This was fixed for tunneling protocols by limiting tunnel GRO
> > to one level with encap_mark, but both VLAN and TEB still have this
> > problem.  Thus, the kernel is vulnerable to a stack overflow, if we
> > receive a packet composed entirely of VLAN headers.
> > 
> > This patch adds a recursion counter to the GRO layer to prevent stack
> > overflow.  When a gro_receive function hits the recursion limit, GRO is
> > aborted for this skb and it is processed normally.
> > 
> > Thanks to Vladimír Beneš  for the initial bug report.
> 
> 
> Hi Sabrina
> 
> Have you considered using a per cpu counter ?
>
> It might be cheaper than using a 4-bit field in skb.

I thought about it, but this looked a bit simpler. I can try some
benchmarking tomorrow.


> Really this counter does not need to be stored in skb. GRO already uses
> way too much space in skb->cb[]

For net-next I'm working on turning GRO into a loop, which would
eliminate these few bits.


> Also please add appropriate unlikely() clauses, since most GRO traffic
> is not trying to kill hosts ;)

Right. I'll send a v2 with this later.


Thanks,

-- 
Sabrina

Re: [PATCH net] net: add recursion limit to GRO

[Eric Dumazet]

On Mon, 2016-10-10 at 15:43 +0200, Sabrina Dubroca wrote:
> Currently, GRO can do unlimited recursion through the gro_receive
> handlers.  This was fixed for tunneling protocols by limiting tunnel GRO
> to one level with encap_mark, but both VLAN and TEB still have this
> problem.  Thus, the kernel is vulnerable to a stack overflow, if we
> receive a packet composed entirely of VLAN headers.
> 
> This patch adds a recursion counter to the GRO layer to prevent stack
> overflow.  When a gro_receive function hits the recursion limit, GRO is
> aborted for this skb and it is processed normally.
> 
> Thanks to Vladimír Beneš  for the initial bug report.


Hi Sabrina

Have you considered using a per cpu counter ?

It might be cheaper than using a 4-bit field in skb.

Really this counter does not need to be stored in skb. GRO already uses
way too much space in skb->cb[]

Also please add appropriate unlikely() clauses, since most GRO traffic
is not trying to kill hosts ;)

Thanks.

[PATCH net] net: add recursion limit to GRO

[Sabrina Dubroca]

Currently, GRO can do unlimited recursion through the gro_receive
handlers.  This was fixed for tunneling protocols by limiting tunnel GRO
to one level with encap_mark, but both VLAN and TEB still have this
problem.  Thus, the kernel is vulnerable to a stack overflow, if we
receive a packet composed entirely of VLAN headers.

This patch adds a recursion counter to the GRO layer to prevent stack
overflow.  When a gro_receive function hits the recursion limit, GRO is
aborted for this skb and it is processed normally.

Thanks to Vladimír Beneš  for the initial bug report.

Fixes: CVE-2016-7039
Fixes: 9b174d88c257 ("net: Add Transparent Ethernet Bridging GRO support.")
Fixes: 66e5133f19e9 ("vlan: Add GRO support for non hardware accelerated vlan")
Signed-off-by: Sabrina Dubroca 
Reviewed-by: Jiri Benc 
Acked-by: Hannes Frederic Sowa 
---
 drivers/net/geneve.c  |  2 +-
 drivers/net/vxlan.c   |  2 +-
 include/linux/netdevice.h | 24 +++-
 net/8021q/vlan.c  |  2 +-
 net/core/dev.c|  1 +
 net/ethernet/eth.c|  2 +-
 net/ipv4/af_inet.c|  2 +-
 net/ipv4/fou.c|  4 ++--
 net/ipv4/gre_offload.c|  2 +-
 net/ipv4/udp_offload.c|  8 +++-
 net/ipv6/ip6_offload.c|  2 +-
 11 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 3c20e87bb761..16af1ce99233 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -453,7 +453,7 @@ static struct sk_buff **geneve_gro_receive(struct sock *sk,
 
skb_gro_pull(skb, gh_len);
skb_gro_postpull_rcsum(skb, gh, gh_len);
-   pp = ptype->callbacks.gro_receive(head, skb);
+   pp = call_gro_receive(ptype->callbacks.gro_receive, head, skb);
flush = 0;
 
 out_unlock:
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index e7d16687538b..c1639a3e95a4 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -583,7 +583,7 @@ static struct sk_buff **vxlan_gro_receive(struct sock *sk,
}
}
 
-   pp = eth_gro_receive(head, skb);
+   pp = call_gro_receive(eth_gro_receive, head, skb);
flush = 0;
 
 out:
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 136ae6bbe81e..11a7218c3661 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2169,7 +2169,10 @@ struct napi_gro_cb {
/* Used to determine if flush_id can be ignored */
u8  is_atomic:1;
 
-   /* 5 bit hole */
+   /* Number of gro_receive callbacks this packet already went through */
+   u8 recursion_counter:4;
+
+   /* 1 bit hole */
 
/* used to support CHECKSUM_COMPLETE for tunneling protocols */
__wsum  csum;
@@ -2180,6 +2183,25 @@ struct napi_gro_cb {
 
 #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb)
 
+#define GRO_RECURSION_LIMIT 15
+static inline int gro_recursion_inc_test(struct sk_buff *skb)
+{
+   return ++NAPI_GRO_CB(skb)->recursion_counter == GRO_RECURSION_LIMIT;
+}
+
+typedef struct sk_buff **(*gro_receive_t)(struct sk_buff **, struct sk_buff *);
+static inline struct sk_buff **call_gro_receive(gro_receive_t cb,
+   struct sk_buff **head,
+   struct sk_buff *skb)
+{
+   if (gro_recursion_inc_test(skb)) {
+   NAPI_GRO_CB(skb)->flush |= 1;
+   return NULL;
+   }
+
+   return cb(head, skb);
+}
+
 struct packet_type {
__be16  type;   /* This is really htons(ether_type). */
struct net_device   *dev;   /* NULL is wildcarded here   */
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 8de138d3306b..f2531ad66b68 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -664,7 +664,7 @@ static struct sk_buff **vlan_gro_receive(struct sk_buff 
**head,
 
skb_gro_pull(skb, sizeof(*vhdr));
skb_gro_postpull_rcsum(skb, vhdr, sizeof(*vhdr));
-   pp = ptype->callbacks.gro_receive(head, skb);
+   pp = call_gro_receive(ptype->callbacks.gro_receive, head, skb);
 
 out_unlock:
rcu_read_unlock();
diff --git a/net/core/dev.c b/net/core/dev.c
index f1fe26f66458..f7cd4b8b035d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4511,6 +4511,7 @@ static enum gro_result dev_gro_receive(struct napi_struct 
*napi, struct sk_buff
NAPI_GRO_CB(skb)->flush = 0;
NAPI_GRO_CB(skb)->free = 0;
NAPI_GRO_CB(skb)->encap_mark = 0;
+   NAPI_GRO_CB(skb)->recursion_counter = 0;
NAPI_GRO_CB(skb)->is_fou = 0;
NAPI_GRO_CB(skb)->is_atomic = 1;
NAPI_GRO_CB(skb)->gro_remcsum_start = 0;
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 66dff5e3d772..02acfff36028 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -439,7 +439,7 @@ struct sk_buff **eth_gro_receive(struct sk_buff **head,
 
skb_gro_pull(skb, sizeof(*eh));