[net] xfrm: fix stack access out of bounds with CONFIG_XFRM_SUB_POLICY

Submitted by Sabrina Dubroca on April 21, 2017, 10:14 a.m.

Details

Message ID 17208ec89e09ed7223ffd2302abfa3475255c384.1492769294.git.sd@queasysnail.net
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Sabrina Dubroca April 21, 2017, 10:14 a.m.
When CONFIG_XFRM_SUB_POLICY=y, xfrm_dst stores a copy of the flowi for
that dst. Unfortunately, the code that allocates and fills this copy
doesn't care about what type of flowi (flowi, flowi4, flowi6) gets
passed. In multiple code paths (from raw_sendmsg, from TCP when
replying to a FIN, in vxlan, geneve, and gre), the flowi that gets
passed to xfrm is actually an on-stack flowi4, so we end up reading
memory on the stack past the end of the flowi4 struct.

Since xfrm_dst->origin isn't used anywhere, just get rid of it.
xfrm_dst->partner isn't used either, so get rid of that too.

Fixes: ca116922afa8 ("xfrm: Eliminate "fl" and "pol" args to xfrm_bundle_ok().")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 include/net/xfrm.h     | 10 ----------
 net/xfrm/xfrm_policy.c | 47 -----------------------------------------------
 2 files changed, 57 deletions(-)

Comments

Herbert Xu April 21, 2017, 11:06 a.m.
On Fri, Apr 21, 2017 at 12:14:51PM +0200, Sabrina Dubroca wrote:
> When CONFIG_XFRM_SUB_POLICY=y, xfrm_dst stores a copy of the flowi for
> that dst. Unfortunately, the code that allocates and fills this copy
> doesn't care about what type of flowi (flowi, flowi4, flowi6) gets
> passed. In multiple code paths (from raw_sendmsg, from TCP when
> replying to a FIN, in vxlan, geneve, and gre), the flowi that gets
> passed to xfrm is actually an on-stack flowi4, so we end up reading
> memory on the stack past the end of the flowi4 struct.
> 
> Since xfrm_dst->origin isn't used anywhere, just get rid of it.
> xfrm_dst->partner isn't used either, so get rid of that too.
> 
> Fixes: ca116922afa8 ("xfrm: Eliminate "fl" and "pol" args to xfrm_bundle_ok().")

The commit you refer to here doesn't seem to have caused this bug.

Thanks,
Sabrina Dubroca April 21, 2017, 12:05 p.m.
2017-04-21, 19:06:44 +0800, Herbert Xu wrote:
> On Fri, Apr 21, 2017 at 12:14:51PM +0200, Sabrina Dubroca wrote:
> > When CONFIG_XFRM_SUB_POLICY=y, xfrm_dst stores a copy of the flowi for
> > that dst. Unfortunately, the code that allocates and fills this copy
> > doesn't care about what type of flowi (flowi, flowi4, flowi6) gets
> > passed. In multiple code paths (from raw_sendmsg, from TCP when
> > replying to a FIN, in vxlan, geneve, and gre), the flowi that gets
> > passed to xfrm is actually an on-stack flowi4, so we end up reading
> > memory on the stack past the end of the flowi4 struct.
> > 
> > Since xfrm_dst->origin isn't used anywhere, just get rid of it.
> > xfrm_dst->partner isn't used either, so get rid of that too.
> > 
> > Fixes: ca116922afa8 ("xfrm: Eliminate "fl" and "pol" args to xfrm_bundle_ok().")
> 
> The commit you refer to here doesn't seem to have caused this bug.

You're right. I had a note about that but it got lost.  The bug
(ignoring what flavor of flowi is passed) is older than that (from the
introduction of subpolicies, I suspect, but I would have to dig more
into the history), but this commit removed the last uses of
origin/partner.

Looking into raw_sendmsg(), this code may have been safe before
9d6ec938019c ("ipv4: Use flowi4 in public route lookup interfaces."),
since full flowi were used.

If we want a fix for kernels that don't have ca116922afa8, we would
probably need to take the size of the flowi* struct depending on the
address family passed to xfrm_resolve_and_create_bundle().


I'm not sure how to proceed here, any advice?


Thanks,
Herbert Xu April 24, 2017, 7:59 a.m.
On Fri, Apr 21, 2017 at 02:05:31PM +0200, Sabrina Dubroca wrote:
> 
> You're right. I had a note about that but it got lost.  The bug
> (ignoring what flavor of flowi is passed) is older than that (from the
> introduction of subpolicies, I suspect, but I would have to dig more
> into the history), but this commit removed the last uses of
> origin/partner.
> 
> Looking into raw_sendmsg(), this code may have been safe before
> 9d6ec938019c ("ipv4: Use flowi4 in public route lookup interfaces."),
> since full flowi were used.
> 
> If we want a fix for kernels that don't have ca116922afa8, we would
> probably need to take the size of the flowi* struct depending on the
> address family passed to xfrm_resolve_and_create_bundle().
> 
> 
> I'm not sure how to proceed here, any advice?

I think the Fixes header should simply refer to the commit that
introduced the bug.  You're instead using it as a hint for how
to backport the patch.  That is beyond the scope of the Fixes
header.

So if the bug started with 9d6ec938019c then just use that in
your Fixes header.  If you want to help the backporter with more
information then you can put ca116922afa8 in the body of the commit
description.

Cheers,

Patch hide | download patch | download mbox

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 14d82bf16692..85bc8e7ade2e 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -945,10 +945,6 @@  struct xfrm_dst {
 	struct flow_cache_object flo;
 	struct xfrm_policy *pols[XFRM_POLICY_TYPE_MAX];
 	int num_pols, num_xfrms;
-#ifdef CONFIG_XFRM_SUB_POLICY
-	struct flowi *origin;
-	struct xfrm_selector *partner;
-#endif
 	u32 xfrm_genid;
 	u32 policy_genid;
 	u32 route_mtu_cached;
@@ -964,12 +960,6 @@  static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
 	dst_release(xdst->route);
 	if (likely(xdst->u.dst.xfrm))
 		xfrm_state_put(xdst->u.dst.xfrm);
-#ifdef CONFIG_XFRM_SUB_POLICY
-	kfree(xdst->origin);
-	xdst->origin = NULL;
-	kfree(xdst->partner);
-	xdst->partner = NULL;
-#endif
 }
 #endif
 
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 236cbbc0ab9c..56fba8f073f5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1793,43 +1793,6 @@  static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	goto out;
 }
 
-#ifdef CONFIG_XFRM_SUB_POLICY
-static int xfrm_dst_alloc_copy(void **target, const void *src, int size)
-{
-	if (!*target) {
-		*target = kmalloc(size, GFP_ATOMIC);
-		if (!*target)
-			return -ENOMEM;
-	}
-
-	memcpy(*target, src, size);
-	return 0;
-}
-#endif
-
-static int xfrm_dst_update_parent(struct dst_entry *dst,
-				  const struct xfrm_selector *sel)
-{
-#ifdef CONFIG_XFRM_SUB_POLICY
-	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
-	return xfrm_dst_alloc_copy((void **)&(xdst->partner),
-				   sel, sizeof(*sel));
-#else
-	return 0;
-#endif
-}
-
-static int xfrm_dst_update_origin(struct dst_entry *dst,
-				  const struct flowi *fl)
-{
-#ifdef CONFIG_XFRM_SUB_POLICY
-	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
-	return xfrm_dst_alloc_copy((void **)&(xdst->origin), fl, sizeof(*fl));
-#else
-	return 0;
-#endif
-}
-
 static int xfrm_expand_policies(const struct flowi *fl, u16 family,
 				struct xfrm_policy **pols,
 				int *num_pols, int *num_xfrms)
@@ -1901,16 +1864,6 @@  xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols,
 
 	xdst = (struct xfrm_dst *)dst;
 	xdst->num_xfrms = err;
-	if (num_pols > 1)
-		err = xfrm_dst_update_parent(dst, &pols[1]->selector);
-	else
-		err = xfrm_dst_update_origin(dst, fl);
-	if (unlikely(err)) {
-		dst_free(dst);
-		XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTBUNDLECHECKERROR);
-		return ERR_PTR(err);
-	}
-
 	xdst->num_pols = num_pols;
 	memcpy(xdst->pols, pols, sizeof(struct xfrm_policy *) * num_pols);
 	xdst->policy_genid = atomic_read(&pols[0]->genid);