Re: flow cache removed = xfrm doesnt work

[Florian Westphal <fw@strlen.de>]

Steffen Klassert <steffen.klassert@secunet.com> wrote:
> 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?

Doesn't help.

>  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.