[syzbot] kernel BUG in warn_crc32c_csum_combine

[syzbot] kernel BUG in warn_crc32c_csum_combine

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    4d48f589d294 Add linux-next specific files for 20221021
git tree:       linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1224e236880000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2c4b7d600a5739a6
dashboard link: https://syzkaller.appspot.com/bug?extid=1e9af9185d8850e2c2fa
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16f390f2880000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171f9c8c880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/0c86bd0b39a0/disk-4d48f589.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/074059d37f1f/vmlinux-4d48f589.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/4a30bce99f60/mount_1.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com

------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:120!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 3637 Comm: syz-executor164 Not tainted 6.1.0-rc1-next-20221021-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/11/2022
RIP: 0010:skb_push.cold-0x2/0x24
Code: f8 4c 8b 4c 24 10 8b 4b 70 41 56 45 89 e8 4c 89 e2 41 57 48 89 ee 48 c7 c7 80 69 d4 8a ff 74 24 10 ff 74 24 20 e8 b2 77 c1 ff <0f> 0b e8 d4 d0 f1 f7 4c 8b 64 24 18 e8 ba 52 3e f8 48 c7 c1 e0 76
RSP: 0018:ffffc90003e7ee70 EFLAGS: 00010286
RAX: 0000000000000086 RBX: ffff888079c00280 RCX: 0000000000000000
RDX: ffff888020a3ba80 RSI: ffffffff81621a58 RDI: fffff520007cfdc0
RBP: ffffffff8ad47720 R08: 0000000000000086 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000075626b73 R12: ffffffff883cc6c6
R13: 0000000000000048 R14: ffffffff8ad46940 R15: 00000000000000c0
FS:  00007f2b0a939700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f9f0b184060 CR3: 00000000755db000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 skb_over_panic net/core/skbuff.c:125 [inline]
 warn_crc32c_csum_combine.cold+0x0/0x1d net/core/skbuff.c:2152
 dump_esp_combs net/key/af_key.c:3009 [inline]
 pfkey_send_acquire+0x1856/0x2520 net/key/af_key.c:3230
 km_query+0xac/0x220 net/xfrm/xfrm_state.c:2248
 xfrm_state_find+0x2bfe/0x4f10 net/xfrm/xfrm_state.c:1165
 xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:2392 [inline]
 xfrm_tmpl_resolve+0x2f3/0xd40 net/xfrm/xfrm_policy.c:2437
 xfrm_resolve_and_create_bundle+0x123/0x2580 net/xfrm/xfrm_policy.c:2730
 xfrm_lookup_with_ifid+0x229/0x20f0 net/xfrm/xfrm_policy.c:3064
 xfrm_lookup net/xfrm/xfrm_policy.c:3193 [inline]
 xfrm_lookup_route+0x36/0x1e0 net/xfrm/xfrm_policy.c:3204
 ip_route_output_flow+0x114/0x150 net/ipv4/route.c:2880
 udp_sendmsg+0x1963/0x2740 net/ipv4/udp.c:1224
 inet_sendmsg+0x99/0xe0 net/ipv4/af_inet.c:825
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:734
 ____sys_sendmsg+0x334/0x8c0 net/socket.c:2482
 ___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
 __sys_sendmmsg+0x18b/0x460 net/socket.c:2622
 __do_sys_sendmmsg net/socket.c:2651 [inline]
 __se_sys_sendmmsg net/socket.c:2648 [inline]
 __x64_sys_sendmmsg+0x99/0x100 net/socket.c:2648
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f2b0a9adf89
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f2b0a9392f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
RAX: ffffffffffffffda RBX: 00007f2b0aa334d0 RCX: 00007f2b0a9adf89
RDX: 000000000800001d RSI: 0000000020007fc0 RDI: 0000000000000003
RBP: 00007f2b0aa002b8 R08: 0000000000000000 R09: 0000000000000000
R10: 000000a742250118 R11: 0000000000000246 R12: 00007f2b0aa000b8
R13: 00007f2b0aa001b8 R14: 0100000000000000 R15: 00007f2b0aa334d8
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:skb_push.cold-0x2/0x24
Code: f8 4c 8b 4c 24 10 8b 4b 70 41 56 45 89 e8 4c 89 e2 41 57 48 89 ee 48 c7 c7 80 69 d4 8a ff 74 24 10 ff 74 24 20 e8 b2 77 c1 ff <0f> 0b e8 d4 d0 f1 f7 4c 8b 64 24 18 e8 ba 52 3e f8 48 c7 c1 e0 76
RSP: 0018:ffffc90003e7ee70 EFLAGS: 00010286
RAX: 0000000000000086 RBX: ffff888079c00280 RCX: 0000000000000000
RDX: ffff888020a3ba80 RSI: ffffffff81621a58 RDI: fffff520007cfdc0
RBP: ffffffff8ad47720 R08: 0000000000000086 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000075626b73 R12: ffffffff883cc6c6
R13: 0000000000000048 R14: ffffffff8ad46940 R15: 00000000000000c0
FS:  00007f2b0a939700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fcc1415d300 CR3: 00000000755db000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: [syzbot] kernel BUG in warn_crc32c_csum_combine

[Eric Dumazet]

On Sun, Oct 23, 2022 at 7:37 PM syzbot
<syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    4d48f589d294 Add linux-next specific files for 20221021
> git tree:       linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=1224e236880000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=2c4b7d600a5739a6
> dashboard link: https://syzkaller.appspot.com/bug?extid=1e9af9185d8850e2c2fa
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16f390f2880000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171f9c8c880000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/0c86bd0b39a0/disk-4d48f589.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/074059d37f1f/vmlinux-4d48f589.xz
> mounted in repro: https://storage.googleapis.com/syzbot-assets/4a30bce99f60/mount_1.gz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:120!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 3637 Comm: syz-executor164 Not tainted 6.1.0-rc1-next-20221021-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/11/2022
> RIP: 0010:skb_push.cold-0x2/0x24
> Code: f8 4c 8b 4c 24 10 8b 4b 70 41 56 45 89 e8 4c 89 e2 41 57 48 89 ee 48 c7 c7 80 69 d4 8a ff 74 24 10 ff 74 24 20 e8 b2 77 c1 ff <0f> 0b e8 d4 d0 f1 f7 4c 8b 64 24 18 e8 ba 52 3e f8 48 c7 c1 e0 76
> RSP: 0018:ffffc90003e7ee70 EFLAGS: 00010286
> RAX: 0000000000000086 RBX: ffff888079c00280 RCX: 0000000000000000
> RDX: ffff888020a3ba80 RSI: ffffffff81621a58 RDI: fffff520007cfdc0
> RBP: ffffffff8ad47720 R08: 0000000000000086 R09: 0000000000000000
> R10: 0000000080000000 R11: 0000000075626b73 R12: ffffffff883cc6c6
> R13: 0000000000000048 R14: ffffffff8ad46940 R15: 00000000000000c0
> FS:  00007f2b0a939700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f9f0b184060 CR3: 00000000755db000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  skb_over_panic net/core/skbuff.c:125 [inline]
>  warn_crc32c_csum_combine.cold+0x0/0x1d net/core/skbuff.c:2152
>  dump_esp_combs net/key/af_key.c:3009 [inline]
>  pfkey_send_acquire+0x1856/0x2520 net/key/af_key.c:3230
>  km_query+0xac/0x220 net/xfrm/xfrm_state.c:2248
>  xfrm_state_find+0x2bfe/0x4f10 net/xfrm/xfrm_state.c:1165
>  xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:2392 [inline]
>  xfrm_tmpl_resolve+0x2f3/0xd40 net/xfrm/xfrm_policy.c:2437
>  xfrm_resolve_and_create_bundle+0x123/0x2580 net/xfrm/xfrm_policy.c:2730
>  xfrm_lookup_with_ifid+0x229/0x20f0 net/xfrm/xfrm_policy.c:3064
>  xfrm_lookup net/xfrm/xfrm_policy.c:3193 [inline]
>  xfrm_lookup_route+0x36/0x1e0 net/xfrm/xfrm_policy.c:3204
>  ip_route_output_flow+0x114/0x150 net/ipv4/route.c:2880
>  udp_sendmsg+0x1963/0x2740 net/ipv4/udp.c:1224
>  inet_sendmsg+0x99/0xe0 net/ipv4/af_inet.c:825
>  sock_sendmsg_nosec net/socket.c:714 [inline]
>  sock_sendmsg+0xcf/0x120 net/socket.c:734
>  ____sys_sendmsg+0x334/0x8c0 net/socket.c:2482
>  ___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
>  __sys_sendmmsg+0x18b/0x460 net/socket.c:2622
>  __do_sys_sendmmsg net/socket.c:2651 [inline]
>  __se_sys_sendmmsg net/socket.c:2648 [inline]
>  __x64_sys_sendmmsg+0x99/0x100 net/socket.c:2648
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x7f2b0a9adf89
> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f2b0a9392f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
> RAX: ffffffffffffffda RBX: 00007f2b0aa334d0 RCX: 00007f2b0a9adf89
> RDX: 000000000800001d RSI: 0000000020007fc0 RDI: 0000000000000003
> RBP: 00007f2b0aa002b8 R08: 0000000000000000 R09: 0000000000000000
> R10: 000000a742250118 R11: 0000000000000246 R12: 00007f2b0aa000b8
> R13: 00007f2b0aa001b8 R14: 0100000000000000 R15: 00007f2b0aa334d8
>  </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:skb_push.cold-0x2/0x24
> Code: f8 4c 8b 4c 24 10 8b 4b 70 41 56 45 89 e8 4c 89 e2 41 57 48 89 ee 48 c7 c7 80 69 d4 8a ff 74 24 10 ff 74 24 20 e8 b2 77 c1 ff <0f> 0b e8 d4 d0 f1 f7 4c 8b 64 24 18 e8 ba 52 3e f8 48 c7 c1 e0 76
> RSP: 0018:ffffc90003e7ee70 EFLAGS: 00010286
> RAX: 0000000000000086 RBX: ffff888079c00280 RCX: 0000000000000000
> RDX: ffff888020a3ba80 RSI: ffffffff81621a58 RDI: fffff520007cfdc0
> RBP: ffffffff8ad47720 R08: 0000000000000086 R09: 0000000000000000
> R10: 0000000080000000 R11: 0000000075626b73 R12: ffffffff883cc6c6
> R13: 0000000000000048 R14: ffffffff8ad46940 R15: 00000000000000c0
> FS:  00007f2b0a939700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fcc1415d300 CR3: 00000000755db000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this issue, for details see:
> https://goo.gl/tpsmEJ#testing-patches

pfkey_send_acquire() allocates and skb, and then later this skb seems
to be too small to fit all dump info.

Maybe ->available status flips during the duration of the call ?

(So count_esp_combs() might return a value, but later dump_esp_combs()
needs more space)


Relevant patch suggests this could happen

commit ba953a9d89a00c078b85f4b190bc1dde66fe16b5
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu Aug 4 18:03:46 2022 +0800

    af_key: Do not call xfrm_probe_algs in parallel

Re: [syzbot] kernel BUG in warn_crc32c_csum_combine

[Herbert Xu]

On Sun, Oct 23, 2022 at 07:49:44PM -0700, Eric Dumazet wrote:
>
> pfkey_send_acquire() allocates and skb, and then later this skb seems
> to be too small to fit all dump info.
> 
> Maybe ->available status flips during the duration of the call ?
> 
> (So count_esp_combs() might return a value, but later dump_esp_combs()
> needs more space)

Thanks!

> Relevant patch suggests this could happen
> 
> commit ba953a9d89a00c078b85f4b190bc1dde66fe16b5
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu Aug 4 18:03:46 2022 +0800
> 
>     af_key: Do not call xfrm_probe_algs in parallel

Yes this looks like the same issue just in a different spot.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

With name space support, it is possible for a pfkey_register to
occur in the middle of a send_acquire, thus changing the number
of supported algorithms.

This can be fixed by taking a mutex to make it single-threaded
again.

Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/key/af_key.c b/net/key/af_key.c
index c85df5b958d2..4ceef96fef57 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -3160,6 +3160,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		(sockaddr_size * 2) +
 		sizeof(struct sadb_x_policy);
 
+	mutex_lock(&pfkey_mutex);
 	if (x->id.proto == IPPROTO_AH)
 		size += count_ah_combs(t);
 	else if (x->id.proto == IPPROTO_ESP)
@@ -3171,8 +3172,10 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	}
 
 	skb =  alloc_skb(size + 16, GFP_ATOMIC);
-	if (skb == NULL)
+	if (skb == NULL) {
+		mutex_unlock(&pfkey_mutex);
 		return -ENOMEM;
+	}
 
 	hdr = skb_put(skb, sizeof(struct sadb_msg));
 	hdr->sadb_msg_version = PF_KEY_V2;
@@ -3228,6 +3231,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		dump_ah_combs(skb, t);
 	else if (x->id.proto == IPPROTO_ESP)
 		dump_esp_combs(skb, t);
+	mutex_unlock(&pfkey_mutex);
 
 	/* security context */
 	if (xfrm_ctx) {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] af_key: Fix send_acquire race with pfkey_register

[Eric Dumazet]

On Sun, Oct 23, 2022 at 10:10 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> With name space support, it is possible for a pfkey_register to
> occur in the middle of a send_acquire, thus changing the number
> of supported algorithms.
>
> This can be fixed by taking a mutex to make it single-threaded
> again.
>
> Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
> Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index c85df5b958d2..4ceef96fef57 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -3160,6 +3160,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>                 (sockaddr_size * 2) +
>                 sizeof(struct sadb_x_policy);
>
> +       mutex_lock(&pfkey_mutex);
>         if (x->id.proto == IPPROTO_AH)
>                 size += count_ah_combs(t);
>         else if (x->id.proto == IPPROTO_ESP)
> @@ -3171,8 +3172,10 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>         }
>
>         skb =  alloc_skb(size + 16, GFP_ATOMIC);

Are you sure we can sleep in mutex_lock() ?

Use of GFP_ATOMIC would suggest otherwise :/


> -       if (skb == NULL)
> +       if (skb == NULL) {
> +               mutex_unlock(&pfkey_mutex);
>                 return -ENOMEM;
> +       }
>
>         hdr = skb_put(skb, sizeof(struct sadb_msg));
>         hdr->sadb_msg_version = PF_KEY_V2;
> @@ -3228,6 +3231,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>                 dump_ah_combs(skb, t);
>         else if (x->id.proto == IPPROTO_ESP)
>                 dump_esp_combs(skb, t);
> +       mutex_unlock(&pfkey_mutex);
>
>         /* security context */
>         if (xfrm_ctx) {
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[v2 PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

On Sun, Oct 23, 2022 at 10:21:05PM -0700, Eric Dumazet wrote:
>
> Are you sure we can sleep in mutex_lock() ?
> 
> Use of GFP_ATOMIC would suggest otherwise :/

Good point.  Acquires are triggered from the network stack so
it may be in BH context.

---8<---
With name space support, it is possible for a pfkey_register to
occur in the middle of a send_acquire, thus changing the number
of supported algorithms.

This can be fixed by taking a lock to make it single-threaded
again.  As this lock can be taken from both thread and softirq
contexts, we need to take the necessary precausions with disabling
BH and make it a spin lock.

Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/key/af_key.c b/net/key/af_key.c
index c85df5b958d2..4e0d21e2045e 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -23,6 +23,7 @@
 #include <linux/proc_fs.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <net/xfrm.h>
@@ -39,6 +40,7 @@ struct netns_pfkey {
 	atomic_t socks_nr;
 };
 static DEFINE_MUTEX(pfkey_mutex);
+static DEFINE_SPINLOCK(pfkey_alg_lock);
 
 #define DUMMY_MARK 0
 static const struct xfrm_mark dummy_mark = {0, 0};
@@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
 		pfk->registered |= (1<<hdr->sadb_msg_satype);
 	}
 
-	mutex_lock(&pfkey_mutex);
+	spin_lock_bh(&pfkey_alg_lock);
 	xfrm_probe_algs();
 
 	supp_skb = compose_sadb_supported(hdr, GFP_KERNEL | __GFP_ZERO);
-	mutex_unlock(&pfkey_mutex);
+	spin_unlock_bh(&pfkey_alg_lock);
 
 	if (!supp_skb) {
 		if (hdr->sadb_msg_satype != SADB_SATYPE_UNSPEC)
@@ -3160,6 +3162,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		(sockaddr_size * 2) +
 		sizeof(struct sadb_x_policy);
 
+	spin_lock_bh(&pfkey_alg_lock);
 	if (x->id.proto == IPPROTO_AH)
 		size += count_ah_combs(t);
 	else if (x->id.proto == IPPROTO_ESP)
@@ -3171,8 +3174,10 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	}
 
 	skb =  alloc_skb(size + 16, GFP_ATOMIC);
-	if (skb == NULL)
+	if (skb == NULL) {
+		spin_unlock_bh(&pfkey_alg_lock);
 		return -ENOMEM;
+	}
 
 	hdr = skb_put(skb, sizeof(struct sadb_msg));
 	hdr->sadb_msg_version = PF_KEY_V2;
@@ -3228,6 +3233,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		dump_ah_combs(skb, t);
 	else if (x->id.proto == IPPROTO_ESP)
 		dump_esp_combs(skb, t);
+	spin_unlock_bh(&pfkey_alg_lock);
 
 	/* security context */
 	if (xfrm_ctx) {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] af_key: Fix send_acquire race with pfkey_register

[Eric Dumazet]

On Sun, Oct 23, 2022 at 11:06 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Sun, Oct 23, 2022 at 10:21:05PM -0700, Eric Dumazet wrote:
> >
> > Are you sure we can sleep in mutex_lock() ?
> >
> > Use of GFP_ATOMIC would suggest otherwise :/
>
> Good point.  Acquires are triggered from the network stack so
> it may be in BH context.
>
> ---8<---
> With name space support, it is possible for a pfkey_register to
> occur in the middle of a send_acquire, thus changing the number
> of supported algorithms.
>
> This can be fixed by taking a lock to make it single-threaded
> again.  As this lock can be taken from both thread and softirq
> contexts, we need to take the necessary precausions with disabling
> BH and make it a spin lock.
>
> Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
> Fixes: 283bc9f35bbb ("xfrm: Namespacify xfrm state/policy locks")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index c85df5b958d2..4e0d21e2045e 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -23,6 +23,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <net/net_namespace.h>
>  #include <net/netns/generic.h>
>  #include <net/xfrm.h>
> @@ -39,6 +40,7 @@ struct netns_pfkey {
>         atomic_t socks_nr;
>  };
>  static DEFINE_MUTEX(pfkey_mutex);
> +static DEFINE_SPINLOCK(pfkey_alg_lock);
>
>  #define DUMMY_MARK 0
>  static const struct xfrm_mark dummy_mark = {0, 0};
> @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
>                 pfk->registered |= (1<<hdr->sadb_msg_satype);
>         }
>
> -       mutex_lock(&pfkey_mutex);
> +       spin_lock_bh(&pfkey_alg_lock);
>         xfrm_probe_algs();
>
>         supp_skb = compose_sadb_supported(hdr, GFP_KERNEL | __GFP_ZERO);

s/GFP_KERNEL/GFP_ATOMIC/

> -       mutex_unlock(&pfkey_mutex);
> +       spin_unlock_bh(&pfkey_alg_lock);
>
>         if (!supp_skb) {
>                 if (hdr->sadb_msg_satype != SADB_SATYPE_UNSPEC)
> @@ -3160,6 +3162,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>                 (sockaddr_size * 2) +
>                 sizeof(struct sadb_x_policy);
>
> +       spin_lock_bh(&pfkey_alg_lock);
>         if (x->id.proto == IPPROTO_AH)
>                 size += count_ah_combs(t);
>         else if (x->id.proto == IPPROTO_ESP)
> @@ -3171,8 +3174,10 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>         }
>
>         skb =  alloc_skb(size + 16, GFP_ATOMIC);
> -       if (skb == NULL)
> +       if (skb == NULL) {
> +               spin_unlock_bh(&pfkey_alg_lock);
>                 return -ENOMEM;
> +       }
>
>         hdr = skb_put(skb, sizeof(struct sadb_msg));
>         hdr->sadb_msg_version = PF_KEY_V2;
> @@ -3228,6 +3233,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
>                 dump_ah_combs(skb, t);
>         else if (x->id.proto == IPPROTO_ESP)
>                 dump_esp_combs(skb, t);
> +       spin_unlock_bh(&pfkey_alg_lock);
>
>         /* security context */
>         if (xfrm_ctx) {
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] af_key: Fix send_acquire race with pfkey_register

[Sabrina Dubroca]

2022-10-24, 14:06:12 +0800, Herbert Xu wrote:
> @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
>  		pfk->registered |= (1<<hdr->sadb_msg_satype);
>  	}
>  
> -	mutex_lock(&pfkey_mutex);
> +	spin_lock_bh(&pfkey_alg_lock);
>  	xfrm_probe_algs();

I don't think we can do that:

void xfrm_probe_algs(void)
{
	int i, status;

	BUG_ON(in_softirq());


>  
>  	supp_skb = compose_sadb_supported(hdr, GFP_KERNEL | __GFP_ZERO);
> -	mutex_unlock(&pfkey_mutex);
> +	spin_unlock_bh(&pfkey_alg_lock);
>  
>  	if (!supp_skb) {
>  		if (hdr->sadb_msg_satype != SADB_SATYPE_UNSPEC)

-- 
Sabrina

Re: [v2 PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

On Sun, Oct 23, 2022 at 11:31:00PM -0700, Eric Dumazet wrote:
>
> s/GFP_KERNEL/GFP_ATOMIC/

Thanks, I clearly wasn't thinking when I made this patch :)
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

On Mon, Oct 24, 2022 at 09:20:00AM +0200, Sabrina Dubroca wrote:
> 2022-10-24, 14:06:12 +0800, Herbert Xu wrote:
> > @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
> >  		pfk->registered |= (1<<hdr->sadb_msg_satype);
> >  	}
> >  
> > -	mutex_lock(&pfkey_mutex);
> > +	spin_lock_bh(&pfkey_alg_lock);
> >  	xfrm_probe_algs();
> 
> I don't think we can do that:
> 
> void xfrm_probe_algs(void)
> {
> 	int i, status;
> 
> 	BUG_ON(in_softirq());

Indeed.  I was also wrong in stating that this bug was created by
namespaces.  This race has always existed since this code was first
added.

---8<---
The function pfkey_send_acquire may race with pfkey_register
(which could even be in a different name space).  This may result
in a buffer overrun.

Allocating the maximum amount of memory that could be used prevents
this.

Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/key/af_key.c b/net/key/af_key.c
index c85df5b958d2..213287814328 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2905,7 +2905,7 @@ static int count_ah_combs(const struct xfrm_tmpl *t)
 			break;
 		if (!aalg->pfkey_supported)
 			continue;
-		if (aalg_tmpl_set(t, aalg) && aalg->available)
+		if (aalg_tmpl_set(t, aalg))
 			sz += sizeof(struct sadb_comb);
 	}
 	return sz + sizeof(struct sadb_prop);
@@ -2923,7 +2923,7 @@ static int count_esp_combs(const struct xfrm_tmpl *t)
 		if (!ealg->pfkey_supported)
 			continue;
 
-		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
+		if (!(ealg_tmpl_set(t, ealg)))
 			continue;
 
 		for (k = 1; ; k++) {
@@ -2934,16 +2934,17 @@ static int count_esp_combs(const struct xfrm_tmpl *t)
 			if (!aalg->pfkey_supported)
 				continue;
 
-			if (aalg_tmpl_set(t, aalg) && aalg->available)
+			if (aalg_tmpl_set(t, aalg))
 				sz += sizeof(struct sadb_comb);
 		}
 	}
 	return sz + sizeof(struct sadb_prop);
 }
 
-static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
+static int dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 {
 	struct sadb_prop *p;
+	int sz = 0;
 	int i;
 
 	p = skb_put(skb, sizeof(struct sadb_prop));
@@ -2971,13 +2972,17 @@ static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			c->sadb_comb_soft_addtime = 20*60*60;
 			c->sadb_comb_hard_usetime = 8*60*60;
 			c->sadb_comb_soft_usetime = 7*60*60;
+			sz += sizeof(*c);
 		}
 	}
+
+	return sz + sizeof(*p);
 }
 
-static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
+static int dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 {
 	struct sadb_prop *p;
+	int sz = 0;
 	int i, k;
 
 	p = skb_put(skb, sizeof(struct sadb_prop));
@@ -3019,8 +3024,11 @@ static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			c->sadb_comb_soft_addtime = 20*60*60;
 			c->sadb_comb_hard_usetime = 8*60*60;
 			c->sadb_comb_soft_usetime = 7*60*60;
+			sz += sizeof(*c);
 		}
 	}
+
+	return sz + sizeof(*p);
 }
 
 static int key_notify_policy_expire(struct xfrm_policy *xp, const struct km_event *c)
@@ -3150,6 +3158,7 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	struct sadb_x_sec_ctx *sec_ctx;
 	struct xfrm_sec_ctx *xfrm_ctx;
 	int ctx_size = 0;
+	int alg_size = 0;
 
 	sockaddr_size = pfkey_sockaddr_size(x->props.family);
 	if (!sockaddr_size)
@@ -3161,16 +3170,16 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		sizeof(struct sadb_x_policy);
 
 	if (x->id.proto == IPPROTO_AH)
-		size += count_ah_combs(t);
+		alg_size = count_ah_combs(t);
 	else if (x->id.proto == IPPROTO_ESP)
-		size += count_esp_combs(t);
+		alg_size = count_esp_combs(t);
 
 	if ((xfrm_ctx = x->security)) {
 		ctx_size = PFKEY_ALIGN8(xfrm_ctx->ctx_len);
 		size +=  sizeof(struct sadb_x_sec_ctx) + ctx_size;
 	}
 
-	skb =  alloc_skb(size + 16, GFP_ATOMIC);
+	skb =  alloc_skb(size + alg_size + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
@@ -3224,10 +3233,13 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	pol->sadb_x_policy_priority = xp->priority;
 
 	/* Set sadb_comb's. */
+	alg_size = 0;
 	if (x->id.proto == IPPROTO_AH)
-		dump_ah_combs(skb, t);
+		alg_size = dump_ah_combs(skb, t);
 	else if (x->id.proto == IPPROTO_ESP)
-		dump_esp_combs(skb, t);
+		alg_size = dump_esp_combs(skb, t);
+
+	hdr->sadb_msg_len += alg_size / 8;
 
 	/* security context */
 	if (xfrm_ctx) {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Sabrina Dubroca]

2022-10-25, 14:06:48 +0800, Herbert Xu wrote:
> On Mon, Oct 24, 2022 at 09:20:00AM +0200, Sabrina Dubroca wrote:
> > 2022-10-24, 14:06:12 +0800, Herbert Xu wrote:
> > > @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
> > >  		pfk->registered |= (1<<hdr->sadb_msg_satype);
> > >  	}
> > >  
> > > -	mutex_lock(&pfkey_mutex);
> > > +	spin_lock_bh(&pfkey_alg_lock);
> > >  	xfrm_probe_algs();
> > 
> > I don't think we can do that:
> > 
> > void xfrm_probe_algs(void)
> > {
> > 	int i, status;
> > 
> > 	BUG_ON(in_softirq());
> 
> Indeed.  I was also wrong in stating that this bug was created by
> namespaces.  This race has always existed since this code was first
> added.
> 
> ---8<---
> The function pfkey_send_acquire may race with pfkey_register
> (which could even be in a different name space).  This may result
> in a buffer overrun.
> 
> Allocating the maximum amount of memory that could be used prevents
> this.
> 
> Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

LGTM, thanks.

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
>
> LGTM, thanks.
> 
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks for the review and comments!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Eric Dumazet]

On Wed, Oct 26, 2022 at 8:42 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
> >
> > LGTM, thanks.
> >
> > Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
>
> Thanks for the review and comments!

SGTM, thanks for the fix.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Herbert Xu]

On Wed, Oct 26, 2022 at 08:45:57PM -0700, Eric Dumazet wrote:
>
> SGTM, thanks for the fix.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks Eric!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] af_key: Fix send_acquire race with pfkey_register

[Steffen Klassert]

On Wed, Oct 26, 2022 at 08:45:57PM -0700, Eric Dumazet wrote:
> On Wed, Oct 26, 2022 at 8:42 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
> > >
> > > LGTM, thanks.
> > >
> > > Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> >
> > Thanks for the review and comments!
> 
> SGTM, thanks for the fix.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Applied, thanks everyone!