netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xfrm: Fix oops in xfrm_replay_advance_bmp
@ 2020-12-14 13:38 Shmulik Ladkani
  2020-12-18 15:16 ` Steffen Klassert
  0 siblings, 1 reply; 3+ messages in thread
From: Shmulik Ladkani @ 2020-12-14 13:38 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: Herbert Xu, David S . Miller, Jakub Kicinski, netdev, Shmulik Ladkani

When setting xfrm replay_window to values higher than 32, a rare
page-fault occurs in xfrm_replay_advance_bmp:

  BUG: unable to handle page fault for address: ffff8af350ad7920
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0002) - not-present page
  PGD ad001067 P4D ad001067 PUD 0
  Oops: 0002 [#1] SMP PTI
  CPU: 3 PID: 30 Comm: ksoftirqd/3 Kdump: loaded Not tainted 5.4.52-050452-generic #202007160732
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014
  RIP: 0010:xfrm_replay_advance_bmp+0xbb/0x130
  RSP: 0018:ffffa1304013ba40 EFLAGS: 00010206
  RAX: 000000000000010d RBX: 0000000000000002 RCX: 00000000ffffff4b
  RDX: 0000000000000018 RSI: 00000000004c234c RDI: 00000000ffb3dbff
  RBP: ffffa1304013ba50 R08: ffff8af330ad7920 R09: 0000000007fffffa
  R10: 0000000000000800 R11: 0000000000000010 R12: ffff8af29d6258c0
  R13: ffff8af28b95c700 R14: 0000000000000000 R15: ffff8af29d6258fc
  FS:  0000000000000000(0000) GS:ffff8af339ac0000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: ffff8af350ad7920 CR3: 0000000015ee4000 CR4: 00000000001406e0
  Call Trace:
   xfrm_input+0x4e5/0xa10
   xfrm4_rcv_encap+0xb5/0xe0
   xfrm4_udp_encap_rcv+0x140/0x1c0

Analysis revealed offending code is when accessing:

	replay_esn->bmp[nr] |= (1U << bitnr);

with 'nr' being 0x07fffffa.

This happened in an SMP system when reordering of packets was present;
A packet arrived with a "too old" sequence number (outside the window,
i.e 'diff > replay_window'), and therefore the following calculation:

			bitnr = replay_esn->replay_window - (diff - pos);

yields a negative result, but since bitnr is u32 we get a large unsigned
quantity (in crash dump above: 0xffffff4b seen in ecx).

This was supposed to be protected by xfrm_input()'s former call to:

		if (x->repl->check(x, skb, seq)) {

However, the state's spinlock x->lock is *released* after '->check()'
is performed, and gets re-acquired before '->advance()' - which gives a
chance for a different core to update the xfrm state, e.g. by advancing
'replay_esn->seq' when it encounters more packets - leading to a
'diff > replay_window' situation when original core continues to
xfrm_replay_advance_bmp().

An attempt to fix this issue was suggested in commit bcf66bf54aab
("xfrm: Perform a replay check after return from async codepaths"),
by calling 'x->repl->recheck()' after lock is re-acquired, but fix
applied only to asyncronous crypto algorithms.

Augment the fix, by *always* calling 'recheck()' - irrespective if we're
using async crypto.

Fixes: 0ebea8ef3559 ("[IPSEC]: Move state lock into x->type->input")
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 net/xfrm/xfrm_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 37456d022cfa..61e6220ddd5a 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -660,7 +660,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		/* only the first xfrm gets the encap type */
 		encap_type = 0;
 
-		if (async && x->repl->recheck(x, skb, seq)) {
+		if (x->repl->recheck(x, skb, seq)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATESEQERROR);
 			goto drop_unlock;
 		}
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] xfrm: Fix oops in xfrm_replay_advance_bmp
  2020-12-14 13:38 [PATCH] xfrm: Fix oops in xfrm_replay_advance_bmp Shmulik Ladkani
@ 2020-12-18 15:16 ` Steffen Klassert
  2020-12-18 15:30   ` Shmulik Ladkani
  0 siblings, 1 reply; 3+ messages in thread
From: Steffen Klassert @ 2020-12-18 15:16 UTC (permalink / raw)
  To: Shmulik Ladkani
  Cc: Herbert Xu, David S . Miller, Jakub Kicinski, netdev, Shmulik Ladkani

On Mon, Dec 14, 2020 at 03:38:32PM +0200, Shmulik Ladkani wrote:
> When setting xfrm replay_window to values higher than 32, a rare
> page-fault occurs in xfrm_replay_advance_bmp:
> 
>   BUG: unable to handle page fault for address: ffff8af350ad7920
>   #PF: supervisor write access in kernel mode
>   #PF: error_code(0x0002) - not-present page
>   PGD ad001067 P4D ad001067 PUD 0
>   Oops: 0002 [#1] SMP PTI
>   CPU: 3 PID: 30 Comm: ksoftirqd/3 Kdump: loaded Not tainted 5.4.52-050452-generic #202007160732
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014
>   RIP: 0010:xfrm_replay_advance_bmp+0xbb/0x130
>   RSP: 0018:ffffa1304013ba40 EFLAGS: 00010206
>   RAX: 000000000000010d RBX: 0000000000000002 RCX: 00000000ffffff4b
>   RDX: 0000000000000018 RSI: 00000000004c234c RDI: 00000000ffb3dbff
>   RBP: ffffa1304013ba50 R08: ffff8af330ad7920 R09: 0000000007fffffa
>   R10: 0000000000000800 R11: 0000000000000010 R12: ffff8af29d6258c0
>   R13: ffff8af28b95c700 R14: 0000000000000000 R15: ffff8af29d6258fc
>   FS:  0000000000000000(0000) GS:ffff8af339ac0000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: ffff8af350ad7920 CR3: 0000000015ee4000 CR4: 00000000001406e0
>   Call Trace:
>    xfrm_input+0x4e5/0xa10
>    xfrm4_rcv_encap+0xb5/0xe0
>    xfrm4_udp_encap_rcv+0x140/0x1c0
> 
> Analysis revealed offending code is when accessing:
> 
> 	replay_esn->bmp[nr] |= (1U << bitnr);
> 
> with 'nr' being 0x07fffffa.
> 
> This happened in an SMP system when reordering of packets was present;
> A packet arrived with a "too old" sequence number (outside the window,
> i.e 'diff > replay_window'), and therefore the following calculation:
> 
> 			bitnr = replay_esn->replay_window - (diff - pos);
> 
> yields a negative result, but since bitnr is u32 we get a large unsigned
> quantity (in crash dump above: 0xffffff4b seen in ecx).
> 
> This was supposed to be protected by xfrm_input()'s former call to:
> 
> 		if (x->repl->check(x, skb, seq)) {
> 
> However, the state's spinlock x->lock is *released* after '->check()'
> is performed, and gets re-acquired before '->advance()' - which gives a
> chance for a different core to update the xfrm state, e.g. by advancing
> 'replay_esn->seq' when it encounters more packets - leading to a
> 'diff > replay_window' situation when original core continues to
> xfrm_replay_advance_bmp().
> 
> An attempt to fix this issue was suggested in commit bcf66bf54aab
> ("xfrm: Perform a replay check after return from async codepaths"),
> by calling 'x->repl->recheck()' after lock is re-acquired, but fix
> applied only to asyncronous crypto algorithms.
> 
> Augment the fix, by *always* calling 'recheck()' - irrespective if we're
> using async crypto.
> 
> Fixes: 0ebea8ef3559 ("[IPSEC]: Move state lock into x->type->input")
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>

Applied, thanks a lot Shmulik!

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] xfrm: Fix oops in xfrm_replay_advance_bmp
  2020-12-18 15:16 ` Steffen Klassert
@ 2020-12-18 15:30   ` Shmulik Ladkani
  0 siblings, 0 replies; 3+ messages in thread
From: Shmulik Ladkani @ 2020-12-18 15:30 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: Herbert Xu, David S . Miller, Jakub Kicinski, netdev, Shmulik Ladkani

On Fri, 18 Dec 2020 16:16:12 +0100
Steffen Klassert <steffen.klassert@secunet.com> wrote:

> Applied, thanks a lot Shmulik!

Thanks Steffen, please make sure this hits -stable



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-12-18 15:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-14 13:38 [PATCH] xfrm: Fix oops in xfrm_replay_advance_bmp Shmulik Ladkani
2020-12-18 15:16 ` Steffen Klassert
2020-12-18 15:30   ` Shmulik Ladkani

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).