[PATCH ipsec-next] xfrm: don't pull esp/auth header in xfrm_parse_spi

[PATCH ipsec-next] xfrm: don't pull esp/auth header in xfrm_parse_spi

[Florian Westphal]

syzbot reported an issue where pointer to ip header content was not
reloaded after xfrm_parse_spi().

Its not intuitive that this function changes skb->head, so switch to
skb_pointer_header.

Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 syzkaller reproducer did not spew a warning even after 15 minutes
 whereas before it would fire reliably after at most 30 seconds.

 include/net/xfrm.h    |  3 +--
 net/xfrm/xfrm_input.c | 39 +++++++++++++++++++++++----------------
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 3cb618bbcfa5..07ffa9688a11 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1579,7 +1579,6 @@ int xfrm4_rcv_encap(struct sk_buff *skb, int nexthdr, __be32 spi,
 		    int encap_type);
 int xfrm4_transport_finish(struct sk_buff *skb, int async);
 int xfrm4_rcv(struct sk_buff *skb);
-int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq);
 
 static inline int xfrm4_rcv_spi(struct sk_buff *skb, int nexthdr, __be32 spi)
 {
@@ -1694,7 +1693,7 @@ int km_report(struct net *net, u8 proto, struct xfrm_selector *sel,
 	      xfrm_address_t *addr);
 
 void xfrm_input_init(void);
-int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq);
+int xfrm_parse_spi(const struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq);
 
 void xfrm_probe_algs(void);
 int xfrm_count_pfkey_auth_supported(void);
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 8ac9d32fb79d..92fbb784ea8c 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -141,37 +141,44 @@ EXPORT_SYMBOL(secpath_set);
 
 /* Fetch spi and seq from ipsec header */
 
-int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq)
+int xfrm_parse_spi(const struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq)
 {
-	int offset, offset_seq;
-	int hlen;
+	int th = skb_transport_offset(skb);
+	union {
+		struct ip_auth_hdr _auth;
+		struct ip_comp_hdr _comp;
+		struct ip_esp_hdr _esp;
+	} _tmp;
+	const struct ip_auth_hdr *auth;
+	const struct ip_comp_hdr *comp;
+	const struct ip_esp_hdr *esp;
 
 	switch (nexthdr) {
 	case IPPROTO_AH:
-		hlen = sizeof(struct ip_auth_hdr);
-		offset = offsetof(struct ip_auth_hdr, spi);
-		offset_seq = offsetof(struct ip_auth_hdr, seq_no);
+		auth = skb_header_pointer(skb, th, sizeof(*auth), &_tmp._auth);
+		if (!auth)
+			return -EINVAL;
+		*spi = auth->spi;
+		*seq = auth->seq_no;
 		break;
 	case IPPROTO_ESP:
-		hlen = sizeof(struct ip_esp_hdr);
-		offset = offsetof(struct ip_esp_hdr, spi);
-		offset_seq = offsetof(struct ip_esp_hdr, seq_no);
+		esp = skb_header_pointer(skb, th, sizeof(*esp), &_tmp._esp);
+		if (!esp)
+			return -EINVAL;
+		*spi = esp->spi;
+		*seq = esp->seq_no;
 		break;
 	case IPPROTO_COMP:
-		if (!pskb_may_pull(skb, sizeof(struct ip_comp_hdr)))
+		comp = skb_header_pointer(skb, th, sizeof(*comp), &_tmp._comp);
+		if (!comp)
 			return -EINVAL;
-		*spi = htonl(ntohs(*(__be16 *)(skb_transport_header(skb) + 2)));
+		*spi = htonl(ntohs(comp->cpi));
 		*seq = 0;
 		return 0;
 	default:
 		return 1;
 	}
 
-	if (!pskb_may_pull(skb, hlen))
-		return -EINVAL;
-
-	*spi = *(__be32 *)(skb_transport_header(skb) + offset);
-	*seq = *(__be32 *)(skb_transport_header(skb) + offset_seq);
 	return 0;
 }
 EXPORT_SYMBOL(xfrm_parse_spi);
-- 
2.13.6

Re: [PATCH ipsec-next] xfrm: don't pull esp/auth header in xfrm_parse_spi

[Steffen Klassert]

On Thu, Nov 02, 2017 at 01:54:10PM +0100, Florian Westphal wrote:
> syzbot reported an issue where pointer to ip header content was not
> reloaded after xfrm_parse_spi().
> 
> Its not intuitive that this function changes skb->head, so switch to
> skb_pointer_header.

I have to admit that this is not intuitive, but we pull
these headers later anyway. So using skb_header_pointer()
here might add another memcpy that could be avoided.

I think I'll stay with the fix you submitted for the ipsec
tree already.