pull request (net): ipsec 2017-11-16

pull request (net): ipsec 2017-11-16

[Steffen Klassert]

1) Copy policy family in clone_policy, otherwise this can
   trigger a BUG_ON in af_key. From Herbert Xu.

2) Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."
   This added a regression with transport mode when no addresses
   are configured on the policy template.

Both patches are stable candidates.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit b39545684a90ef3374abc0969d64c7bc540d128d:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2017-11-11 09:10:39 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master

for you to fetch changes up to 94802151894d482e82c324edf2c658f8e6b96508:

  Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find." (2017-11-15 06:42:28 +0100)

----------------------------------------------------------------
Herbert Xu (1):
      xfrm: Copy policy family in clone_policy

Steffen Klassert (1):
      Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

 net/xfrm/xfrm_policy.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

[PATCH 1/2] xfrm: Copy policy family in clone_policy

[Steffen Klassert]

From: Herbert Xu <herbert@gondor.apana.org.au>

The syzbot found an ancient bug in the IPsec code.  When we cloned
a socket policy (for example, for a child TCP socket derived from a
listening socket), we did not copy the family field.  This results
in a live policy with a zero family field.  This triggers a BUG_ON
check in the af_key code when the cloned policy is retrieved.

This patch fixes it by copying the family field over.

Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 6eb228a..2a60938 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1306,6 +1306,7 @@ static struct xfrm_policy *clone_policy(const struct xfrm_policy *old, int dir)
 		newp->xfrm_nr = old->xfrm_nr;
 		newp->index = old->index;
 		newp->type = old->type;
+		newp->family = old->family;
 		memcpy(newp->xfrm_vec, old->xfrm_vec,
 		       newp->xfrm_nr*sizeof(struct xfrm_tmpl));
 		spin_lock_bh(&net->xfrm.xfrm_policy_lock);
-- 
2.7.4

[PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[Steffen Klassert]

This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.

This commit breaks transport mode when the policy template
has widlcard addresses configured, so revert it.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 2a60938..6bc16bb 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1362,29 +1362,36 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
 	struct net *net = xp_net(policy);
 	int nx;
 	int i, error;
+	xfrm_address_t *daddr = xfrm_flowi_daddr(fl, family);
+	xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family);
 	xfrm_address_t tmp;
 
 	for (nx = 0, i = 0; i < policy->xfrm_nr; i++) {
 		struct xfrm_state *x;
-		xfrm_address_t *local;
-		xfrm_address_t *remote;
+		xfrm_address_t *remote = daddr;
+		xfrm_address_t *local  = saddr;
 		struct xfrm_tmpl *tmpl = &policy->xfrm_vec[i];
 
-		remote = &tmpl->id.daddr;
-		local = &tmpl->saddr;
-		if (xfrm_addr_any(local, tmpl->encap_family)) {
-			error = xfrm_get_saddr(net, fl->flowi_oif,
-					       &tmp, remote,
-					       tmpl->encap_family, 0);
-			if (error)
-				goto fail;
-			local = &tmp;
+		if (tmpl->mode == XFRM_MODE_TUNNEL ||
+		    tmpl->mode == XFRM_MODE_BEET) {
+			remote = &tmpl->id.daddr;
+			local = &tmpl->saddr;
+			if (xfrm_addr_any(local, tmpl->encap_family)) {
+				error = xfrm_get_saddr(net, fl->flowi_oif,
+						       &tmp, remote,
+						       tmpl->encap_family, 0);
+				if (error)
+					goto fail;
+				local = &tmp;
+			}
 		}
 
 		x = xfrm_state_find(remote, local, fl, tmpl, policy, &error, family);
 
 		if (x && x->km.state == XFRM_STATE_VALID) {
 			xfrm[nx++] = x;
+			daddr = remote;
+			saddr = local;
 			continue;
 		}
 		if (x) {
-- 
2.7.4

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[Steffen Klassert]

On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
> 
> This commit breaks transport mode when the policy template
> has widlcard addresses configured, so revert it.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

David, can you please queue this one up for v4.14-stable?
Commit ID is 94802151894d482e82c324edf2c658f8e6b96508

v4.14 is unusable for some people without this revert.

Thanks!

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Sat, 23 Dec 2017 10:22:17 +0100

> On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
>> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
>> 
>> This commit breaks transport mode when the policy template
>> has widlcard addresses configured, so revert it.
>> 
>> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> David, can you please queue this one up for v4.14-stable?
> Commit ID is 94802151894d482e82c324edf2c658f8e6b96508
> 
> v4.14 is unusable for some people without this revert.

Yes, but it adds back the stack out-of-bounds bug.

If I queue up the revert, I would also need to queue up whatever
follow-on you used to fix the out-of-bounds bug properly.  Which
commit is that?

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[Steffen Klassert]

On Sat, Dec 23, 2017 at 10:56:12AM -0500, David Miller wrote:
> From: Steffen Klassert <steffen.klassert@secunet.com>
> Date: Sat, 23 Dec 2017 10:22:17 +0100
> 
> > On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
> >> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
> >> 
> >> This commit breaks transport mode when the policy template
> >> has widlcard addresses configured, so revert it.
> >> 
> >> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > David, can you please queue this one up for v4.14-stable?
> > Commit ID is 94802151894d482e82c324edf2c658f8e6b96508
> > 
> > v4.14 is unusable for some people without this revert.
> 
> Yes, but it adds back the stack out-of-bounds bug.
> 
> If I queue up the revert, I would also need to queue up whatever
> follow-on you used to fix the out-of-bounds bug properly.  Which
> commit is that?

This is commit ddc47e4404b58f03e98345398fb12d38fe291512
("xfrm: Fix stack-out-of-bounds read on socket policy lookup.")

It is included in the pull request for the net tree that
I sent yesterday. The patch looks save, but not so sure
if it should go directly to stable. These bugs reported by
the syzbot are usually quite subtile and I already broke
something when I tried to fix the original stack out-of-bounds
bug. So maybe we should wait until the v4.15 release before
backporting...

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[robsonde]

> On 24/12/2017, at 5:09 AM, Steffen Klassert <steffen.klassert@secunet.com> wrote:
> 
>> On Sat, Dec 23, 2017 at 10:56:12AM -0500, David Miller wrote:
>> From: Steffen Klassert <steffen.klassert@secunet.com>
>> Date: Sat, 23 Dec 2017 10:22:17 +0100
>> 
>>>> On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
>>>> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
>>>> 
>>>> This commit breaks transport mode when the policy template
>>>> has widlcard addresses configured, so revert it.
>>>> 
>>>> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>>> 
>>> David, can you please queue this one up for v4.14-stable?
>>> Commit ID is 94802151894d482e82c324edf2c658f8e6b96508
>>> 
>>> v4.14 is unusable for some people without this revert.
>> 
>> Yes, but it adds back the stack out-of-bounds bug.
>> 
>> If I queue up the revert, I would also need to queue up whatever
>> follow-on you used to fix the out-of-bounds bug properly.  Which
>> commit is that?
> 
> This is commit ddc47e4404b58f03e98345398fb12d38fe291512
> ("xfrm: Fix stack-out-of-bounds read on socket policy lookup.")
> 
> It is included in the pull request for the net tree that
> I sent yesterday. The patch looks save, but not so sure
> if it should go directly to stable. These bugs reported by
> the syzbot are usually quite subtile and I already broke
> something when I tried to fix the original stack out-of-bounds
> bug. So maybe we should wait until the v4.15 release before
> backporting...
> 

At this time I cant build an IPSec VPN on a 4.14 kernel. 4.14 is LTS and so has been used as kernel choice for a prod system. This is a production fault.

My only choice is going back to a 4.13 or waiting for 4.15 or a custom build, which I am not keen on.

Unless the memory bug effects security and can be exploited, then I ask that the revert be back ported to 4.14

Thanks

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[Nicolas Dichtel]

Le 23/12/2017 à 17:09, Steffen Klassert a écrit :
> On Sat, Dec 23, 2017 at 10:56:12AM -0500, David Miller wrote:
>> From: Steffen Klassert <steffen.klassert@secunet.com>
>> Date: Sat, 23 Dec 2017 10:22:17 +0100
>>
>>> On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
>>>> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
>>>>
>>>> This commit breaks transport mode when the policy template
>>>> has widlcard addresses configured, so revert it.
>>>>
>>>> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>>>
>>> David, can you please queue this one up for v4.14-stable?
>>> Commit ID is 94802151894d482e82c324edf2c658f8e6b96508
>>>
>>> v4.14 is unusable for some people without this revert.
>>
>> Yes, but it adds back the stack out-of-bounds bug.
>>
>> If I queue up the revert, I would also need to queue up whatever
>> follow-on you used to fix the out-of-bounds bug properly.  Which
>> commit is that?
> 
> This is commit ddc47e4404b58f03e98345398fb12d38fe291512
> ("xfrm: Fix stack-out-of-bounds read on socket policy lookup.")
> 
> It is included in the pull request for the net tree that
> I sent yesterday. The patch looks save, but not so sure
> if it should go directly to stable. These bugs reported by
> the syzbot are usually quite subtile and I already broke
> something when I tried to fix the original stack out-of-bounds
> bug. So maybe we should wait until the v4.15 release before
> backporting...
> 
This patch is still missing in the 4.14 stable. Without it, some IPsec scenarii
are broken. Is there a plan to queue this patch for the 4.14 stable ?


Thank you,
Nicolas

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 5 Jan 2018 17:12:59 +0100

> Le 23/12/2017 à 17:09, Steffen Klassert a écrit :
>> On Sat, Dec 23, 2017 at 10:56:12AM -0500, David Miller wrote:
>>> From: Steffen Klassert <steffen.klassert@secunet.com>
>>> Date: Sat, 23 Dec 2017 10:22:17 +0100
>>>
>>>> On Thu, Nov 16, 2017 at 11:00:40AM +0100, Steffen Klassert wrote:
>>>>> This reverts commit c9f3f813d462c72dbe412cee6a5cbacf13c4ad5e.
>>>>>
>>>>> This commit breaks transport mode when the policy template
>>>>> has widlcard addresses configured, so revert it.
>>>>>
>>>>> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>>>>
>>>> David, can you please queue this one up for v4.14-stable?
>>>> Commit ID is 94802151894d482e82c324edf2c658f8e6b96508
>>>>
>>>> v4.14 is unusable for some people without this revert.
>>>
>>> Yes, but it adds back the stack out-of-bounds bug.
>>>
>>> If I queue up the revert, I would also need to queue up whatever
>>> follow-on you used to fix the out-of-bounds bug properly.  Which
>>> commit is that?
>> 
>> This is commit ddc47e4404b58f03e98345398fb12d38fe291512
>> ("xfrm: Fix stack-out-of-bounds read on socket policy lookup.")
>> 
>> It is included in the pull request for the net tree that
>> I sent yesterday. The patch looks save, but not so sure
>> if it should go directly to stable. These bugs reported by
>> the syzbot are usually quite subtile and I already broke
>> something when I tried to fix the original stack out-of-bounds
>> bug. So maybe we should wait until the v4.15 release before
>> backporting...
>> 
> This patch is still missing in the 4.14 stable. Without it, some IPsec scenarii
> are broken. Is there a plan to queue this patch for the 4.14 stable ?

I will in my next batch of stable submissions.

Re: [PATCH 2/2] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."

[Nicolas Dichtel]

Le 05/01/2018 à 18:17, David Miller a écrit :
[snip]
> I will in my next batch of stable submissions.
> 
Thank you!

Re: pull request (net): ipsec 2017-11-16

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Thu, 16 Nov 2017 11:00:38 +0100

> 1) Copy policy family in clone_policy, otherwise this can
>    trigger a BUG_ON in af_key. From Herbert Xu.
> 
> 2) Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find."
>    This added a regression with transport mode when no addresses
>    are configured on the policy template.
> 
> Both patches are stable candidates.
> 
> Please pull or let me know if there are problems.

Pulled, thanks Steffen.