[PATCH 1/2] xfrm: fix a potential use after free in xfrm4_policy.c

[r***@gmail.com]

From: Li RongQing <***@gmail.com>

pskb_may_pull() maybe change skb->data and make xprth pointer oboslete,
so recompute the xprth

Signed-off-by: Li RongQing <***@gmail.com>
---
net/ipv4/xfrm4_policy.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 6156f68..a4d8177 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -98,11 +98,14 @@ static int xfrm4_fill_dst(struct xfrm_dst *xdst, struct net_device *dev,
return 0;
}

+#define NEXT_HEAD(skb) (skb_network_header(skb) + ihl)
+
static void
_decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
{
const struct iphdr *iph = ip_hdr(skb);
- u8 *xprth = skb_network_header(skb) + iph->ihl * 4;
+ int ihl = iph->ihl * 4;
+ u8 *xprth = NEXT_HEAD(skb);
struct flowi4 *fl4 = &fl->u.ip4;
int oif = 0;

@@ -122,7 +125,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
case IPPROTO_DCCP:
if (xprth + 4 < skb->data ||
pskb_may_pull(skb, xprth + 4 - skb->data)) {
- __be16 *ports = (__be16 *)xprth;
+ __be16 *ports = (__be16 *)NEXT_HEAD(skb);

fl4->fl4_sport = ports[!!reverse];
fl4->fl4_dport = ports[!reverse];
@@ -131,7 +134,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_ICMP:
if (pskb_may_pull(skb, xprth + 2 - skb->data)) {
- u8 *icmp = xprth;
+ u8 *icmp = NEXT_HEAD(skb);

fl4->fl4_icmp_type = icmp[0];
fl4->fl4_icmp_code = icmp[1];
@@ -140,7 +143,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_ESP:
if (pskb_may_pull(skb, xprth + 4 - skb->data)) {
- __be32 *ehdr = (__be32 *)xprth;
+ __be32 *ehdr = (__be32 *)NEXT_HEAD(skb);

fl4->fl4_ipsec_spi = ehdr[0];
}
@@ -148,7 +151,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_AH:
if (pskb_may_pull(skb, xprth + 8 - skb->data)) {
- __be32 *ah_hdr = (__be32 *)xprth;
+ __be32 *ah_hdr = (__be32 *)NEXT_HEAD(skb);

fl4->fl4_ipsec_spi = ah_hdr[1];
}
@@ -156,7 +159,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_COMP:
if (pskb_may_pull(skb, xprth + 4 - skb->data)) {
- __be16 *ipcomp_hdr = (__be16 *)xprth;
+ __be16 *ipcomp_hdr = (__be16 *)NEXT_HEAD(skb);

fl4->fl4_ipsec_spi = htonl(ntohs(ipcomp_hdr[1]));
}
@@ -164,8 +167,8 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_GRE:
if (pskb_may_pull(skb, xprth + 12 - skb->data)) {
- __be16 *greflags = (__be16 *)xprth;
- __be32 *gre_hdr = (__be32 *)xprth;
+ __be16 *greflags = (__be16 *)NEXT_HEAD(skb);
+ __be32 *gre_hdr = (__be32 *)NEXT_HEAD(skb);

if (greflags[0] & GRE_KEY) {
if (greflags[0] & GRE_CSUM)

[r***@gmail.com]

From: Li RongQing <***@gmail.com>

pskb_may_pull() maybe change skb->data and make nh and exthdr pointer
oboslete, so recompute the nd and exthdr

Signed-off-by: Li RongQing <***@gmail.com>
---
net/ipv6/xfrm6_policy.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index ac49f84..115fd3b 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -170,8 +170,10 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
case IPPROTO_DCCP:
if (!onlyproto && (nh + offset + 4 < skb->data ||
pskb_may_pull(skb, nh + offset + 4 - skb->data))) {
- __be16 *ports = (__be16 *)exthdr;
+ __be16 *ports;

+ nh = skb_network_header(skb);
+ ports = (__be16*)(nh + offset);
fl6->fl6_sport = ports[!!reverse];
fl6->fl6_dport = ports[!reverse];
}
@@ -180,8 +182,10 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)

case IPPROTO_ICMPV6:
if (!onlyproto && pskb_may_pull(skb, nh + offset + 2 - skb->data)) {
- u8 *icmp = (u8 *)exthdr;
+ u8 *icmp;

+ nh = skb_network_header(skb);
+ icmp = (u8*)(nh + offset);
fl6->fl6_icmp_type = icmp[0];
fl6->fl6_icmp_code = icmp[1];
}
@@ -192,8 +196,9 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
case IPPROTO_MH:
if (!onlyproto && pskb_may_pull(skb, nh + offset + 3 - skb->data)) {
struct ip6_mh *mh;
- mh = (struct ip6_mh *)exthdr;

+ nh = skb_network_header(skb);
+ mh = (struct ip6_mh*)(nh + offset);
fl6->fl6_mh_type = mh->ip6mh_type;
}
fl6->flowi6_proto = nexthdr;

[Sergei Shtylyov]

Hello.

Post by r***@gmail.com
pskb_may_pull() maybe change skb->data and make nh and exthdr pointer
oboslete, so recompute the nd and exthdr
---
net/ipv6/xfrm6_policy.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index ac49f84..115fd3b 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -170,8 +170,10 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
if (!onlyproto && (nh + offset + 4 < skb->data ||
pskb_may_pull(skb, nh + offset + 4 - skb->data))) {
- __be16 *ports = (__be16 *)exthdr;
+ __be16 *ports;
+ nh = skb_network_header(skb);
+ ports = (__be16*)(nh + offset);

Please insert a space between '__be16' and * like it was done in the
deleted assignment.

[...]

Post by r***@gmail.com
@@ -180,8 +182,10 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
if (!onlyproto && pskb_may_pull(skb, nh + offset + 2 - skb->data)) {
- u8 *icmp = (u8 *)exthdr;
+ u8 *icmp;
+ nh = skb_network_header(skb);
+ icmp = (u8*)(nh + offset);

Likewise.

[...]

Post by r***@gmail.com
@@ -192,8 +196,9 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
if (!onlyproto && pskb_may_pull(skb, nh + offset + 3 - skb->data)) {
struct ip6_mh *mh;
- mh = (struct ip6_mh *)exthdr;
+ nh = skb_network_header(skb);
+ mh = (struct ip6_mh*)(nh + offset);

Likewise.

[...]

WBR, Sergei

[David Miller]

From: ***@gmail.com
Date: Mon, 20 Oct 2014 16:49:13 +0800

Post by r***@gmail.com
pskb_may_pull() maybe change skb->data and make xprth pointer oboslete,
so recompute the xprth

Please don't use macros that hide uses of local variables.

That is almost as bad as hiding control flow inside of
a macro.