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

Message ID 1510826440-19452-3-git-send-email-steffen.klassert@secunet.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [1/2] xfrm: Copy policy family in clone_policy
Related show

Commit Message

Steffen Klassert Nov. 16, 2017, 10 a.m. UTC
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(-)

Comments

Steffen Klassert Dec. 23, 2017, 9:22 a.m. UTC | #1
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!
David Miller Dec. 23, 2017, 3:56 p.m. UTC | #2
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?
Steffen Klassert Dec. 23, 2017, 4:09 p.m. UTC | #3
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...
robsonde@gmail.com Dec. 23, 2017, 6:10 p.m. UTC | #4
> 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
Nicolas Dichtel Jan. 5, 2018, 4:12 p.m. UTC | #5
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
David Miller Jan. 5, 2018, 5:17 p.m. UTC | #6
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.
Nicolas Dichtel Jan. 8, 2018, 4:06 p.m. UTC | #7
Le 05/01/2018 à 18:17, David Miller a écrit :
[snip]
> I will in my next batch of stable submissions.
> 
Thank you!

Patch
diff mbox series

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) {