On Sat, Nov 25, 2017 at 04:50:31AM +0900, David Miller wrote:

From: Florian Westphal fw@strlen.de Date: Fri, 24 Nov 2017 20:32:12 +0100

...

Tomas Charvat tc@excello.cz wrote:

[ CC stable, Steffen ]

...

Hi Florian and David, I'm running several servers that use XFRM ipsec. It do work well on all kernels bellow 4.14.0.

It doesnt work on 4.14.0-2. There is no any error in dmesg or in userspace when I do configure policies.

Since there is not much info about XFRM in dmesg I have no clue, where to start when I want to debug this issue.

David, please consider picking up 94802151894d482e82c324edf2c658f8e6b96508 ("Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find.")

for the 4.14.y stable queue.

I think its a pretty safe bet that this fixes the problem, it broke transport mode wildcard policy lookup.

Ok, once we have confirmation that this fixes it I also need to pair it up with Steffen's alternative fix for the bug that commit was trying to fix.

We need this revert in the 4.14.y stable tree anyway as it broke transport mode IPsec.

I thought quite a lot about the original problem that I tried to fix. It is a rather subtile thing, like almost all bugs reported from syzcaller I have seen.

In between I think our template validation is not strict enough. It is possible to configure policies with transport mode template where the selector address family does not match the templates address family. The address family can not change on a transport mode transformation, so this configuration does not make much sense but lead to problems because we use the assumption that the address family can not change on thransport mode later on.

Unfortunately the reproducer provided by syzcaller does not trigger anything on my test setup, so I don't even know if this fixes this exact problem.

Florian, could you please give the patch blelow a try?

Subject: [PATCH] xfrm: Fix stack-out-of-bounds with misconfigured transport mode policies.

On policies with a transport mode template, we pass the addresses from the flowi to xfrm_state_find(), assuming that the IP addresses (and address family) don't change during transformation.

Unfortunately our policy template validation is not strict enough. It is possible to configure policies with transport mode template where the address family of the template does not match the selectors address family. This lead to stack-out-of-bound reads because we compare arddesses of the wrong family. Fix this by refusing such a configuration, address family can not change on transport mode.

We use the assumption that, on transport mode, the first templates address family must match the address family of the policy selector. Subsequent transport mode templates must match the address family of the previous template.

Signed-off-by: Steffen Klassert steffen.klassert@secunet.com --- net/xfrm/xfrm_user.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 983b0233767b..57ad016ae675 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1419,11 +1419,14 @@ static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,

static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family) { + u16 prev_family; int i;

if (nr > XFRM_MAX_DEPTH) return -EINVAL;

+ prev_family = family; + for (i = 0; i < nr; i++) { /* We never validated the ut->family value, so many * applications simply leave it at zero. The check was @@ -1435,6 +1438,12 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family) if (!ut[i].family) ut[i].family = family;

+ if ((ut[i].mode == XFRM_MODE_TRANSPORT) && + (ut[i].family != prev_family)) + return -EINVAL; + + prev_family = ut[i].family; + switch (ut[i].family) { case AF_INET: break;

On Mon, Nov 27, 2017 at 11:39:40AM +0100, Florian Westphal wrote:

Steffen Klassert steffen.klassert@secunet.com wrote:

...

Florian, could you please give the patch blelow a try?

Doesn't help.

Thanks for testing!

...

static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family) {

• u16 prev_family; int i;

if (nr > XFRM_MAX_DEPTH) return -EINVAL;

• prev_family = family;

family is AF_INET6 (10), nr is 1.

...

for (i = 0; i < nr; i++) { /* We never validated the ut->family value, so many * applications simply leave it at zero. The check was @@ -1435,6 +1438,12 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family) if (!ut[i].family) ut[i].family = family;

• if ((ut[i].mode == XFRM_MODE_TRANSPORT) &&
  

•     (ut[i].family != prev_family))
  

• 	return -EINVAL;
  

mode is XFRM_MODE_TRANSPORT, family == prev_family (10), so no -EINVAL here.

Socket is AF_INET6. setsockopt(3, SOL_IPV6, 0x23 /* IPV6_??? */, ... sendto(3, "", 0, MSG_EOR|MSG_NOSIGNAL|0x10, {sa_family=AF_INET, sin_port=htons(20002), sin_addr=inet_addr("0.0.0.0")}, 16) = -1 EAGAIN (Resource temporarily unavailable)

and then used with AF_INET address, which causes the KASAN warning as we feed xfrm_state_find with on-stack ipv4 addresses.

This means that a policy with IPv6 selector matched an IPv4 flow, hmm. This is probably because most selector fields are wildcard. We could catch this by checking the address families of selector and flow in xfrm_sk_policy_lookup() before the policy match().

I'll try to get the reproducer to work here so that I don't need to bother you with this, thanks again!