Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> Do you mean in addition or instead of the previous one? From your
> description, it sounds like it alone should prevent the crash.

I'm going to submit them both, so if you could test them together.  You're
right, though, I think this should also prevent the crash.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Mon, Oct 19, 2015 at 12:33 PM, David Howells  wrote:
> Dmitry Vyukov  wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> I have an additional patch to prevent keyrings from being constructed by
> request_key() at all (though it can still search for them).  Could you give
> this a spin in addition to the previous one also?

Do you mean in addition or instead of the previous one? From your
description, it sounds like it alone should prevent the crash.


> Thanks,
> David
> ---
> commit 27874345bb8d2c39f3d493607a86ecbfcb100405
> Author: David Howells 
> Date:   Mon Oct 19 11:20:28 2015 +0100
>
> KEYS: Don't permit request_key() to construct a new keyring
>
> If request_key() is used to find a keyring, only do the search part - 
> don't
> do the construction part if the keyring was not found by the search.  We
> don't really want keyrings in the negative instantiated state since the
> rejected/negative instantiation error value in the payload is unioned with
> keyring metadata.
>
> Signed-off-by: David Howells 
>
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 486ef6fa393b..0d6253124278 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct 
> keyring_search_context *ctx,
>
> kenter("");
>
> +   if (ctx->index_key.type == _type_keyring)
> +   return ERR_PTR(-EPERM);
> +
> user = key_user_lookup(current_fsuid());
> if (!user)
> return ERR_PTR(-ENOMEM);
>
> --
> You received this message because you are subscribed to the Google Groups 
> "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to syzkaller+unsubscr...@googlegroups.com.
> To post to this group, send email to syzkal...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/syzkaller/17443.1445250818%40warthog.procyon.org.uk.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

I have an additional patch to prevent keyrings from being constructed by
request_key() at all (though it can still search for them).  Could you give
this a spin in addition to the previous one also?

Thanks,
David
---
commit 27874345bb8d2c39f3d493607a86ecbfcb100405
Author: David Howells 
Date:   Mon Oct 19 11:20:28 2015 +0100

KEYS: Don't permit request_key() to construct a new keyring

If request_key() is used to find a keyring, only do the search part - don't
do the construction part if the keyring was not found by the search.  We
don't really want keyrings in the negative instantiated state since the
rejected/negative instantiation error value in the payload is unioned with
keyring metadata.

Signed-off-by: David Howells 

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 486ef6fa393b..0d6253124278 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct 
keyring_search_context *ctx,
 
kenter("");
 
+   if (ctx->index_key.type == _type_keyring)
+   return ERR_PTR(-EPERM);
+   
user = key_user_lookup(current_fsuid());
if (!user)
return ERR_PTR(-ENOMEM);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> Yes, sure. Do I need to say something like:
> 
> Tested-by: Dmitry Vyukov 
> 
> in future?

That helps:-)

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Mon, Oct 19, 2015 at 11:30 AM, David Howells  wrote:
> Dmitry Vyukov  wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> Can I put you down as a Tested-by?
>
> David


Yes, sure. Do I need to say something like:

Tested-by: Dmitry Vyukov 

in future?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

Can I put you down as a Tested-by?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Thu, Oct 15, 2015 at 9:21 PM, David Howells  wrote:
> Does the attached patch fix it for you?

Yes, it fixes the crash for me.


> David
> ---
> commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
> Author: David Howells 
> Date:   Thu Oct 15 17:21:37 2015 +0100
>
> KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
>
> The following sequence of commands:
>
> i=`keyctl add user a a @s`
> keyctl request2 keyring foo bar @t
> keyctl unlink $i @s
>
> tries to invoke an upcall to instantiate a keyring if one doesn't already
> exist by that name within the user's keyring set.  However, if the upcall
> fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
> other error code.  When the key is garbage collected, the key destroy
> function is called unconditionally and keyring_destroy() uses list_empty()
> on keyring->type_data.link - which is in a union with reject_error.
> Subsequently, the kernel tries to unlink the keyring from the keyring 
> names
> list - which oopses like this:
>
> BUG: unable to handle kernel paging request at ff8a
> IP: [] keyring_destroy+0x3d/0x88
> ...
> Workqueue: events key_garbage_collector
> ...
> RIP: 0010:[] keyring_destroy+0x3d/0x88
> RSP: 0018:88003e2f3d30  EFLAGS: 00010203
> RAX: ff82 RBX: 88003bf1a900 RCX: 
> RDX:  RSI: 3bfc6901 RDI: 81a73a40
> RBP: 88003e2f3d38 R08: 0152 R09: 
> R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
> R13:  R14: 88003bf1a908 R15: 88003e2f4000
> ...
> CR2: ff8a CR3: 3e3ec000 CR4: 06f0
> ...
> Call Trace:
>  [] key_gc_unused_keys.constprop.1+0x5d/0x10f
>  [] key_garbage_collector+0x1fa/0x351
>  [] process_one_work+0x28e/0x547
>  [] worker_thread+0x26e/0x361
>  [] ? rescuer_thread+0x2a8/0x2a8
>  [] kthread+0xf3/0xfb
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>  [] ret_from_fork+0x3f/0x70
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>
> Note the value in RAX.  This is a 32-bit representation of -ENOKEY.
>
> The solution is to only call ->destroy() if the key was successfully
> instantiated.
>
> Reported-by: Dmitry Vyukov 
> Signed-off-by: David Howells 
>
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 39eac1fd5706..addf060399e0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
> *keys)
> kdebug("- %u", key->serial);
> key_check(key);
>
> -   /* Throw away the key data */
> -   if (key->type->destroy)
> +   /* Throw away the key data if the key is instantiated */
> +   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
> +   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
> +   key->type->destroy)
> key->type->destroy(key);
>
> security_key_free(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> Do you mean in addition or instead of the previous one? From your
> description, it sounds like it alone should prevent the crash.

I'm going to submit them both, so if you could test them together.  You're
right, though, I think this should also prevent the crash.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

Can I put you down as a Tested-by?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

I have an additional patch to prevent keyrings from being constructed by
request_key() at all (though it can still search for them).  Could you give
this a spin in addition to the previous one also?

Thanks,
David
---
commit 27874345bb8d2c39f3d493607a86ecbfcb100405
Author: David Howells 
Date:   Mon Oct 19 11:20:28 2015 +0100

KEYS: Don't permit request_key() to construct a new keyring

If request_key() is used to find a keyring, only do the search part - don't
do the construction part if the keyring was not found by the search.  We
don't really want keyrings in the negative instantiated state since the
rejected/negative instantiation error value in the payload is unioned with
keyring metadata.

Signed-off-by: David Howells 

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 486ef6fa393b..0d6253124278 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct 
keyring_search_context *ctx,
 
kenter("");
 
+   if (ctx->index_key.type == _type_keyring)
+   return ERR_PTR(-EPERM);
+   
user = key_user_lookup(current_fsuid());
if (!user)
return ERR_PTR(-ENOMEM);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread David Howells 
Dmitry Vyukov  wrote:

> Yes, sure. Do I need to say something like:
> 
> Tested-by: Dmitry Vyukov 
> 
> in future?

That helps:-)

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Mon, Oct 19, 2015 at 12:33 PM, David Howells  wrote:
> Dmitry Vyukov  wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> I have an additional patch to prevent keyrings from being constructed by
> request_key() at all (though it can still search for them).  Could you give
> this a spin in addition to the previous one also?

Do you mean in addition or instead of the previous one? From your
description, it sounds like it alone should prevent the crash.


> Thanks,
> David
> ---
> commit 27874345bb8d2c39f3d493607a86ecbfcb100405
> Author: David Howells 
> Date:   Mon Oct 19 11:20:28 2015 +0100
>
> KEYS: Don't permit request_key() to construct a new keyring
>
> If request_key() is used to find a keyring, only do the search part - 
> don't
> do the construction part if the keyring was not found by the search.  We
> don't really want keyrings in the negative instantiated state since the
> rejected/negative instantiation error value in the payload is unioned with
> keyring metadata.
>
> Signed-off-by: David Howells 
>
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 486ef6fa393b..0d6253124278 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct 
> keyring_search_context *ctx,
>
> kenter("");
>
> +   if (ctx->index_key.type == _type_keyring)
> +   return ERR_PTR(-EPERM);
> +
> user = key_user_lookup(current_fsuid());
> if (!user)
> return ERR_PTR(-ENOMEM);
>
> --
> You received this message because you are subscribed to the Google Groups 
> "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to syzkaller+unsubscr...@googlegroups.com.
> To post to this group, send email to syzkal...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/syzkaller/17443.1445250818%40warthog.procyon.org.uk.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Thu, Oct 15, 2015 at 9:21 PM, David Howells  wrote:
> Does the attached patch fix it for you?

Yes, it fixes the crash for me.


> David
> ---
> commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
> Author: David Howells 
> Date:   Thu Oct 15 17:21:37 2015 +0100
>
> KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
>
> The following sequence of commands:
>
> i=`keyctl add user a a @s`
> keyctl request2 keyring foo bar @t
> keyctl unlink $i @s
>
> tries to invoke an upcall to instantiate a keyring if one doesn't already
> exist by that name within the user's keyring set.  However, if the upcall
> fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
> other error code.  When the key is garbage collected, the key destroy
> function is called unconditionally and keyring_destroy() uses list_empty()
> on keyring->type_data.link - which is in a union with reject_error.
> Subsequently, the kernel tries to unlink the keyring from the keyring 
> names
> list - which oopses like this:
>
> BUG: unable to handle kernel paging request at ff8a
> IP: [] keyring_destroy+0x3d/0x88
> ...
> Workqueue: events key_garbage_collector
> ...
> RIP: 0010:[] keyring_destroy+0x3d/0x88
> RSP: 0018:88003e2f3d30  EFLAGS: 00010203
> RAX: ff82 RBX: 88003bf1a900 RCX: 
> RDX:  RSI: 3bfc6901 RDI: 81a73a40
> RBP: 88003e2f3d38 R08: 0152 R09: 
> R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
> R13:  R14: 88003bf1a908 R15: 88003e2f4000
> ...
> CR2: ff8a CR3: 3e3ec000 CR4: 06f0
> ...
> Call Trace:
>  [] key_gc_unused_keys.constprop.1+0x5d/0x10f
>  [] key_garbage_collector+0x1fa/0x351
>  [] process_one_work+0x28e/0x547
>  [] worker_thread+0x26e/0x361
>  [] ? rescuer_thread+0x2a8/0x2a8
>  [] kthread+0xf3/0xfb
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>  [] ret_from_fork+0x3f/0x70
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>
> Note the value in RAX.  This is a 32-bit representation of -ENOKEY.
>
> The solution is to only call ->destroy() if the key was successfully
> instantiated.
>
> Reported-by: Dmitry Vyukov 
> Signed-off-by: David Howells 
>
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 39eac1fd5706..addf060399e0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
> *keys)
> kdebug("- %u", key->serial);
> key_check(key);
>
> -   /* Throw away the key data */
> -   if (key->type->destroy)
> +   /* Throw away the key data if the key is instantiated */
> +   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
> +   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
> +   key->type->destroy)
> key->type->destroy(key);
>
> security_key_free(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-19 Thread Dmitry Vyukov 
On Mon, Oct 19, 2015 at 11:30 AM, David Howells  wrote:
> Dmitry Vyukov  wrote:
>
>> > Does the attached patch fix it for you?
>>
>> Yes, it fixes the crash for me.
>
> Can I put you down as a Tested-by?
>
> David


Yes, sure. Do I need to say something like:

Tested-by: Dmitry Vyukov 

in future?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-15 Thread David Howells 
Does the attached patch fix it for you?

David
---
commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
Author: David Howells 
Date:   Thu Oct 15 17:21:37 2015 +0100

KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring

The following sequence of commands:

i=`keyctl add user a a @s`
keyctl request2 keyring foo bar @t
keyctl unlink $i @s

tries to invoke an upcall to instantiate a keyring if one doesn't already
exist by that name within the user's keyring set.  However, if the upcall
fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
other error code.  When the key is garbage collected, the key destroy
function is called unconditionally and keyring_destroy() uses list_empty()
on keyring->type_data.link - which is in a union with reject_error.
Subsequently, the kernel tries to unlink the keyring from the keyring names
list - which oopses like this:

BUG: unable to handle kernel paging request at ff8a
IP: [] keyring_destroy+0x3d/0x88
...
Workqueue: events key_garbage_collector
...
RIP: 0010:[] keyring_destroy+0x3d/0x88
RSP: 0018:88003e2f3d30  EFLAGS: 00010203
RAX: ff82 RBX: 88003bf1a900 RCX: 
RDX:  RSI: 3bfc6901 RDI: 81a73a40
RBP: 88003e2f3d38 R08: 0152 R09: 
R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
R13:  R14: 88003bf1a908 R15: 88003e2f4000
...
CR2: ff8a CR3: 3e3ec000 CR4: 06f0
...
Call Trace:
 [] key_gc_unused_keys.constprop.1+0x5d/0x10f
 [] key_garbage_collector+0x1fa/0x351
 [] process_one_work+0x28e/0x547
 [] worker_thread+0x26e/0x361
 [] ? rescuer_thread+0x2a8/0x2a8
 [] kthread+0xf3/0xfb
 [] ? kthread_create_on_node+0x1c2/0x1c2
 [] ret_from_fork+0x3f/0x70
 [] ? kthread_create_on_node+0x1c2/0x1c2

Note the value in RAX.  This is a 32-bit representation of -ENOKEY.

The solution is to only call ->destroy() if the key was successfully
instantiated.

Reported-by: Dmitry Vyukov 
Signed-off-by: David Howells 

diff --git a/security/keys/gc.c b/security/keys/gc.c
index 39eac1fd5706..addf060399e0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
*keys)
kdebug("- %u", key->serial);
key_check(key);
 
-   /* Throw away the key data */
-   if (key->type->destroy)
+   /* Throw away the key data if the key is instantiated */
+   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
+   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
+   key->type->destroy)
key->type->destroy(key);
 
security_key_free(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-15 Thread David Howells 
Dmitry Vyukov  wrote:

> RAX: ff82

This is the value that matters.  It would appear to be -ENOKEY and would be in
key->type_data.reject_error, I think.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-15 Thread David Howells 
Dmitry Vyukov  wrote:

> RAX: ff82

This is the value that matters.  It would appear to be -ENOKEY and would be in
key->type_data.reject_error, I think.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: GPF in keyring_destroy

2015-10-15 Thread David Howells 
Does the attached patch fix it for you?

David
---
commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
Author: David Howells 
Date:   Thu Oct 15 17:21:37 2015 +0100

KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring

The following sequence of commands:

i=`keyctl add user a a @s`
keyctl request2 keyring foo bar @t
keyctl unlink $i @s

tries to invoke an upcall to instantiate a keyring if one doesn't already
exist by that name within the user's keyring set.  However, if the upcall
fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
other error code.  When the key is garbage collected, the key destroy
function is called unconditionally and keyring_destroy() uses list_empty()
on keyring->type_data.link - which is in a union with reject_error.
Subsequently, the kernel tries to unlink the keyring from the keyring names
list - which oopses like this:

BUG: unable to handle kernel paging request at ff8a
IP: [] keyring_destroy+0x3d/0x88
...
Workqueue: events key_garbage_collector
...
RIP: 0010:[] keyring_destroy+0x3d/0x88
RSP: 0018:88003e2f3d30  EFLAGS: 00010203
RAX: ff82 RBX: 88003bf1a900 RCX: 
RDX:  RSI: 3bfc6901 RDI: 81a73a40
RBP: 88003e2f3d38 R08: 0152 R09: 
R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
R13:  R14: 88003bf1a908 R15: 88003e2f4000
...
CR2: ff8a CR3: 3e3ec000 CR4: 06f0
...
Call Trace:
 [] key_gc_unused_keys.constprop.1+0x5d/0x10f
 [] key_garbage_collector+0x1fa/0x351
 [] process_one_work+0x28e/0x547
 [] worker_thread+0x26e/0x361
 [] ? rescuer_thread+0x2a8/0x2a8
 [] kthread+0xf3/0xfb
 [] ? kthread_create_on_node+0x1c2/0x1c2
 [] ret_from_fork+0x3f/0x70
 [] ? kthread_create_on_node+0x1c2/0x1c2

Note the value in RAX.  This is a 32-bit representation of -ENOKEY.

The solution is to only call ->destroy() if the key was successfully
instantiated.

Reported-by: Dmitry Vyukov 
Signed-off-by: David Howells 

diff --git a/security/keys/gc.c b/security/keys/gc.c
index 39eac1fd5706..addf060399e0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
*keys)
kdebug("- %u", key->serial);
key_check(key);
 
-   /* Throw away the key data */
-   if (key->type->destroy)
+   /* Throw away the key data if the key is instantiated */
+   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
+   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
+   key->type->destroy)
key->type->destroy(key);
 
security_key_free(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

GPF in keyring_destroy

2015-10-12 Thread Dmitry Vyukov 
Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
long r0 = syscall(SYS_mmap, 0x20001000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
long r1 = syscall(SYS_mmap, 0x2000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
*(uint64_t*)0x2131 = 0x947;
*(uint64_t*)0x2139 = 0xa9be;
*(uint64_t*)0x2141 = 0x4;
*(uint64_t*)0x2149 = 0xfff8;
*(uint64_t*)0x2151 = 0x3;
*(uint64_t*)0x2159 = 0x1;
*(uint64_t*)0x2161 = 0x7;
*(uint64_t*)0x2169 = 0x3b;
long r11 = syscall(SYS_mmap, 0x20002000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r16 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r20 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r24 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
return 0;
}

To reproduce run the program several times, then wait a minute, run
again, repeat until crashes (the crash happens in a background thread
during deferred garbage collection).

Found with syzkaller fuzzer.

On commit c6fa8e6de3dc420cba092bf155b2ed25bcd537f7
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)


BUG: unable to handle kernel paging request at ff8a
IP: [< inline >] list_del include/linux/list.h:107
IP: [] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
PGD 0
Oops: 0002 [#1] SMP KASAN
Modules linked in:
CPU: 2 PID: 505 Comm: kworker/2:1 Not tainted 4.3.0-rc4+ #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: events key_garbage_collector
task: 88006dd93400 ti: 88006dd68000 task.ti: 88006dd68000
RIP: 0010:[]  [] keyring_destroy+0x3d/0x80
RSP: 0018:88006dd6fda8  EFLAGS: 00010213
RAX: ff82 RBX: 88006d73bc00 RCX: 
RDX:  RSI: 88006dd93400 RDI: 821077a0
RBP: 88006dd6fdb0 R08: 88006dd68000 R09: 88006dd93d40
R10: 072c R11: 0001 R12: 88006d73bc00
R13: 7fff R14: 88006dd7 R15: 
FS:  () GS:88006e80() knlGS:
CS:  0010 DS:  ES:  CR0: 8005003b
CR2: ff8a CR3: 6cda6000 CR4: 06e0
Stack:
 88006d73bc08 88006dd6fdd0 81290131 
 56177c32 88006dd6fe18 81290391 88006e626d28
  81e812a0 88006e746080 88006e81d8c0
Call Trace:
 [] key_gc_unused_keys.constprop.1+0xa1/0xf0
security/keys/gc.c:139
 [] key_garbage_collector+0x1a1/0x360 security/keys/gc.c:294
 [] process_one_work+0x13e/0x3c0 kernel/workqueue.c:2030
 [] worker_thread+0x115/0x450 kernel/workqueue.c:2162
 [< inline >] ? context_switch kernel/sched/core.c:2651
 [] ? __schedule+0x311/0x7e0 kernel/sched/core.c:3115
 [] ? process_one_work+0x3c0/0x3c0
kernel/workqueue.c:1973 (discriminator 1)
 [] kthread+0xc4/0xe0 kernel/kthread.c:209
 [] ? kthread_park+0x50/0x50 kernel/kthread.c:448
 [] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:472
 [] ? kthread_park+0x50/0x50 kernel/kthread.c:448
Code: 48 c7 c7 a0 77 10 82 e8 f2 a8 5a 00 48 8b 83 98 00 00 00 48 85
c0 74 36 48 8d 93 98 00 00 00 48 39 d0 74 2a 48 8b 93 a0 00 00 00 <48>
89 50 08 48 89 02 48 b8 00 01 00 00 00 00 ad de 48 89 83 98
RIP  [< inline >] list_del include/linux/list.h:107
RIP  [] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
 RSP 
CR2: ff8a
---[ end

GPF in keyring_destroy

2015-10-12 Thread Dmitry Vyukov 
Hello,

The following program crashes kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include 
#include 
#include 

int main()
{
long r0 = syscall(SYS_mmap, 0x20001000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
long r1 = syscall(SYS_mmap, 0x2000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
*(uint64_t*)0x2131 = 0x947;
*(uint64_t*)0x2139 = 0xa9be;
*(uint64_t*)0x2141 = 0x4;
*(uint64_t*)0x2149 = 0xfff8;
*(uint64_t*)0x2151 = 0x3;
*(uint64_t*)0x2159 = 0x1;
*(uint64_t*)0x2161 = 0x7;
*(uint64_t*)0x2169 = 0x3b;
long r11 = syscall(SYS_mmap, 0x20002000ul, 0x1000ul, 0x3ul,
0x32ul, 0xul, 0x0ul);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r16 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r20 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
memcpy((void*)0x20002000,
"\x5d\x27\x7b\x65\x63\x72\x79\x70\x74\x66\x73\x5c\x73\x65\x6c\x69\x6e\x75\x78\x6e\x66\x73\x00",
23);
memcpy((void*)0x20001ff4,
"\x23\x73\x65\x6c\x69\x6e\x75\x78\x2c\x62\x64\x65\x76\x72\x61\x6d\x66\x73\xe6\x68\x66\x73\x70\x6c\x75\x73\x7b\x2c\x28\x72\x6f\x6f\x74\x66\x73\x00",
36);
memcpy((void*)0x20002000, "\x6b\x65\x79\x72\x69\x6e\x67\x00", 8);
long r24 = syscall(SYS_request_key, 0x20002000ul,
0x20001ff4ul, 0x20002000ul, 0xfffbul, 0, 0);
return 0;
}

To reproduce run the program several times, then wait a minute, run
again, repeat until crashes (the crash happens in a background thread
during deferred garbage collection).

Found with syzkaller fuzzer.

On commit c6fa8e6de3dc420cba092bf155b2ed25bcd537f7
(git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)


BUG: unable to handle kernel paging request at ff8a
IP: [< inline >] list_del include/linux/list.h:107
IP: [] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
PGD 0
Oops: 0002 [#1] SMP KASAN
Modules linked in:
CPU: 2 PID: 505 Comm: kworker/2:1 Not tainted 4.3.0-rc4+ #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: events key_garbage_collector
task: 88006dd93400 ti: 88006dd68000 task.ti: 88006dd68000
RIP: 0010:[]  [] keyring_destroy+0x3d/0x80
RSP: 0018:88006dd6fda8  EFLAGS: 00010213
RAX: ff82 RBX: 88006d73bc00 RCX: 
RDX:  RSI: 88006dd93400 RDI: 821077a0
RBP: 88006dd6fdb0 R08: 88006dd68000 R09: 88006dd93d40
R10: 072c R11: 0001 R12: 88006d73bc00
R13: 7fff R14: 88006dd7 R15: 
FS:  () GS:88006e80() knlGS:
CS:  0010 DS:  ES:  CR0: 8005003b
CR2: ff8a CR3: 6cda6000 CR4: 06e0
Stack:
 88006d73bc08 88006dd6fdd0 81290131 
 56177c32 88006dd6fe18 81290391 88006e626d28
  81e812a0 88006e746080 88006e81d8c0
Call Trace:
 [] key_gc_unused_keys.constprop.1+0xa1/0xf0
security/keys/gc.c:139
 [] key_garbage_collector+0x1a1/0x360 security/keys/gc.c:294
 [] process_one_work+0x13e/0x3c0 kernel/workqueue.c:2030
 [] worker_thread+0x115/0x450 kernel/workqueue.c:2162
 [< inline >] ? context_switch kernel/sched/core.c:2651
 [] ? __schedule+0x311/0x7e0 kernel/sched/core.c:3115
 [] ? process_one_work+0x3c0/0x3c0
kernel/workqueue.c:1973 (discriminator 1)
 [] kthread+0xc4/0xe0 kernel/kthread.c:209
 [] ? kthread_park+0x50/0x50 kernel/kthread.c:448
 [] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:472
 [] ? kthread_park+0x50/0x50 kernel/kthread.c:448
Code: 48 c7 c7 a0 77 10 82 e8 f2 a8 5a 00 48 8b 83 98 00 00 00 48 85
c0 74 36 48 8d 93 98 00 00 00 48 39 d0 74 2a 48 8b 93 a0 00 00 00 <48>
89 50 08 48 89 02 48 b8 00 01 00 00 00 00 ad de 48 89 83 98
RIP  [< inline >] list_del include/linux/list.h:107
RIP  [] keyring_destroy+0x3d/0x80 security/keys/keyring.c:392
 RSP 
CR2: ff8a
---[ end