general protection fault in keyctl_pkey_params_get

[syzbot]

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+a22e0d...@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 syzk...@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

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

We need to check the return value of match_token() for Opt_err (-1)
before doing anything with it.

Reported-by: syzbot+a22e0d...@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 <ebig...@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

[Eric Biggers]

Ping. David, are you planning to apply this?

- Eric

[Eric Biggers]

Ping.

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

We need to check the return value of match_token() for Opt_err (-1)
before doing anything with it.

Reported-by: syzbot+a22e0d...@syzkaller.appspotmail.com
Fixes: 00d60fd3b932 ("KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]")

Cc: David Howells <dhow...@redhat.com>

Signed-off-by: Eric Biggers <ebig...@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

[Linus Torvalds]

On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers <ebig...@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

[Linus Torvalds]

On Mon, Dec 17, 2018 at 10:43 AM Linus Torvalds
<torv...@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

[Linus Torvalds]

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

[Linus Torvalds]

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

[James Bottomley]

If this is to replace Eric's patch, didn't you want to set token_mask
to (1<<Opt_err)?

James

[Linus Torvalds]

On Mon, Dec 17, 2018 at 11:51 AM James Bottomley
<James.B...@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

[Mimi Zohar]

On Mon, 2018-12-17 at 11:06 -0800, Linus Torvalds wrote:
> On Mon, Dec 17, 2018 at 10:49 AM Linus Torvalds
> <torv...@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

[Mimi Zohar]

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

[Linus Torvalds]

On Mon, Dec 17, 2018 at 12:21 PM Mimi Zohar <zo...@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

[James Bottomley]

On Mon, 2018-12-17 at 12:02 -0800, Linus Torvalds wrote:

> On Mon, Dec 17, 2018 at 11:51 AM James Bottomley
> <James.B...@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

[Dmitry Vyukov]

On Mon, Dec 17, 2018 at 7:43 PM Linus Torvalds
<torv...@linux-foundation.org> wrote:
>
> On Mon, Dec 17, 2018 at 10:13 AM Eric Biggers <ebig...@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-bug...@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.

[Eric Biggers]

Hi David and 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

[Linus Torvalds]

On Mon, Dec 31, 2018 at 2:45 PM Eric Biggers <ebig...@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