* general protection fault in keyctl_pkey_params_get @ 2018-11-03 15:48 syzbot 2018-11-03 17:30 ` [PATCH] KEYS: fix parsing invalid pkey info string Eric Biggers 0 siblings, 1 reply; 18+ messages in thread From: syzbot @ 2018-11-03 15:48 UTC (permalink / raw) To: dhowells, jmorris, keyrings, linux-kernel, linux-security-module, serge, syzkaller-bugs Hello, syzbot found the following crash on: HEAD commit: 5f21585384a4 Merge tag 'for-linus-20181102' of git://git.k.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=10ba246d400000 kernel config: https://syzkaller.appspot.com/x/.config?x=9384ecb1c973baed dashboard link: https://syzkaller.appspot.com/bug?extid=a22e0dc07567662c50bc compiler: gcc (GCC) 8.0.1 20180413 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1026dcd5400000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=167e882b400000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+a22e0dc07567662c50bc@syzkaller.appspotmail.com kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN CPU: 1 PID: 7247 Comm: syz-executor236 Not tainted 4.19.0+ #317 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:keyctl_pkey_params_parse security/keys/keyctl_pkey.c:56 [inline] RIP: 0010:keyctl_pkey_params_get+0x2e7/0x550 security/keys/keyctl_pkey.c:96 Code: fe 48 8b 44 24 38 48 c1 e8 03 42 80 3c 28 00 0f 85 f7 01 00 00 4c 8b a4 24 e0 00 00 00 4c 89 e0 4c 89 e2 48 c1 e8 03 83 e2 07 <42> 0f b6 04 28 38 d0 7f 08 84 c0 0f 85 e0 01 00 00 41 0f b6 04 24 RSP: 0018:ffff8801ce84faa0 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff8801ce84fd60 RCX: ffffffff83429990 RDX: 0000000000000000 RSI: ffffffff8342999e RDI: 0000000000000001 RBP: ffff8801ce84fc08 R08: ffff8801ce928480 R09: ffffed0039e73310 R10: ffffed0039e73310 R11: 0000000000000001 R12: 0000000000000000 R13: dffffc0000000000 R14: ffffffffffffffff R15: ffffffffffffffff FS: 0000000001e1c880(0000) GS:ffff8801daf00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000414cd0 CR3: 00000001ce5e6000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: keyctl_pkey_params_get_2+0x12f/0x580 security/keys/keyctl_pkey.c:130 kasan: CONFIG_KASAN_INLINE enabled keyctl_pkey_verify+0xaa/0x400 security/keys/keyctl_pkey.c:293 kasan: GPF could be caused by NULL-ptr deref or user memory access __do_sys_keyctl security/keys/keyctl.c:1768 [inline] __se_sys_keyctl security/keys/keyctl.c:1637 [inline] __x64_sys_keyctl+0x101/0x430 security/keys/keyctl.c:1637 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x444c39 Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 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 0f 83 2b ce fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffefdd59f88 EFLAGS: 00000286 ORIG_RAX: 00000000000000fa RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000444c39 RDX: 00000000200002c0 RSI: 00000000200000c0 RDI: 000000000000001c RBP: 0000000000000000 R08: 0000000020000380 R09: 00000000004002e0 R10: 00000000fffffd2a R11: 0000000000000286 R12: 0000000000011956 R13: 0000000000401f80 R14: 0000000000000000 R15: 0000000000000000 Modules linked in: ---[ end trace e5c46bb5200c69dc ]--- general protection fault: 0000 [#2] PREEMPT SMP KASAN RIP: 0010:keyctl_pkey_params_parse security/keys/keyctl_pkey.c:56 [inline] RIP: 0010:keyctl_pkey_params_get+0x2e7/0x550 security/keys/keyctl_pkey.c:96 CPU: 0 PID: 7401 Comm: syz-executor236 Tainted: G D 4.19.0+ #317 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:keyctl_pkey_params_parse security/keys/keyctl_pkey.c:56 [inline] RIP: 0010:keyctl_pkey_params_get+0x2e7/0x550 security/keys/keyctl_pkey.c:96 Code: fe 48 8b 44 24 38 48 c1 e8 03 42 80 3c 28 00 0f 85 f7 01 00 00 4c 8b a4 24 e0 00 00 00 4c 89 e0 4c 89 e2 48 c1 e8 03 83 e2 07 <42> 0f b6 04 28 38 d0 7f 08 84 c0 0f 85 e0 01 00 00 41 0f b6 04 24 Code: fe 48 8b 44 24 38 48 c1 e8 03 42 80 3c 28 00 0f 85 f7 01 00 00 4c 8b a4 24 e0 00 00 00 4c 89 e0 4c 89 e2 48 c1 e8 03 83 e2 07 <42> 0f b6 04 28 38 d0 7f 08 84 c0 0f 85 e0 01 00 00 41 0f b6 04 24 RSP: 0018:ffff8801c52cfaa0 EFLAGS: 00010246 RAX: 0000000000000002 RBX: ffff8801c52cfd60 RCX: ffffffff83429990 RDX: 0000000000000000 RSI: ffffffff8342999e RDI: 0000000000000001 RBP: ffff8801c52cfc08 R08: ffff8801b8a86000 R09: ffffed0039bb1378 R10: ffffed0039bb1378 R11: 0000000000000001 R12: 0000000000000010 R13: dffffc0000000000 R14: ffffffffffffffff R15: ffffffffffffffff FS: 0000000001e1c880(0000) GS:ffff8801dae00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 RSP: 0018:ffff8801ce84faa0 EFLAGS: 00010246 CR2: 0000000000414cd0 CR3: 00000001ce654000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: RAX: 0000000000000000 RBX: ffff8801ce84fd60 RCX: ffffffff83429990 keyctl_pkey_params_get_2+0x12f/0x580 security/keys/keyctl_pkey.c:130 RDX: 0000000000000000 RSI: ffffffff8342999e RDI: 0000000000000001 keyctl_pkey_verify+0xaa/0x400 security/keys/keyctl_pkey.c:293 RBP: ffff8801ce84fc08 R08: ffff8801ce928480 R09: ffffed0039e73310 R10: ffffed0039e73310 R11: 0000000000000001 R12: 0000000000000000 __do_sys_keyctl security/keys/keyctl.c:1768 [inline] __se_sys_keyctl security/keys/keyctl.c:1637 [inline] __x64_sys_keyctl+0x101/0x430 security/keys/keyctl.c:1637 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 R13: dffffc0000000000 R14: ffffffffffffffff R15: ffffffffffffffff FS: 0000000001e1c880(0000) GS:ffff8801daf00000(0000) knlGS:0000000000000000 entry_SYSCALL_64_after_hwframe+0x49/0xbe CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 RIP: 0033:0x444c39 Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 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 0f 83 2b ce fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffefdd59f88 EFLAGS: 00000286 ORIG_RAX: 00000000000000fa RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000444c39 RDX: 00000000200002c0 RSI: 00000000200000c0 RDI: 000000000000001c RBP: 0000000000000000 R08: 0000000020000380 R09: 00000000004002e0 R10: 00000000fffffd2a R11: 0000000000000286 R12: 0000000000011a27 CR2: 0000000000414cd0 CR3: 00000001ce5e6000 CR4: 00000000001406e0 R13: 0000000000401f80 R14: 0000000000000000 R15: 0000000000000000 Modules linked in: ---[ end trace e5c46bb5200c69dd ]--- DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 RIP: 0010:keyctl_pkey_params_parse security/keys/keyctl_pkey.c:56 [inline] RIP: 0010:keyctl_pkey_params_get+0x2e7/0x550 security/keys/keyctl_pkey.c:96 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Code: fe 48 8b 44 24 38 48 c1 e8 03 42 80 3c 28 00 0f 85 f7 01 00 00 4c 8b a4 24 e0 00 00 00 4c 89 e0 4c 89 e2 48 c1 e8 03 83 e2 07 <42> 0f b6 04 28 38 d0 7f 08 84 c0 0f 85 e0 01 00 00 41 0f b6 04 24 --- This bug 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 bug report. See: https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] KEYS: fix parsing invalid pkey info string 2018-11-03 15:48 general protection fault in keyctl_pkey_params_get syzbot @ 2018-11-03 17:30 ` Eric Biggers 2018-11-28 23:20 ` Eric Biggers 0 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2018-11-03 17:30 UTC (permalink / raw) To: keyrings, dhowells; +Cc: linux-security-module, linux-kernel, syzkaller-bugs From: Eric Biggers <ebiggers@google.com> We need to check the return value of match_token() for Opt_err (-1) before doing anything with it. Reported-by: syzbot+a22e0dc07567662c50bc@syzkaller.appspotmail.com Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]") Signed-off-by: Eric Biggers <ebiggers@google.com> --- security/keys/keyctl_pkey.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c index 783978842f13a..987fac8051d70 100644 --- a/security/keys/keyctl_pkey.c +++ b/security/keys/keyctl_pkey.c @@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params) if (*p == '\0' || *p == ' ' || *p == '\t') continue; token = match_token(p, param_keys, args); + if (token == Opt_err) + return -EINVAL; if (__test_and_set_bit(token, &token_mask)) return -EINVAL; q = args[0].from; -- 2.19.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] KEYS: fix parsing invalid pkey info string 2018-11-03 17:30 ` [PATCH] KEYS: fix parsing invalid pkey info string Eric Biggers @ 2018-11-28 23:20 ` Eric Biggers 2018-12-06 18:26 ` Eric Biggers 2018-12-17 18:12 ` [PATCH RESEND] " Eric Biggers 0 siblings, 2 replies; 18+ messages in thread From: Eric Biggers @ 2018-11-28 23:20 UTC (permalink / raw) To: keyrings, dhowells; +Cc: linux-security-module, linux-kernel, syzkaller-bugs On Sat, Nov 03, 2018 at 10:30:35AM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > We need to check the return value of match_token() for Opt_err (-1) > before doing anything with it. > > Reported-by: syzbot+a22e0dc07567662c50bc@syzkaller.appspotmail.com > Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]") > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > security/keys/keyctl_pkey.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c > index 783978842f13a..987fac8051d70 100644 > --- a/security/keys/keyctl_pkey.c > +++ b/security/keys/keyctl_pkey.c > @@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params) > if (*p == '\0' || *p == ' ' || *p == '\t') > continue; > token = match_token(p, param_keys, args); > + if (token == Opt_err) > + return -EINVAL; > if (__test_and_set_bit(token, &token_mask)) > return -EINVAL; > q = args[0].from; > -- > 2.19.1 Ping. David, are you planning to apply this? - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] KEYS: fix parsing invalid pkey info string 2018-11-28 23:20 ` Eric Biggers @ 2018-12-06 18:26 ` Eric Biggers 2018-12-17 18:12 ` [PATCH RESEND] " Eric Biggers 1 sibling, 0 replies; 18+ messages in thread From: Eric Biggers @ 2018-12-06 18:26 UTC (permalink / raw) To: keyrings, dhowells; +Cc: linux-security-module, linux-kernel, syzkaller-bugs On Wed, Nov 28, 2018 at 03:20:20PM -0800, Eric Biggers wrote: > On Sat, Nov 03, 2018 at 10:30:35AM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > We need to check the return value of match_token() for Opt_err (-1) > > before doing anything with it. > > > > Reported-by: syzbot+a22e0dc07567662c50bc@syzkaller.appspotmail.com > > Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]") > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > security/keys/keyctl_pkey.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c > > index 783978842f13a..987fac8051d70 100644 > > --- a/security/keys/keyctl_pkey.c > > +++ b/security/keys/keyctl_pkey.c > > @@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params) > > if (*p == '\0' || *p == ' ' || *p == '\t') > > continue; > > token = match_token(p, param_keys, args); > > + if (token == Opt_err) > > + return -EINVAL; > > if (__test_and_set_bit(token, &token_mask)) > > return -EINVAL; > > q = args[0].from; > > -- > > 2.19.1 > > Ping. David, are you planning to apply this? > > - Eric > Ping. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-11-28 23:20 ` Eric Biggers 2018-12-06 18:26 ` Eric Biggers @ 2018-12-17 18:12 ` Eric Biggers 2018-12-17 18:43 ` Linus Torvalds 1 sibling, 1 reply; 18+ messages in thread From: Eric Biggers @ 2018-12-17 18:12 UTC (permalink / raw) To: Linus Torvalds; +Cc: David Howells, keyrings, linux-kernel, syzkaller-bugs From: Eric Biggers <ebiggers@google.com> We need to check the return value of match_token() for Opt_err (-1) before doing anything with it. Reported-by: syzbot+a22e0dc07567662c50bc@syzkaller.appspotmail.com Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]") Cc: David Howells <dhowells@redhat.com> Signed-off-by: Eric Biggers <ebiggers@google.com> --- Hi Linus, please consider applying this patch. It's been ignored by the keyrings maintainer for a month and a half with multiple reminders. It fixes an easily reachable stack corruption in the new keyctl operations that were added in v4.20. It was immediately reached by syzbot even without any definitions for the new keyctls yet. security/keys/keyctl_pkey.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c index 783978842f13a..987fac8051d70 100644 --- a/security/keys/keyctl_pkey.c +++ b/security/keys/keyctl_pkey.c @@ -50,6 +50,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params) if (*p == '\0' || *p == ' ' || *p == '\t') continue; token = match_token(p, param_keys, args); + if (token == Opt_err) + return -EINVAL; if (__test_and_set_bit(token, &token_mask)) return -EINVAL; q = args[0].from; -- 2.20.0.405.gbc1bbc6f85-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 18:12 ` [PATCH RESEND] " Eric Biggers @ 2018-12-17 18:43 ` Linus Torvalds 2018-12-17 18:49 ` Linus Torvalds 2018-12-18 12:34 ` Dmitry Vyukov 0 siblings, 2 replies; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 18:43 UTC (permalink / raw) To: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers <ebiggers@kernel.org> wrote: > > Hi Linus, please consider applying this patch. It's been ignored by the > keyrings maintainer for a month and a half with multiple reminders. It > fixes an easily reachable stack corruption in the new keyctl operations > that were added in v4.20. It was immediately reached by syzbot even > without any definitions for the new keyctls yet. The getoptions() code in security/keys/trusted.c has exactly the same buggy pattern, and seems to actually be the source of that idiocy. Mind fixing that one too and getting rid of this incorrect code entirely? Also, maybe the right fix is to do the "check for duplicate tokens" only *after* all the other validation (ie after the switch())? Or maybe just remove it entirely, since it's clearly entirely incorrect from the very start. Finally, the code was actually originally introduced in commit 5208cc83423d ("keys, trusted: fix: *do not* allow duplicate key options"), this second place you found is just pattern matching from that original garbage, that was acked and "reviewed" by several people. The fix should have that clarification. Commit 00d60fd3b932 wasn't the _origin_ of this bug, even if it made a copy of it. Looking around, nobody else has picked up that incorrect pattern. Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 18:43 ` Linus Torvalds @ 2018-12-17 18:49 ` Linus Torvalds 2018-12-17 19:06 ` Linus Torvalds 2018-12-18 12:34 ` Dmitry Vyukov 1 sibling, 1 reply; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 18:49 UTC (permalink / raw) To: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 10:43 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Or maybe just remove it entirely, since it's clearly entirely > incorrect from the very start. .. or another alternative: remove the "Opt_err = -1" entirely. Again, this seems to be a buggy pattern exclusive to the security code. Nobody else does it, and using that negative value is basically the source of those two bugs. It seems pointless and wrong. So the *simplest* fix would seem to be to literally remove all those "= -1" for the Opt_err initialization. Making the code smaller, simpler, and fixing the bug in the process. Hmm? Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 18:49 ` Linus Torvalds @ 2018-12-17 19:06 ` Linus Torvalds 2018-12-17 19:39 ` Linus Torvalds 2018-12-17 20:21 ` Mimi Zohar 0 siblings, 2 replies; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 19:06 UTC (permalink / raw) To: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 10:49 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So the *simplest* fix would seem to be to literally remove all those > "= -1" for the Opt_err initialization. Making the code smaller, > simpler, and fixing the bug in the process. Something like this git grep -l 'Opt_err = -1' | xargs sed -i 's/Opt_err = -1/Opt_err/' would do it, but I also notice that security/integrity/ima/ima_policy.c then has some truly funky code that plays around with the enum numbers , ie #define pt(token) policy_tokens[token + Opt_err].pattern which actually depends on the ordering of policy_tokens[], and depends on the exact values, ie it literally (and quite insanely) sets Opt_err to -1, and then Opt_measure to 1, and leaves 0 unused. That code seriously makes no sense at all, and is fundamentally broken. It would be better to use #define pt(token) policy_tokens[token - Opt_measure].pattern instead, but even then you should have a huge comment about how the policy_tokens[] array absolutely has to be properly ordered. Honestly, for being about "security", all of this code seems to be doing some really questionable things with all those Opt_xyz enums. Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 19:06 ` Linus Torvalds @ 2018-12-17 19:39 ` Linus Torvalds 2018-12-17 19:51 ` James Bottomley 2018-12-17 20:21 ` Mimi Zohar 1 sibling, 1 reply; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 19:39 UTC (permalink / raw) To: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs [-- Attachment #1: Type: text/plain, Size: 1204 bytes --] On Mon, Dec 17, 2018 at 11:06 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Honestly, for being about "security", all of this code seems to be > doing some really questionable things with all those Opt_xyz enums. Yeah, at least security/keys/trusted.c ends up mixing that enum and just using "int" completely randomly, and you have datablob_parse() returning either a negative integer _or_ an "Opt_xyz" value, so having Opt_err be -1 is doubly confusing there (it would also be "-EPERM" depending on how you treat it). There doesn't seem to be any _actual_ confusion (because Opt_err is always turned into an actual real error code), but it's just another sign of "those enums should not be negative". So on the whole, I think that the "Opt_err = -1" is a serious mistake, but at least for now, ima_policy.c clearly has (bogus) code that relies on it. But the two cases that use "test_and_set_bit()" do not seem to have any reason to use that -1 enum, so while we can't do the "just remove -1" in general, I do think the proper fix is to just do it for those two files. Eric, mind testing a patch like that? Untested patch attached just for completeness.. Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 938 bytes --] security/keys/keyctl_pkey.c | 2 +- security/keys/trusted.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c index 783978842f13..70e65a2ff207 100644 --- a/security/keys/keyctl_pkey.c +++ b/security/keys/keyctl_pkey.c @@ -25,7 +25,7 @@ static void keyctl_pkey_params_free(struct kernel_pkey_params *params) } enum { - Opt_err = -1, + Opt_err, Opt_enc, /* "enc=<encoding>" eg. "enc=oaep" */ Opt_hash, /* "hash=<digest-name>" eg. "hash=sha1" */ }; diff --git a/security/keys/trusted.c b/security/keys/trusted.c index ff6789365a12..697bfc6c8192 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -711,7 +711,7 @@ static int key_unseal(struct trusted_key_payload *p, } enum { - Opt_err = -1, + Opt_err, Opt_new, Opt_load, Opt_update, Opt_keyhandle, Opt_keyauth, Opt_blobauth, Opt_pcrinfo, Opt_pcrlock, Opt_migratable, ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 19:39 ` Linus Torvalds @ 2018-12-17 19:51 ` James Bottomley 2018-12-17 20:02 ` Linus Torvalds 0 siblings, 1 reply; 18+ messages in thread From: James Bottomley @ 2018-12-17 19:51 UTC (permalink / raw) To: Linus Torvalds, ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, 2018-12-17 at 11:39 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:06 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Honestly, for being about "security", all of this code seems to be > > doing some really questionable things with all those Opt_xyz enums. > > Yeah, at least security/keys/trusted.c ends up mixing that enum and > just using "int" completely randomly, and you have datablob_parse() > returning either a negative integer _or_ an "Opt_xyz" value, so > having Opt_err be -1 is doubly confusing there (it would also be "- > EPERM" depending on how you treat it). > > There doesn't seem to be any _actual_ confusion (because Opt_err is > always turned into an actual real error code), but it's just another > sign of "those enums should not be negative". > > So on the whole, I think that the "Opt_err = -1" is a serious > mistake, but at least for now, ima_policy.c clearly has (bogus) code > that relies on it. > > But the two cases that use "test_and_set_bit()" do not seem to have > any reason to use that -1 enum, so while we can't do the "just remove > -1" in general, I do think the proper fix is to just do it for those > two files. > > Eric, mind testing a patch like that? Untested patch attached just > for completeness.. If this is to replace Eric's patch, didn't you want to set token_mask to (1<<Opt_err)? James ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 19:51 ` James Bottomley @ 2018-12-17 20:02 ` Linus Torvalds 2018-12-17 20:29 ` Mimi Zohar ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 20:02 UTC (permalink / raw) To: James Bottomley Cc: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 11:51 AM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > If this is to replace Eric's patch, didn't you want to set token_mask > to (1<<Opt_err)? No, let's not add any extra code that is trying to be subtle. Subtle interactions was where the bug came from. The code already checks the actual Opt_xyz for errors in a switch statement. The token_mask should be _purely_ about duplicate options (or conflicting ones). Talking about the conflicting ones: Opt_hash checks that Opt_policydigest isn't set. But Opt_policydigest doesn't check that Opt_hash isn't set, so you can mix the two if you just do it in the right order. But that's a separate bug, and doesn't seem to be a huge deal. But it *is* an example of how bogus all of this stuff is. Clearly people weren't really paying attention when writing any of this code. Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 20:02 ` Linus Torvalds @ 2018-12-17 20:29 ` Mimi Zohar 2018-12-18 0:44 ` James Bottomley 2018-12-31 22:45 ` Eric Biggers 2 siblings, 0 replies; 18+ messages in thread From: Mimi Zohar @ 2018-12-17 20:29 UTC (permalink / raw) To: Linus Torvalds, James Bottomley Cc: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, 2018-12-17 at 12:02 -0800, Linus Torvalds wrote: > Talking about the conflicting ones: Opt_hash checks that > Opt_policydigest isn't set. But Opt_policydigest doesn't check that > Opt_hash isn't set, so you can mix the two if you just do it in the > right order. > > But that's a separate bug, and doesn't seem to be a huge deal. > > But it *is* an example of how bogus all of this stuff is. Clearly > people weren't really paying attention when writing any of this code. A file signature is more restrictive than just a file hash. I'll clean up this and the other ugliness. Mimi ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 20:02 ` Linus Torvalds 2018-12-17 20:29 ` Mimi Zohar @ 2018-12-18 0:44 ` James Bottomley 2018-12-31 22:45 ` Eric Biggers 2 siblings, 0 replies; 18+ messages in thread From: James Bottomley @ 2018-12-18 0:44 UTC (permalink / raw) To: Linus Torvalds Cc: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, 2018-12-17 at 12:02 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:51 AM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > If this is to replace Eric's patch, didn't you want to set > > token_mask to (1<<Opt_err)? > > No, let's not add any extra code that is trying to be subtle. Subtle > interactions was where the bug came from. Sure; I suppose liking the TPM gives me a taste for subtlety. > The code already checks the actual Opt_xyz for errors in a switch > statement. The token_mask should be _purely_ about duplicate options > (or conflicting ones). > > Talking about the conflicting ones: Opt_hash checks that > Opt_policydigest isn't set. But Opt_policydigest doesn't check that > Opt_hash isn't set, so you can mix the two if you just do it in the > right order. > > But that's a separate bug, and doesn't seem to be a huge deal. Actually, I'm afraid it's not a bug, it's a command line ordering thing. To check the policy digest length, we need to know which hash algorithm you're using, so if you're specifying a hash algorithm, it has to appear *before* a policydigest as a command input. I take it this is another subtlety you'd like documenting ...? > But it *is* an example of how bogus all of this stuff is. Clearly > people weren't really paying attention when writing any of this code. Hey, not going to argue here. The whole policy session passed in is questionable because some of the actions the kernel takes on the key have to depend on what was in the policy (which you don't know and can't deduce from the hash). The only way to get it to work universally is to pass the actual policy in and have the kernel construct the policy session itself. Fortunately fixing it can be low priority because we don't seem to have any users. James ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 20:02 ` Linus Torvalds 2018-12-17 20:29 ` Mimi Zohar 2018-12-18 0:44 ` James Bottomley @ 2018-12-31 22:45 ` Eric Biggers 2019-01-01 21:08 ` Linus Torvalds 2 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2018-12-31 22:45 UTC (permalink / raw) To: David Howells, Linus Torvalds Cc: James Bottomley, James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, keyrings, Linux List Kernel Mailing, syzkaller-bugs Hi David and Linus, On Mon, Dec 17, 2018 at 12:02:01PM -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 11:51 AM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > If this is to replace Eric's patch, didn't you want to set token_mask > > to (1<<Opt_err)? > > No, let's not add any extra code that is trying to be subtle. Subtle > interactions was where the bug came from. > > The code already checks the actual Opt_xyz for errors in a switch > statement. The token_mask should be _purely_ about duplicate options > (or conflicting ones). > > Talking about the conflicting ones: Opt_hash checks that > Opt_policydigest isn't set. But Opt_policydigest doesn't check that > Opt_hash isn't set, so you can mix the two if you just do it in the > right order. > > But that's a separate bug, and doesn't seem to be a huge deal. > > But it *is* an example of how bogus all of this stuff is. Clearly > people weren't really paying attention when writing any of this code. > > Linus KEYCTL_PKEY_QUERY is still failing basic fuzzing even after Linus' fix that changed Opt_err from -1 to 0. The crash is still in keyctl_pkey_params_parse(): token = match_token(p, param_keys, args); if (__test_and_set_bit(token, &token_mask)) return -EINVAL; q = args[0].from; if (!q[0]) return -EINVAL; Now it crashes on '!q[0]' because 'args[0].from' is uninitialized when token == Opt_err. args[0] is only initialized when the parsed token had a pattern that set it. David, where are the tests for these new keyctls? - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-31 22:45 ` Eric Biggers @ 2019-01-01 21:08 ` Linus Torvalds 0 siblings, 0 replies; 18+ messages in thread From: Linus Torvalds @ 2019-01-01 21:08 UTC (permalink / raw) To: Eric Biggers Cc: David Howells, James Bottomley, James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 31, 2018 at 2:45 PM Eric Biggers <ebiggers@kernel.org> wrote: > > KEYCTL_PKEY_QUERY is still failing basic fuzzing even after Linus' fix that > changed Opt_err from -1 to 0. The crash is still in keyctl_pkey_params_parse(): > > token = match_token(p, param_keys, args); > if (__test_and_set_bit(token, &token_mask)) > return -EINVAL; > q = args[0].from; > if (!q[0]) > return -EINVAL; > > Now it crashes on '!q[0]' because 'args[0].from' is uninitialized when > token == Opt_err. args[0] is only initialized when the parsed token had a > pattern that set it. Argh., how embarrassing. And it turns out that James' suggestion to initialize token_mask would actually have fixed that, for subtle reasons (but subtle was what I didn't want). I detest that match_token() interface, but this key code then mis-uses it in ways it wasn't even meant for, and tries to "share" error paths that aren't actually common. I'll take your original patch, which I clearly should have done originally. Thanks, and sorry for the wasted time, Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 19:06 ` Linus Torvalds 2018-12-17 19:39 ` Linus Torvalds @ 2018-12-17 20:21 ` Mimi Zohar 2018-12-17 20:31 ` Linus Torvalds 1 sibling, 1 reply; 18+ messages in thread From: Mimi Zohar @ 2018-12-17 20:21 UTC (permalink / raw) To: Linus Torvalds, ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe Cc: David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, 2018-12-17 at 11:06 -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 10:49 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > So the *simplest* fix would seem to be to literally remove all those > > "= -1" for the Opt_err initialization. Making the code smaller, > > simpler, and fixing the bug in the process. > > Something like this > > git grep -l 'Opt_err = -1' | xargs sed -i 's/Opt_err = -1/Opt_err/' > > would do it, but I also notice that > security/integrity/ima/ima_policy.c then has some truly funky code > that plays around with the enum numbers , ie > > #define pt(token) policy_tokens[token + Opt_err].pattern Yikes! > which actually depends on the ordering of policy_tokens[], and depends > on the exact values, ie it literally (and quite insanely) sets Opt_err > to -1, and then Opt_measure to 1, and leaves 0 unused. That code > seriously makes no sense at all, and is fundamentally broken. > > It would be better to use > > #define pt(token) policy_tokens[token - Opt_measure].pattern > > instead, but even then you should have a huge comment about how the > policy_tokens[] array absolutely has to be properly ordered. Will do. > > Honestly, for being about "security", all of this code seems to be > doing some really questionable things with all those Opt_xyz enums. It's being used for parsing and displaying the policy, which do need to be in sync. Mimi ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 20:21 ` Mimi Zohar @ 2018-12-17 20:31 ` Linus Torvalds 0 siblings, 0 replies; 18+ messages in thread From: Linus Torvalds @ 2018-12-17 20:31 UTC (permalink / raw) To: zohar Cc: ebiggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 12:21 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > It's being used for parsing and displaying the policy, which do need > to be in sync. Yes, but it needs a comment somewhere. Also, the way you use those enums as array indices also implies that for your case, Opt_err should definitely not be -1, and it should instead be at the *end* of the enum list (the same way it's at the end of the array). That would also automatically mean that "Opt_measure" would have value 0, and the pl(token) macro shouldn't need any offsetting at all, because the enums and the array indices just automatically match up (as long as they are always updated together!) Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH RESEND] KEYS: fix parsing invalid pkey info string 2018-12-17 18:43 ` Linus Torvalds 2018-12-17 18:49 ` Linus Torvalds @ 2018-12-18 12:34 ` Dmitry Vyukov 1 sibling, 0 replies; 18+ messages in thread From: Dmitry Vyukov @ 2018-12-18 12:34 UTC (permalink / raw) To: Linus Torvalds Cc: Eric Biggers, James Morris James Morris, Mimi Zohar, Jarkko Sakkinen, Peter Huewe, David Howells, keyrings, Linux List Kernel Mailing, syzkaller-bugs On Mon, Dec 17, 2018 at 7:43 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > Hi Linus, please consider applying this patch. It's been ignored by the > > keyrings maintainer for a month and a half with multiple reminders. It > > fixes an easily reachable stack corruption in the new keyctl operations > > that were added in v4.20. It was immediately reached by syzbot even > > without any definitions for the new keyctls yet. > > The getoptions() code in security/keys/trusted.c has exactly the same > buggy pattern, and seems to actually be the source of that idiocy. > > Mind fixing that one too and getting rid of this incorrect code entirely? > > Also, maybe the right fix is to do the "check for duplicate tokens" > only *after* all the other validation (ie after the switch())? > > Or maybe just remove it entirely, since it's clearly entirely > incorrect from the very start. > > Finally, the code was actually originally introduced in commit > 5208cc83423d ("keys, trusted: fix: *do not* allow duplicate key > options"), this second place you found is just pattern matching from > that original garbage, that was acked and "reviewed" by several > people. ... also acked by 0 tests added by that commit. > The fix should have that clarification. Commit 00d60fd3b932 > wasn't the _origin_ of this bug, even if it made a copy of it. > > Looking around, nobody else has picked up that incorrect pattern. > > Linus > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAHk-%3Dwhmh8WdcKHdYjioJNjyeewv%3DfO1H0hxXqDh6kiX0YvzmQ%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-01-01 21:09 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-03 15:48 general protection fault in keyctl_pkey_params_get syzbot 2018-11-03 17:30 ` [PATCH] KEYS: fix parsing invalid pkey info string Eric Biggers 2018-11-28 23:20 ` Eric Biggers 2018-12-06 18:26 ` Eric Biggers 2018-12-17 18:12 ` [PATCH RESEND] " Eric Biggers 2018-12-17 18:43 ` Linus Torvalds 2018-12-17 18:49 ` Linus Torvalds 2018-12-17 19:06 ` Linus Torvalds 2018-12-17 19:39 ` Linus Torvalds 2018-12-17 19:51 ` James Bottomley 2018-12-17 20:02 ` Linus Torvalds 2018-12-17 20:29 ` Mimi Zohar 2018-12-18 0:44 ` James Bottomley 2018-12-31 22:45 ` Eric Biggers 2019-01-01 21:08 ` Linus Torvalds 2018-12-17 20:21 ` Mimi Zohar 2018-12-17 20:31 ` Linus Torvalds 2018-12-18 12:34 ` Dmitry Vyukov
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).