Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1250448 > unrolled thread

Re: GPF in keyring_destroy

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: GPF in keyring_destroy Dmitry Vyukov <dvyukov@google.com> - 2015-10-19 10:30 +0200
    Re: GPF in keyring_destroy David Howells <dhowells@redhat.com> - 2015-10-19 11:40 +0200
      Re: GPF in keyring_destroy Dmitry Vyukov <dvyukov@google.com> - 2015-10-19 11:40 +0200
        Re: GPF in keyring_destroy David Howells <dhowells@redhat.com> - 2015-10-19 12:30 +0200
    Re: GPF in keyring_destroy David Howells <dhowells@redhat.com> - 2015-10-19 12:40 +0200
      Re: GPF in keyring_destroy Dmitry Vyukov <dvyukov@google.com> - 2015-10-19 12:50 +0200
        Re: GPF in keyring_destroy David Howells <dhowells@redhat.com> - 2015-10-19 13:00 +0200

#1250448 — Re: GPF in keyring_destroy

On Thu, Oct 15, 2015 at 9:21 PM, David Howells <dhowells@redhat.com> wrote:
> Does the attached patch fix it for you?

Yes, it fixes the crash for me.


> David
> ---
> commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
> Author: David Howells <dhowells@redhat.com>
> 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 00000000ffffff8a
>         IP: [<ffffffff8126e051>] keyring_destroy+0x3d/0x88
>         ...
>         Workqueue: events key_garbage_collector
>         ...
>         RIP: 0010:[<ffffffff8126e051>] keyring_destroy+0x3d/0x88
>         RSP: 0018:ffff88003e2f3d30  EFLAGS: 00010203
>         RAX: 00000000ffffff82 RBX: ffff88003bf1a900 RCX: 0000000000000000
>         RDX: 0000000000000000 RSI: 000000003bfc6901 RDI: ffffffff81a73a40
>         RBP: ffff88003e2f3d38 R08: 0000000000000152 R09: 0000000000000000
>         R10: ffff88003e2f3c18 R11: 000000000000865b R12: ffff88003bf1a900
>         R13: 0000000000000000 R14: ffff88003bf1a908 R15: ffff88003e2f4000
>         ...
>         CR2: 00000000ffffff8a CR3: 000000003e3ec000 CR4: 00000000000006f0
>         ...
>         Call Trace:
>          [<ffffffff8126c756>] key_gc_unused_keys.constprop.1+0x5d/0x10f
>          [<ffffffff8126ca71>] key_garbage_collector+0x1fa/0x351
>          [<ffffffff8105ec9b>] process_one_work+0x28e/0x547
>          [<ffffffff8105fd17>] worker_thread+0x26e/0x361
>          [<ffffffff8105faa9>] ? rescuer_thread+0x2a8/0x2a8
>          [<ffffffff810648ad>] kthread+0xf3/0xfb
>          [<ffffffff810647ba>] ? kthread_create_on_node+0x1c2/0x1c2
>          [<ffffffff815f2ccf>] ret_from_fork+0x3f/0x70
>          [<ffffffff810647ba>] ? 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 <dvyukov@google.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>
> 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, &key->flags) &&
> +                   !test_bit(KEY_FLAG_NEGATIVE, &key->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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1250510

Dmitry Vyukov <dvyukov@google.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250518

On Mon, Oct 19, 2015 at 11:30 AM, David Howells <dhowells@redhat.com> wrote:
> Dmitry Vyukov <dvyukov@google.com> 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 <dvyukov@google.com>

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

[toc] | [prev] | [next] | [standalone]

#1250549

Dmitry Vyukov <dvyukov@google.com> wrote:

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

That helps:-)

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

[toc] | [prev] | [next] | [standalone]

#1250550

Dmitry Vyukov <dvyukov@google.com> 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 <dhowells@redhat.com>
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 <dhowells@redhat.com>

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 == &key_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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250555

On Mon, Oct 19, 2015 at 12:33 PM, David Howells <dhowells@redhat.com> wrote:
> Dmitry Vyukov <dvyukov@google.com> 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 <dhowells@redhat.com>
> 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 <dhowells@redhat.com>
>
> 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 == &key_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+unsubscribe@googlegroups.com.
> To post to this group, send email to syzkaller@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250565

Dmitry Vyukov <dvyukov@google.com> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web