* Re: [PATCH v3] apparmor: Fix use-after-free in aa_audit_rule_init
From: Tyler Hicks @ 2019-10-21 16:08 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, John Johansen, James Morris,
Serge E. Hallyn, linux-security-module, linux-kernel
In-Reply-To: <20191021160532.7719-1-navid.emamdoost@gmail.com>
On 2019-10-21 11:05:31, Navid Emamdoost wrote:
> In the implementation of aa_audit_rule_init(), when aa_label_parse()
> fails the allocated memory for rule is released using
> aa_audit_rule_free(). But after this release, the return statement
> tries to access the label field of the rule which results in
> use-after-free. Before releasing the rule, copy errNo and return it
> after release.
>
> Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
Thanks!
Tyler
> ---
> Changes in v3:
> -- applied Tyler Hicks recommendation on err initialization.
>
> security/apparmor/audit.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
> index 5a98661a8b46..597732503815 100644
> --- a/security/apparmor/audit.c
> +++ b/security/apparmor/audit.c
> @@ -197,8 +197,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
> GFP_KERNEL, true, false);
> if (IS_ERR(rule->label)) {
> + int err = PTR_ERR(rule->label);
> aa_audit_rule_free(rule);
> - return PTR_ERR(rule->label);
> + return err;
> }
>
> *vrule = rule;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v2] apparmor: Fix use-after-free in aa_audit_rule_init
From: Navid Emamdoost @ 2019-10-21 16:06 UTC (permalink / raw)
To: Tyler Hicks
Cc: John Johansen, Navid Emamdoost, Stephen McCamant, Kangjie Lu,
James Morris, Serge E. Hallyn, linux-security-module, LKML
In-Reply-To: <20191021154533.GB12140@elm>
On Mon, Oct 21, 2019 at 10:45 AM Tyler Hicks <tyhicks@canonical.com> wrote:
>
> On 2019-10-21 10:23:47, Navid Emamdoost wrote:
> > In the implementation of aa_audit_rule_init(), when aa_label_parse()
> > fails the allocated memory for rule is released using
> > aa_audit_rule_free(). But after this release, the return statement
> > tries to access the label field of the rule which results in
> > use-after-free. Before releasing the rule, copy errNo and return it
> > after release.
> >
> > Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
>
> Ugh! I'm not sure what I was thinking when I authored that patch. :/
>
> > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > ---
> > Changes in v2:
> > -- Fix typo in description
> > -- move err definition inside the if statement.
> >
> > security/apparmor/audit.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
> > index 5a98661a8b46..334065302fb6 100644
> > --- a/security/apparmor/audit.c
> > +++ b/security/apparmor/audit.c
> > @@ -197,8 +197,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> > rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
> > GFP_KERNEL, true, false);
> > if (IS_ERR(rule->label)) {
> > + int err = rule->label;
>
> Since rule->label is a pointer, I'd like to see this:
>
> int err = PTR_ERR(rule->label);
>
> > aa_audit_rule_free(rule);
> > - return PTR_ERR(rule->label);
> > + return PTR_ERR(err);
>
> This line would change to:
>
> return err;
>
Tyler, I made the changes and sent v3.
>
> Tyler
>
> > }
> >
> > *vrule = rule;
> > --
> > 2.17.1
> >
--
Navid.
^ permalink raw reply
* Re: WARNING: refcount bug in find_key_to_update
From: Dmitry Vyukov @ 2019-10-21 16:05 UTC (permalink / raw)
To: David Howells
Cc: syzbot, Jarkko Sakkinen, James Morris, keyrings, LKML,
linux-security-module, Serge E. Hallyn, syzkaller-bugs
In-Reply-To: <8509.1571673553@warthog.procyon.org.uk>
On Mon, Oct 21, 2019 at 5:59 PM David Howells <dhowells@redhat.com> wrote:
>
> syzbot <syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com> wrote:
>
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11c8adab600000
>
> How do I tell what's been passed into the add_key for the encrypted key?
Hi David,
The easiest and most reliable would be to run it and dump the data in
the kernel function.
^ permalink raw reply
* [PATCH v3] apparmor: Fix use-after-free in aa_audit_rule_init
From: Navid Emamdoost @ 2019-10-21 16:05 UTC (permalink / raw)
To: tyhicks
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, John Johansen,
James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20191021154533.GB12140@elm>
In the implementation of aa_audit_rule_init(), when aa_label_parse()
fails the allocated memory for rule is released using
aa_audit_rule_free(). But after this release, the return statement
tries to access the label field of the rule which results in
use-after-free. Before releasing the rule, copy errNo and return it
after release.
Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v3:
-- applied Tyler Hicks recommendation on err initialization.
security/apparmor/audit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 5a98661a8b46..597732503815 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -197,8 +197,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
GFP_KERNEL, true, false);
if (IS_ERR(rule->label)) {
+ int err = PTR_ERR(rule->label);
aa_audit_rule_free(rule);
- return PTR_ERR(rule->label);
+ return err;
}
*vrule = rule;
--
2.17.1
^ permalink raw reply related
* Re: WARNING: refcount bug in find_key_to_update
From: David Howells @ 2019-10-21 15:59 UTC (permalink / raw)
To: syzbot
Cc: dhowells, jarkko.sakkinen, jmorris, keyrings, linux-kernel,
linux-security-module, serge, syzkaller-bugs
In-Reply-To: <000000000000830fe50595115344@google.com>
syzbot <syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com> wrote:
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11c8adab600000
How do I tell what's been passed into the add_key for the encrypted key?
David
^ permalink raw reply
* Re: [PATCH] security/keyring: avoid pagefaults in keyring_read_iterator
From: Jarkko Sakkinen @ 2019-10-21 15:47 UTC (permalink / raw)
To: Chris von Recklinghausen
Cc: David Howells, James Morris, Serge E . Hallyn, keyrings,
linux-security-module, linux-kernel, Waiman Long
In-Reply-To: <20191018184030.8407-1-crecklin@redhat.com>
On Fri, Oct 18, 2019 at 02:40:30PM -0400, Chris von Recklinghausen wrote:
> under a debug kernel, the following circular locking dependency was observed:
>
> [ 5896.294840] ======================================================
> [ 5896.294846] [ INFO: possible circular locking dependency detected ]
> [ 5896.294852] 3.10.0-957.31.1.el7.ppc64le.debug #1 Tainted: G OE ------------ T
> [ 5896.294857] -------------------------------------------------------
> [ 5896.294863] keyctl/21719 is trying to acquire lock:
> [ 5896.294867] (&mm->mmap_sem){++++++}, at: [<c000000000331db8>] might_fault+0x88/0xf0
> [ 5896.294881]
> [ 5896.294881] but task is already holding lock:
> [ 5896.294886] (&type->lock_class){+++++.}, at: [<c0000000004ff504>] keyctl_read_key+0xb4/0x170
> [ 5896.294899]
> [ 5896.294899] which lock already depends on the new lock.
> [ 5896.294899]
> [ 5896.294905]
> [ 5896.294905] the existing dependency chain (in reverse order) is:
> [ 5896.294911]
> -> #1 (&type->lock_class){+++++.}:
> [ 5896.294920] [<c0000000001caaf4>] check_prevs_add+0x144/0x1d0
> [ 5896.294929] [<c0000000001ce338>] lock_acquire+0xe38/0x16c0
> [ 5896.294936] [<c000000000b8e5e4>] down_write+0x84/0x130
> [ 5896.294943] [<c0000000004fd330>] key_link+0x90/0x2e0
> [ 5896.294949] [<c000000000503f44>] call_sbin_request_key+0x154/0x640
> [ 5896.294956] [<c000000000bb1424>] construct_key_and_link+0x38c/0x464
> [ 5896.294964] [<c000000000504bb4>] request_key+0x214/0x230
> [ 5896.294970] [<d0000000047e2490>] nfs_idmap_get_key+0x110/0x460 [nfsv4]
> [ 5896.294986] [<d0000000047e3464>] nfs_map_name_to_uid+0x84/0x2f0 [nfsv4]
> [ 5896.294999] [<d0000000047c3180>] decode_attr_owner+0x1d0/0x2c0 [nfsv4]
> [ 5896.295010] [<d0000000047c6f18>] decode_getfattr_attrs+0x5a8/0xb80 [nfsv4]
> [ 5896.295022] [<d0000000047c75cc>] decode_getfattr_generic.constprop.100+0xdc/0x200 [nfsv4]
> [ 5896.295033] [<d0000000047c8048>] nfs4_xdr_dec_getattr+0xa8/0xb0 [nfsv4]
> [ 5896.295044] [<d0000000035eff58>] rpcauth_unwrap_resp+0xf8/0x150 [sunrpc]
> [ 5896.295060] [<d0000000035d357c>] call_decode+0x29c/0x910 [sunrpc]
> [ 5896.295071] [<d0000000035eb940>] __rpc_execute+0xf0/0x870 [sunrpc]
> [ 5896.295083] [<d0000000035d233c>] rpc_run_task+0x14c/0x1c0 [sunrpc]
> [ 5896.295094] [<d0000000047a12f0>] nfs4_call_sync_sequence+0x70/0xb0 [nfsv4]
> [ 5896.295105] [<d0000000047a2254>] _nfs4_proc_getattr+0xc4/0xf0 [nfsv4]
> [ 5896.295115] [<d0000000047b9ee4>] nfs4_proc_getattr+0x84/0x220 [nfsv4]
> [ 5896.295126] [<d00000000454519c>] __nfs_revalidate_inode+0x1cc/0x7a0 [nfs]
> [ 5896.295138] [<d000000004546284>] nfs_revalidate_mapping+0x1f4/0x520 [nfs]
> [ 5896.295150] [<d00000000453df98>] nfs_file_mmap+0x78/0xb0 [nfs]
> [ 5896.295160] [<c000000000343df8>] mmap_region+0x518/0x780
> [ 5896.295167] [<c000000000344488>] do_mmap+0x428/0x510
> [ 5896.295173] [<c000000000317508>] vm_mmap_pgoff+0x108/0x150
> [ 5896.295179] [<c000000000340f1c>] SyS_mmap_pgoff+0xec/0x2c0
> [ 5896.295186] [<c0000000000173b8>] sys_mmap+0x78/0x90
> [ 5896.295192] [<c00000000000a294>] system_call+0x3c/0x100
> [ 5896.295199]
> -> #0 (&mm->mmap_sem){++++++}:
> [ 5896.295207] [<c0000000001ca990>] check_prev_add+0xa50/0xa70
> [ 5896.295214] [<c0000000001caaf4>] check_prevs_add+0x144/0x1d0
> [ 5896.295221] [<c0000000001ce338>] lock_acquire+0xe38/0x16c0
> [ 5896.295228] [<c000000000331de4>] might_fault+0xb4/0xf0
> [ 5896.295235] [<c0000000004fc644>] keyring_read_iterator+0x54/0xd0
> [ 5896.295242] [<c00000000060fe98>] assoc_array_subtree_iterate+0x4d8/0x790
> [ 5896.295249] [<c0000000004fbc00>] keyring_read+0x80/0xa0
> [ 5896.295255] [<c0000000004ff5a4>] keyctl_read_key+0x154/0x170
> [ 5896.295262] [<c00000000000a294>] system_call+0x3c/0x100
> [ 5896.295269]
> [ 5896.295269] other info that might help us debug this:
> [ 5896.295275] Possible unsafe locking scenario:
> [ 5896.295275]
> [ 5896.295281] CPU0 CPU1
> [ 5896.295285] ---- ----
> [ 5896.295289] lock(&type->lock_class);
> [ 5896.295294] lock(&mm->mmap_sem);
> [ 5896.295301] lock(&type->lock_class);
> [ 5896.295308] lock(&mm->mmap_sem);
> [ 5896.295313]
> [ 5896.295313] *** DEADLOCK ***
> [ 5896.295313]
> [ 5896.295320] 1 lock held by keyctl/21719:
> [ 5896.295323] #0: (&type->lock_class){+++++.}, at: [<c0000000004ff504>] keyctl_read_key+0xb4/0x170
> [ 5896.295337]
> [ 5896.295337] stack backtrace:
> [ 5896.295343] CPU: 1 PID: 21719 Comm: keyctl Kdump: loaded Tainted: G OE ------------ T 3.10.0-957.31.1.el7.ppc64le.debug #1
> [ 5896.295351] Call Trace:
> [ 5896.295355] [c00000016100f8e0] [c0000000000205d0] show_stack+0x90/0x390 (unreliable)
> [ 5896.295363] [c00000016100f9a0] [c000000000bb37d0] dump_stack+0x30/0x44
> [ 5896.295371] [c00000016100f9c0] [c000000000ba7f3c] print_circular_bug+0x36c/0x3a0
> [ 5896.295379] [c00000016100fa60] [c0000000001ca990] check_prev_add+0xa50/0xa70
> [ 5896.295386] [c00000016100fb60] [c0000000001caaf4] check_prevs_add+0x144/0x1d0
> [ 5896.295393] [c00000016100fbb0] [c0000000001ce338] lock_acquire+0xe38/0x16c0
> [ 5896.295400] [c00000016100fce0] [c000000000331de4] might_fault+0xb4/0xf0
> [ 5896.295407] [c00000016100fd00] [c0000000004fc644] keyring_read_iterator+0x54/0xd0
> [ 5896.295415] [c00000016100fd40] [c00000000060fe98] assoc_array_subtree_iterate+0x4d8/0x790
> [ 5896.295423] [c00000016100fd90] [c0000000004fbc00] keyring_read+0x80/0xa0
> [ 5896.295430] [c00000016100fde0] [c0000000004ff5a4] keyctl_read_key+0x154/0x170
> [ 5896.295437] [c00000016100fe30] [c00000000000a294] system_call+0x3c/0x100
>
> The put_user call from keyring_read_iterator caused a page fault which attempts
> to lock mm->mmap_sem and type->lock_class (key->sem) in the reverse order that
> keyring_read_iterator did, thus causing the circular locking dependency.
>
> Remedy this by using access_ok and __put_user instead of put_user so we'll
> return an error instead of faulting in the page.
>
> Also to prevent potential changes in behavior to applications, pre-fault the
> page(s) with the key in keyctl_read_key before taking the read semaphore to
> ensure that the page is present by the time keyring_read_iterator is called.
>
> Suggested-by: Waiman Long <longman@redhat.com>
> Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
> ---
> security/keys/keyctl.c | 10 ++++++++--
> security/keys/keyring.c | 7 +++----
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 9b898c9..f8a2553 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -846,9 +846,15 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
> can_read_key:
> ret = -EOPNOTSUPP;
> if (key->type->read) {
> - /* Read the data with the semaphore held (since we might sleep)
> - * to protect against the key being updated or revoked.
> + /*
> + * Read the data with the semaphore held (since we might sleep)
> + * to protect against the key being updated or revoked. The
> + * user buffer, if not mapped yet, will be faulted in to
> + * prevent read failure.
> */
> + key_serial_t tmp;
> +
> + get_user(tmp, buffer); /* Prefault */
> down_read(&key->sem);
> ret = key_validate(key);
> if (ret == 0)
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index febf36c..7cac3c7 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
> {
> struct keyring_read_iterator_context *ctx = data;
> const struct key *key = keyring_ptr_to_key(object);
> - int ret;
>
> kenter("{%s,%d},,{%zu/%zu}",
> key->type->name, key->serial, ctx->count, ctx->buflen);
> @@ -467,9 +466,9 @@ static int keyring_read_iterator(const void *object, void *data)
> if (ctx->count >= ctx->buflen)
> return 1;
>
> - ret = put_user(key->serial, ctx->buffer);
> - if (ret < 0)
> - return ret;
> + if (!access_ok(ctx->buffer, sizeof(key->serial)) ||
> + __put_user(key->serial, ctx->buffer) < 0)
> + return -EFAULT;
> ctx->buffer++;
> ctx->count += sizeof(key->serial);
> return 0;
> --
> 1.8.3.1
>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [PATCH] security/keyring: avoid pagefaults in keyring_read_iterator
From: Chris von Recklinghausen @ 2019-10-21 15:46 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E . Hallyn, keyrings,
linux-security-module, linux-kernel, Waiman Long
In-Reply-To: <30309.1571667719@warthog.procyon.org.uk>
On 10/21/2019 10:21 AM, David Howells wrote:
> Chris von Recklinghausen <crecklin@redhat.com> wrote:
>
>> The put_user call from keyring_read_iterator caused a page fault which
>> attempts to lock mm->mmap_sem and type->lock_class (key->sem) in the reverse
>> order that keyring_read_iterator did, thus causing the circular locking
>> dependency.
>>
>> Remedy this by using access_ok and __put_user instead of put_user so we'll
>> return an error instead of faulting in the page.
> I wonder if it's better to create a kernel buffer outside of the lock in
> keyctl_read_key(). Hmmm... The reason I didn't want to do that is that
> keyrings have don't have limits on the size. Maybe that's not actually a
> problem, since 1MiB would be able to hold a list of a quarter of a million
> keys.
>
> David
>
Hi David,
Thanks for the feedback.
I can try to prototype that, but regardless of where the kernel buffer
is allocated, the important part is causing the initial pagefault in the
read path outside the lock so __put_user won't fail due to a valid user
address but page backing the user address isn't in-core.
I'll start work on v2.
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH v2] apparmor: Fix use-after-free in aa_audit_rule_init
From: Tyler Hicks @ 2019-10-21 15:45 UTC (permalink / raw)
To: Navid Emamdoost
Cc: john.johansen, emamd001, smccaman, kjlu, James Morris,
Serge E. Hallyn, linux-security-module, linux-kernel
In-Reply-To: <20191021152348.3906-1-navid.emamdoost@gmail.com>
On 2019-10-21 10:23:47, Navid Emamdoost wrote:
> In the implementation of aa_audit_rule_init(), when aa_label_parse()
> fails the allocated memory for rule is released using
> aa_audit_rule_free(). But after this release, the return statement
> tries to access the label field of the rule which results in
> use-after-free. Before releasing the rule, copy errNo and return it
> after release.
>
> Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
Ugh! I'm not sure what I was thinking when I authored that patch. :/
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> -- Fix typo in description
> -- move err definition inside the if statement.
>
> security/apparmor/audit.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
> index 5a98661a8b46..334065302fb6 100644
> --- a/security/apparmor/audit.c
> +++ b/security/apparmor/audit.c
> @@ -197,8 +197,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
> GFP_KERNEL, true, false);
> if (IS_ERR(rule->label)) {
> + int err = rule->label;
Since rule->label is a pointer, I'd like to see this:
int err = PTR_ERR(rule->label);
> aa_audit_rule_free(rule);
> - return PTR_ERR(rule->label);
> + return PTR_ERR(err);
This line would change to:
return err;
Tyler
> }
>
> *vrule = rule;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] apparmor: Fix use-after-free in aa_audit_rule_init
From: Navid Emamdoost @ 2019-10-21 15:25 UTC (permalink / raw)
To: John Johansen
Cc: Markus Elfring, linux-security-module, kernel-janitors, LKML,
Navid Emamdoost, Kangjie Lu, Stephen McCamant, James Morris,
Serge E. Hallyn, Tyler Hicks
In-Reply-To: <57b61298-cbeb-f0ff-c6ba-b8f64d5d0287@canonical.com>
On Sun, Oct 20, 2019 at 1:51 PM John Johansen
<john.johansen@canonical.com> wrote:
>
> On 10/20/19 7:16 AM, Markus Elfring wrote:
> >> … But after this release the the return statement
> >> tries to access the label field of the rule which results in
> >> use-after-free. Before releaseing the rule, copy errNo and return it
> >> after releasing rule.
> >
> Navid thanks for finding this, and Markus thanks for the review
>
> > Please avoid a duplicate word and a typo in this change description.
> > My preference would be a v2 version of the patch with the small clean-ups
> that Markus has pointed out.
John and Markus, I updated and submitted v2.
>
> If I don't see a v2 this week I can pull this one in and do the revisions
> myself adding a little fix-up note.
>
> >
> > …
> >> +++ b/security/apparmor/audit.c
> > …
> >> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> >> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
> >> GFP_KERNEL, true, false);
> >> if (IS_ERR(rule->label)) {
> >> + err = rule->label;
> >
> > How do you think about to define the added local variable in this if branch directly?
> >
> > + int err = rule->label;
> >
>
> yes, since err isn't defined or in use else where this would be preferable
>
> >> aa_audit_rule_free(rule);
> >> - return PTR_ERR(rule->label);
> >> + return PTR_ERR(err);
> >> }
> >>
> >> *vrule = rule;
> >
> >
> > Regards,
> > Markus
> >
>
--
Thanks,
Navid.
^ permalink raw reply
* [PATCH v2] apparmor: Fix use-after-free in aa_audit_rule_init
From: Navid Emamdoost @ 2019-10-21 15:23 UTC (permalink / raw)
To: john.johansen
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, James Morris,
Serge E. Hallyn, linux-security-module, linux-kernel
In-Reply-To: <57b61298-cbeb-f0ff-c6ba-b8f64d5d0287@canonical.com>
In the implementation of aa_audit_rule_init(), when aa_label_parse()
fails the allocated memory for rule is released using
aa_audit_rule_free(). But after this release, the return statement
tries to access the label field of the rule which results in
use-after-free. Before releasing the rule, copy errNo and return it
after release.
Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
-- Fix typo in description
-- move err definition inside the if statement.
security/apparmor/audit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 5a98661a8b46..334065302fb6 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -197,8 +197,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
GFP_KERNEL, true, false);
if (IS_ERR(rule->label)) {
+ int err = rule->label;
aa_audit_rule_free(rule);
- return PTR_ERR(rule->label);
+ return PTR_ERR(err);
}
*vrule = rule;
--
2.17.1
^ permalink raw reply related
* CAAM hw key support status
From: Stefan Müller-Klieser @ 2019-10-21 14:28 UTC (permalink / raw)
To: franck.lenormand; +Cc: horia.geanta, linux-crypto, linux-security-module
Hi Franck,
you sent an RFC to the linux-security-module list:
March 1, 2019, 4:09 p.m. UTC
[RFC,0/2] Create CAAM HW key in linux keyring and use in dmcrypt
https://patchwork.kernel.org/cover/10835635/
Is there any update on this work? How do you want to move forward?
Regards, Stefan
^ permalink raw reply
* Re: [PATCH] security/keyring: avoid pagefaults in keyring_read_iterator
From: David Howells @ 2019-10-21 14:21 UTC (permalink / raw)
To: Chris von Recklinghausen
Cc: dhowells, Jarkko Sakkinen, James Morris, Serge E . Hallyn,
keyrings, linux-security-module, linux-kernel, Waiman Long
In-Reply-To: <20191018184030.8407-1-crecklin@redhat.com>
Chris von Recklinghausen <crecklin@redhat.com> wrote:
> The put_user call from keyring_read_iterator caused a page fault which
> attempts to lock mm->mmap_sem and type->lock_class (key->sem) in the reverse
> order that keyring_read_iterator did, thus causing the circular locking
> dependency.
>
> Remedy this by using access_ok and __put_user instead of put_user so we'll
> return an error instead of faulting in the page.
I wonder if it's better to create a kernel buffer outside of the lock in
keyctl_read_key(). Hmmm... The reason I didn't want to do that is that
keyrings have don't have limits on the size. Maybe that's not actually a
problem, since 1MiB would be able to hold a list of a quarter of a million
keys.
David
^ permalink raw reply
* Re: [Kgdb-bugreport] [PATCH] kernel: convert switch/case fallthrough comments to fallthrough;
From: Daniel Thompson @ 2019-10-21 9:09 UTC (permalink / raw)
To: Joe Perches
Cc: Linus Torvalds, linux-pm, kgdb-bugreport, linux-kernel,
linux-block, linux-security-module, linux-audit, netdev, bpf
In-Reply-To: <f31b38b9ad515a138edaecf85701b1e3db064114.camel@perches.com>
On Fri, Oct 18, 2019 at 09:35:08AM -0700, Joe Perches wrote:
> Use the new pseudo keyword "fallthrough;" and not the
> various /* fallthrough */ style comments.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> This is a single patch for the kernel/ source tree,
> which would otherwise
> be sent through as separate
> patches to 19 maintainer sections.
For the kernel/debug/ files:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Will you be putting this in an immutable branch once you've collected
enough acks?
[TBH I don't actually expect any conflict between this and the pending
kgdb changes for this kernel cycle anyway...]
Daniel.
>
> compilation tested only.
>
> Done by the script in this email:
> https://lore.kernel.org/lkml/9fe980f7e28242c2835ffae34914c5f68e8268a7.camel@perches.com/
>
> kernel/auditfilter.c | 2 +-
> kernel/bpf/cgroup.c | 4 ++--
> kernel/bpf/verifier.c | 4 ++--
> kernel/capability.c | 2 +-
> kernel/compat.c | 6 +++---
> kernel/debug/gdbstub.c | 6 +++---
> kernel/debug/kdb/kdb_keyboard.c | 4 ++--
> kernel/debug/kdb/kdb_support.c | 6 +++---
> kernel/events/core.c | 3 +--
> kernel/futex.c | 4 ++--
> kernel/gcov/gcc_3_4.c | 6 +++---
> kernel/irq/handle.c | 3 +--
> kernel/irq/manage.c | 5 ++---
> kernel/kallsyms.c | 4 ++--
> kernel/pid.c | 2 +-
> kernel/power/hibernate.c | 2 +-
> kernel/power/qos.c | 4 ++--
> kernel/printk/printk.c | 2 +-
> kernel/sched/core.c | 2 +-
> kernel/sched/topology.c | 6 +++---
> kernel/signal.c | 2 +-
> kernel/sys.c | 3 +--
> kernel/time/hrtimer.c | 2 +-
> kernel/time/posix-timers.c | 4 ++--
> kernel/time/tick-broadcast.c | 2 +-
> kernel/time/timer.c | 2 +-
> kernel/trace/blktrace.c | 2 +-
> kernel/trace/trace_events_filter.c | 4 ++--
> 28 files changed, 47 insertions(+), 51 deletions(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index b0126e9c0743..471cd680479d 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -674,7 +674,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->values[i] = AUDIT_UID_UNSET;
> break;
> }
> - /* fall through - if set */
> + fallthrough; /* if set */
> default:
> data->values[i] = f->val;
> }
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ddd8addcdb5c..955631f1b77d 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -797,7 +797,7 @@ cgroup_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> case BPF_FUNC_trace_printk:
> if (capable(CAP_SYS_ADMIN))
> return bpf_get_trace_printk_proto();
> - /* fall through */
> + fallthrough;
> default:
> return NULL;
> }
> @@ -1439,7 +1439,7 @@ static bool cg_sockopt_is_valid_access(int off, int size,
> return prog->expected_attach_type ==
> BPF_CGROUP_GETSOCKOPT;
> case offsetof(struct bpf_sockopt, optname):
> - /* fallthrough */
> + fallthrough;
> case offsetof(struct bpf_sockopt, level):
> if (size != size_default)
> return false;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ffc3e53f5300..d2b6fd8545e2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2249,7 +2249,7 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> case BPF_PROG_TYPE_CGROUP_SKB:
> if (t == BPF_WRITE)
> return false;
> - /* fallthrough */
> + fallthrough;
>
> /* Program types with direct read + write access go here! */
> case BPF_PROG_TYPE_SCHED_CLS:
> @@ -4381,7 +4381,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> off_reg == dst_reg ? dst : src);
> return -EACCES;
> }
> - /* fall-through */
> + fallthrough;
> default:
> break;
> }
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 1444f3954d75..7c59b096c98a 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -93,7 +93,7 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
> break;
> case _LINUX_CAPABILITY_VERSION_2:
> warn_deprecated_v2();
> - /* fall through - v3 is otherwise equivalent to v2. */
> + fallthrough; /* v3 is otherwise equivalent to v2 */
> case _LINUX_CAPABILITY_VERSION_3:
> *tocopy = _LINUX_CAPABILITY_U32S_3;
> break;
> diff --git a/kernel/compat.c b/kernel/compat.c
> index a2bc1d6ceb57..d9c61f4317be 100644
> --- a/kernel/compat.c
> +++ b/kernel/compat.c
> @@ -343,11 +343,11 @@ get_compat_sigset(sigset_t *set, const compat_sigset_t __user *compat)
> return -EFAULT;
> switch (_NSIG_WORDS) {
> case 4: set->sig[3] = v.sig[6] | (((long)v.sig[7]) << 32 );
> - /* fall through */
> + fallthrough;
> case 3: set->sig[2] = v.sig[4] | (((long)v.sig[5]) << 32 );
> - /* fall through */
> + fallthrough;
> case 2: set->sig[1] = v.sig[2] | (((long)v.sig[3]) << 32 );
> - /* fall through */
> + fallthrough;
> case 1: set->sig[0] = v.sig[0] | (((long)v.sig[1]) << 32 );
> }
> #else
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index 4b280fc7dd67..b9d8b7248964 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -1033,14 +1033,14 @@ int gdb_serial_stub(struct kgdb_state *ks)
> return DBG_PASS_EVENT;
> }
> #endif
> - /* Fall through */
> + fallthrough;
> case 'C': /* Exception passing */
> tmp = gdb_cmd_exception_pass(ks);
> if (tmp > 0)
> goto default_handle;
> if (tmp == 0)
> break;
> - /* Fall through - on tmp < 0 */
> + fallthrough; /* on tmp < 0 */
> case 'c': /* Continue packet */
> case 's': /* Single step packet */
> if (kgdb_contthread && kgdb_contthread != current) {
> @@ -1049,7 +1049,7 @@ int gdb_serial_stub(struct kgdb_state *ks)
> break;
> }
> dbg_activate_sw_breakpoints();
> - /* Fall through - to default processing */
> + fallthrough; /* to default processing */
> default:
> default_handle:
> error = kgdb_arch_handle_exception(ks->ex_vector,
> diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
> index 750497b0003a..f877a0a0d7cf 100644
> --- a/kernel/debug/kdb/kdb_keyboard.c
> +++ b/kernel/debug/kdb/kdb_keyboard.c
> @@ -173,11 +173,11 @@ int kdb_get_kbd_char(void)
> case KT_LATIN:
> if (isprint(keychar))
> break; /* printable characters */
> - /* fall through */
> + fallthrough;
> case KT_SPEC:
> if (keychar == K_ENTER)
> break;
> - /* fall through */
> + fallthrough;
> default:
> return -1; /* ignore unprintables */
> }
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index b8e6306e7e13..d636506f695a 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -432,7 +432,7 @@ int kdb_getphysword(unsigned long *word, unsigned long addr, size_t size)
> *word = w8;
> break;
> }
> - /* fall through */
> + fallthrough;
> default:
> diag = KDB_BADWIDTH;
> kdb_printf("kdb_getphysword: bad width %ld\n", (long) size);
> @@ -481,7 +481,7 @@ int kdb_getword(unsigned long *word, unsigned long addr, size_t size)
> *word = w8;
> break;
> }
> - /* fall through */
> + fallthrough;
> default:
> diag = KDB_BADWIDTH;
> kdb_printf("kdb_getword: bad width %ld\n", (long) size);
> @@ -525,7 +525,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
> diag = kdb_putarea(addr, w8);
> break;
> }
> - /* fall through */
> + fallthrough;
> default:
> diag = KDB_BADWIDTH;
> kdb_printf("kdb_putword: bad width %ld\n", (long) size);
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9ec0b0bfddbd..04e75b1144c5 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9361,8 +9361,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> case IF_SRC_KERNELADDR:
> case IF_SRC_KERNEL:
> kernel = 1;
> - /* fall through */
> -
> + fallthrough;
> case IF_SRC_FILEADDR:
> case IF_SRC_FILE:
> if (state != IF_STATE_SOURCE)
> diff --git a/kernel/futex.c b/kernel/futex.c
> index bd18f60e4c6c..ab12b6229d2d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3639,12 +3639,12 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
> switch (cmd) {
> case FUTEX_WAIT:
> val3 = FUTEX_BITSET_MATCH_ANY;
> - /* fall through */
> + fallthrough;
> case FUTEX_WAIT_BITSET:
> return futex_wait(uaddr, flags, val, timeout, val3);
> case FUTEX_WAKE:
> val3 = FUTEX_BITSET_MATCH_ANY;
> - /* fall through */
> + fallthrough;
> case FUTEX_WAKE_BITSET:
> return futex_wake(uaddr, flags, val, val3);
> case FUTEX_REQUEUE:
> diff --git a/kernel/gcov/gcc_3_4.c b/kernel/gcov/gcc_3_4.c
> index 801ee4b0b969..32fc3278166f 100644
> --- a/kernel/gcov/gcc_3_4.c
> +++ b/kernel/gcov/gcc_3_4.c
> @@ -455,7 +455,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
> case RECORD_COUNT:
> /* Advance to next count */
> iter->count++;
> - /* fall through */
> + fallthrough;
> case RECORD_COUNT_LEN:
> if (iter->count < get_func(iter)->n_ctrs[iter->type]) {
> iter->record = 9;
> @@ -465,7 +465,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
> get_type(iter)->offset += iter->count;
> iter->count = 0;
> iter->type++;
> - /* fall through */
> + fallthrough;
> case RECORD_FUNCTION_CHECK:
> if (iter->type < iter->num_types) {
> iter->record = 7;
> @@ -474,7 +474,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
> /* Advance to next function */
> iter->type = 0;
> iter->function++;
> - /* fall through */
> + fallthrough;
> case RECORD_TIME_STAMP:
> if (iter->function < iter->info->n_functions)
> iter->record = 3;
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index a4ace611f47f..b38d2fd70fe1 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -165,8 +165,7 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
> }
>
> __irq_wake_thread(desc, action);
> -
> - /* Fall through - to add to randomness */
> + fallthrough; /* to add to randomness */
> case IRQ_HANDLED:
> *flags |= action->flags;
> break;
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 1753486b440c..baa86020f243 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -222,7 +222,7 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
> case IRQ_SET_MASK_OK:
> case IRQ_SET_MASK_OK_DONE:
> cpumask_copy(desc->irq_common_data.affinity, mask);
> - /* fall through */
> + fallthrough;
> case IRQ_SET_MASK_OK_NOCOPY:
> irq_validate_effective_affinity(data);
> irq_set_thread_affinity(desc);
> @@ -792,8 +792,7 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
> case IRQ_SET_MASK_OK_DONE:
> irqd_clear(&desc->irq_data, IRQD_TRIGGER_MASK);
> irqd_set(&desc->irq_data, flags);
> - /* fall through */
> -
> + fallthrough;
> case IRQ_SET_MASK_OK_NOCOPY:
> flags = irqd_get_trigger_type(&desc->irq_data);
> irq_settings_set_trigger_mask(desc, flags);
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 136ce049c4ad..05ce8a4d4729 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -651,11 +651,11 @@ int kallsyms_show_value(void)
> case 0:
> if (kallsyms_for_perf())
> return 1;
> - /* fallthrough */
> + fallthrough;
> case 1:
> if (has_capability_noaudit(current, CAP_SYSLOG))
> return 1;
> - /* fallthrough */
> + fallthrough;
> default:
> return 0;
> }
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 0a9f2e437217..b2a005a6dea1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -144,7 +144,7 @@ void free_pid(struct pid *pid)
> /* Handle a fork failure of the first process */
> WARN_ON(ns->child_reaper);
> ns->pid_allocated = 0;
> - /* fall through */
> + fallthrough;
> case 0:
> schedule_work(&ns->proc_work);
> break;
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 3c0a5a8170b0..d091dcd57557 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -647,7 +647,7 @@ static void power_down(void)
> break;
> case HIBERNATION_PLATFORM:
> hibernation_platform_enter();
> - /* Fall through */
> + fallthrough;
> case HIBERNATION_SHUTDOWN:
> if (pm_power_off)
> kernel_power_off();
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 9568a2fe7c11..6bf5295b2ade 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -236,7 +236,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
> * changed
> */
> plist_del(node, &c->list);
> - /* fall through */
> + fallthrough;
> case PM_QOS_ADD_REQ:
> plist_node_init(node, new_value);
> plist_add(node, &c->list);
> @@ -309,7 +309,7 @@ bool pm_qos_update_flags(struct pm_qos_flags *pqf,
> break;
> case PM_QOS_UPDATE_REQ:
> pm_qos_flags_remove_req(pqf, req);
> - /* fall through */
> + fallthrough;
> case PM_QOS_ADD_REQ:
> req->flags = val;
> INIT_LIST_HEAD(&req->node);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ca65327a6de8..6b3d7c68e6fe 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1531,7 +1531,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
> /* Read/clear last kernel messages */
> case SYSLOG_ACTION_READ_CLEAR:
> clear = true;
> - /* FALL THRU */
> + fallthrough;
> /* Read last kernel messages */
> case SYSLOG_ACTION_READ_ALL:
> if (!buf || len < 0)
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index dd05a378631a..050b728728f4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2068,7 +2068,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
> state = possible;
> break;
> }
> - /* Fall-through */
> + fallthrough;
> case possible:
> do_set_cpus_allowed(p, cpu_possible_mask);
> state = fail;
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b5667a273bf6..7d6b84e0caca 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1224,13 +1224,13 @@ static void __free_domain_allocs(struct s_data *d, enum s_alloc what,
> case sa_rootdomain:
> if (!atomic_read(&d->rd->refcount))
> free_rootdomain(&d->rd->rcu);
> - /* Fall through */
> + fallthrough;
> case sa_sd:
> free_percpu(d->sd);
> - /* Fall through */
> + fallthrough;
> case sa_sd_storage:
> __sdt_free(cpu_map);
> - /* Fall through */
> + fallthrough;
> case sa_none:
> break;
> }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c4da1ef56fdf..73bdcc1f2561 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -846,7 +846,7 @@ static int check_kill_permission(int sig, struct kernel_siginfo *info,
> */
> if (!sid || sid == task_session(current))
> break;
> - /* fall through */
> + fallthrough;
> default:
> return -EPERM;
> }
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a611d1d58c7d..bad4f30e7f37 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1737,8 +1737,7 @@ void getrusage(struct task_struct *p, int who, struct rusage *r)
>
> if (who == RUSAGE_CHILDREN)
> break;
> - /* fall through */
> -
> + fallthrough;
> case RUSAGE_SELF:
> thread_group_cputime_adjusted(p, &tgutime, &tgstime);
> utime += tgutime;
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 0d4dc241c0fb..8060a35682e1 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -373,7 +373,7 @@ static bool hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
> switch (state) {
> case ODEBUG_STATE_ACTIVE:
> WARN_ON(1);
> - /* fall through */
> + fallthrough;
> default:
> return false;
> }
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 0ec5b7a1d769..6cc658391702 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -413,12 +413,12 @@ static struct pid *good_sigevent(sigevent_t * event)
> rtn = pid_task(pid, PIDTYPE_PID);
> if (!rtn || !same_thread_group(rtn, current))
> return NULL;
> - /* FALLTHRU */
> + fallthrough;
> case SIGEV_SIGNAL:
> case SIGEV_THREAD:
> if (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX)
> return NULL;
> - /* FALLTHRU */
> + fallthrough;
> case SIGEV_NONE:
> return pid;
> default:
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index e51778c312f1..36d7464c8962 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -381,7 +381,7 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
> switch (mode) {
> case TICK_BROADCAST_FORCE:
> tick_broadcast_forced = 1;
> - /* fall through */
> + fallthrough;
> case TICK_BROADCAST_ON:
> cpumask_set_cpu(cpu, tick_broadcast_on);
> if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 4820823515e9..6512d721ef57 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -653,7 +653,7 @@ static bool timer_fixup_activate(void *addr, enum debug_obj_state state)
>
> case ODEBUG_STATE_ACTIVE:
> WARN_ON(1);
> - /* fall through */
> + fallthrough;
> default:
> return false;
> }
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 2d6e93ab0478..0a1753dc69d3 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -717,7 +717,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
> #endif
> case BLKTRACESTART:
> start = 1;
> - /* fall through */
> + fallthrough;
> case BLKTRACESTOP:
> ret = __blk_trace_startstop(q, start);
> break;
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index c9a74f82b14a..78b0bfc4d72e 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -499,7 +499,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> ptr++;
> break;
> }
> - /* fall through */
> + fallthrough;
> default:
> parse_error(pe, FILT_ERR_TOO_MANY_PREDS,
> next - str);
> @@ -1273,7 +1273,7 @@ static int parse_pred(const char *str, void *data,
> switch (op) {
> case OP_NE:
> pred->not = 1;
> - /* Fall through */
> + fallthrough;
> case OP_GLOB:
> case OP_EQ:
> break;
>
>
>
>
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgdb-bugreport@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
^ permalink raw reply
* Re: [GIT PULL] SafeSetID LSM changes for 5.4
From: Ingo Molnar @ 2019-10-21 6:58 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Linus Torvalds, Micah Morton, Jann Horn, Bart Van Assche,
Linux Kernel Mailing List, linux-security-module
In-Reply-To: <20190923233038.GE7828@paulmck-ThinkPad-P72>
* Paul E. McKenney <paulmck@kernel.org> wrote:
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -383,20 +383,22 @@ do { \
> } while (0)
>
> /**
> - * rcu_swap_protected() - swap an RCU and a regular pointer
> - * @rcu_ptr: RCU pointer
> + * rcu_replace() - replace an RCU pointer, returning its old value
> + * @rcu_ptr: RCU pointer, whose old value is returned
> * @ptr: regular pointer
> - * @c: the conditions under which the dereference will take place
> + * @c: the lockdep conditions under which the dereference will take place
> *
> - * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer and
> - * @c is the argument that is passed to the rcu_dereference_protected() call
> - * used to read that pointer.
> + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> + * pointer and @c is the lockdep argument that is passed to the
> + * rcu_dereference_protected() call used to read that pointer. The old
> + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> */
> -#define rcu_swap_protected(rcu_ptr, ptr, c) do { \
> +#define rcu_replace(rcu_ptr, ptr, c) \
> +({ \
> typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> rcu_assign_pointer((rcu_ptr), (ptr)); \
> - (ptr) = __tmp; \
> -} while (0)
> + __tmp; \
> +})
One small suggestion, would it make sense to name it "rcu_replace_pointer()"?
This would make it fit into the pointer handling family of RCU functions:
rcu_assign_pointer(), rcu_access_pointer(), RCU_INIT_POINTER() et al?
rcu_swap() would also look a bit weird if used in MM code. ;-)
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH] apparmor: Fix use-after-free in aa_audit_rule_init
From: John Johansen @ 2019-10-20 18:49 UTC (permalink / raw)
To: Markus Elfring, Navid Emamdoost, linux-security-module
Cc: kernel-janitors, linux-kernel, Navid Emamdoost, Kangjie Lu,
Stephen McCamant, James Morris, Serge E. Hallyn, Tyler Hicks
In-Reply-To: <83dcacc2-a820-fe63-a1b9-1809e8f14f2f@web.de>
On 10/20/19 7:16 AM, Markus Elfring wrote:
>> … But after this release the the return statement
>> tries to access the label field of the rule which results in
>> use-after-free. Before releaseing the rule, copy errNo and return it
>> after releasing rule.
>
Navid thanks for finding this, and Markus thanks for the review
> Please avoid a duplicate word and a typo in this change description.
> My preference would be a v2 version of the patch with the small clean-ups
that Markus has pointed out.
If I don't see a v2 this week I can pull this one in and do the revisions
myself adding a little fix-up note.
>
> …
>> +++ b/security/apparmor/audit.c
> …
>> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
>> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
>> GFP_KERNEL, true, false);
>> if (IS_ERR(rule->label)) {
>> + err = rule->label;
>
> How do you think about to define the added local variable in this if branch directly?
>
> + int err = rule->label;
>
yes, since err isn't defined or in use else where this would be preferable
>> aa_audit_rule_free(rule);
>> - return PTR_ERR(rule->label);
>> + return PTR_ERR(err);
>> }
>>
>> *vrule = rule;
>
>
> Regards,
> Markus
>
^ permalink raw reply
* Re: [PATCH] apparmor: Fix use-after-free in aa_audit_rule_init
From: Markus Elfring @ 2019-10-20 14:16 UTC (permalink / raw)
To: Navid Emamdoost, linux-security-module
Cc: kernel-janitors, linux-kernel, Navid Emamdoost, Kangjie Lu,
Stephen McCamant, James Morris, John Johansen, Serge E. Hallyn,
Tyler Hicks
In-Reply-To: <20191017014619.26708-1-navid.emamdoost@gmail.com>
> … But after this release the the return statement
> tries to access the label field of the rule which results in
> use-after-free. Before releaseing the rule, copy errNo and return it
> after releasing rule.
Please avoid a duplicate word and a typo in this change description.
…
> +++ b/security/apparmor/audit.c
…
> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
> GFP_KERNEL, true, false);
> if (IS_ERR(rule->label)) {
> + err = rule->label;
How do you think about to define the added local variable in this if branch directly?
+ int err = rule->label;
> aa_audit_rule_free(rule);
> - return PTR_ERR(rule->label);
> + return PTR_ERR(err);
> }
>
> *vrule = rule;
Regards,
Markus
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Luis Chamberlain @ 2019-10-19 18:36 UTC (permalink / raw)
To: Alan Maguire
Cc: Brendan Higgins, Matthias Maennich, shuah, john.johansen, jmorris,
serge, keescook, yzaikin, davidgow, tytso, linux-kernel,
linux-security-module, kunit-dev, linux-kselftest, Mike Salvatore
In-Reply-To: <alpine.LRH.2.20.1910191348280.11804@dhcp-10-175-221-34.vpn.oracle.com>
On Sat, Oct 19, 2019 at 01:56:01PM +0100, Alan Maguire wrote:
> On Fri, 18 Oct 2019, Luis Chamberlain wrote:
>
> > On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > > From: Mike Salvatore <mike.salvatore@canonical.com>
> > >
> > > In order to write the tests against the policy unpacking code, some
> > > static functions needed to be exposed for testing purposes. One of the
> > > goals of this patch is to establish a pattern for which testing these
> > > kinds of functions should be done in the future.
> >
> > And you'd run into the same situation expressed elsewhere with kunit of
> > an issue of the kunit test as built-in working but if built as a module
> > then it would not work, given the lack of exports. Symbols namespaces
> > should resolve this [0], and we'd be careful where a driver imports this
> > namespace.
> >
> > [0] https://lwn.net/Articles/798254/
> >
>
> Thanks for the link! Looks interesting for us definitely!
>
> WRT adding tests, I think what we're aiming at is a set of best practices
> to advise test developers using KUnit, while attempting to minimize
> side-effects of any changes we need to make to support testability.
>
> One aspect of this we probably have to consider is inlining of code.
Sure. Makes sense.
Luis
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Alan Maguire @ 2019-10-19 12:56 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Brendan Higgins, Matthias Maennich, shuah, john.johansen, jmorris,
serge, keescook, alan.maguire, yzaikin, davidgow, tytso,
linux-kernel, linux-security-module, kunit-dev, linux-kselftest,
Mike Salvatore
In-Reply-To: <20191018122949.GD11244@42.do-not-panic.com>
On Fri, 18 Oct 2019, Luis Chamberlain wrote:
> On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > From: Mike Salvatore <mike.salvatore@canonical.com>
> >
> > In order to write the tests against the policy unpacking code, some
> > static functions needed to be exposed for testing purposes. One of the
> > goals of this patch is to establish a pattern for which testing these
> > kinds of functions should be done in the future.
>
> And you'd run into the same situation expressed elsewhere with kunit of
> an issue of the kunit test as built-in working but if built as a module
> then it would not work, given the lack of exports. Symbols namespaces
> should resolve this [0], and we'd be careful where a driver imports this
> namespace.
>
> [0] https://lwn.net/Articles/798254/
>
Thanks for the link! Looks interesting for us definitely!
WRT adding tests, I think what we're aiming at is a set of best practices
to advise test developers using KUnit, while attempting to minimize
side-effects of any changes we need to make to support testability.
One aspect of this we probably have to consider is inlining of code. For
cases like this where the functions are small and are called in a small
number of cases, any testability changes we make may push a
previously-inlined function to not be inlined, with potential performance
side-effects for the subsystem. In such cases, I wonder if the right
answer would be to suggest actually defining the functions as
inline in the header file? That way the compiler still gets to decide (as
opposed to __always_inline), and we don't perturb performance too much.
Thanks!
Alan
> Luis
>
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-10-18 21:41 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: shuah, John Johansen, jmorris, serge, Kees Cook, Alan Maguire,
Iurii Zaikin, David Gow, Luis Chamberlain,
Linux Kernel Mailing List, linux-security-module,
KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
Mike Salvatore
In-Reply-To: <20191018162519.GH21137@mit.edu>
On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > > +config SECURITY_APPARMOR_TEST
> > > + bool "Build KUnit tests for policy_unpack.c"
> > > + default n
> > > + depends on KUNIT && SECURITY_APPARMOR
> >
> > Ted, here is an example where doing select on direct dependencies is
> > tricky because SECURITY_APPARMOR has a number of indirect dependencies.
>
> Well, that could be solved by adding a select on all of the indirect
> dependencies. I did get your point about the fact that we could have
In this particular case that would work.
> cases where the indirect dependencies might conflict with one another.
> That's going to be a tough situation regardless of whether we have a
> sat-solver or a human who has to struggle with that situation.
But yeah, that's the real problem.
> It's also going to be a bit sad because it means that we won't be able
> to create a single config that could be used to run all the kunit
> tests when a user pushes a change to a Gerrit server for review. :-/
Yeah...well, we can do the next best thing and generate a set of
kunitconfigs that in sum will run all the tests. Not nearly as nice,
but it's the next best thing, right? If you think about it, it's
really not all that different from the eventual goal of having many
independent test binaries.
> I suppose that if we use a strict definition of "unit tests", and we
> assume that all of the tests impacted by a change in foo/bar/baz.c
> will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can
> automate the generation of the kunitconfig file, perhaps?
Possibly. I have some friends on the TAP team (automated testing team
within Google), and it sounds like that is actually a pretty hard
problem, but something that is at least possible. Still, it would be
nice to have a way to periodically run all the tests.
> The other sad bit about having mutually exclusive config options is
> that we can't easily "run all KUinit tests" for some kind of test
> spinner or zero-day bot.
>
> I'm not sure there's a good solution to that issue, though.
I think, as I mentioned above, the best we can do is probably have a
thing which generates a set of kunitconfigs that in sum will run all
the tests.
Thoughts?
^ permalink raw reply
* Re: [PATCH v0] KEYS: Security LSM Hook for key_create_or_update
From: Lakshmi Ramasubramanian @ 2019-10-18 20:38 UTC (permalink / raw)
To: Casey Schaufler, zohar, dhowells, linux-security-module,
linux-integrity, keyrings, sashal, jamorris
Cc: msft-linux-kernel, prsriva
In-Reply-To: <e5ffe76e-ff9f-7542-2ff7-3ede4f911c2a@schaufler-ca.com>
On 10/18/19 1:25 PM, Casey Schaufler wrote:
>> Problem Statement:
>> key_create_or_update function currently does not have
>> a security LSM hook. The hook is needed to allow security
>> subsystems to use key create or update information.
>
> What security module(s) do you expect to use this?
SELinux is one that I can think of - it has hooks for key_alloc,
key_free, etc. But does not have one for key_create_or_update.
> IMA is not a Linux Security Module.
Agree. But ima utilizes LSM to hook into system operations (such as
read_file given below).
int security_kernel_post_read_file(struct file *file, char *buf,
loff_t size,
enum kernel_read_file_id id)
{
int ret;
ret = call_int_hook(kernel_post_read_file, 0, file,
buf, size, id);
if (ret)
return ret;
return ima_post_read_file(file, buf, size, id);
}
I am currently working on an ima function to measure keys. The change
set I have submitted today is in preparation for that.
> You don't have a security module that provides this hook.
> We don't accept interfaces without users.
Like I have mentioned above, that change in ima will be submitted for
review shortly.
If you have suggestions for a better way to hook into key create\update
that ima can use to measure keys, I'll be happy to investigate that.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v0] KEYS: Security LSM Hook for key_create_or_update
From: Casey Schaufler @ 2019-10-18 20:25 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar, dhowells, linux-security-module,
linux-integrity, keyrings, sashal, jamorris
Cc: msft-linux-kernel, prsriva, casey
In-Reply-To: <20191018195328.6704-1-nramas@linux.microsoft.com>
On 10/18/2019 12:53 PM, Lakshmi Ramasubramanian wrote:
> Problem Statement:
> key_create_or_update function currently does not have
> a security LSM hook. The hook is needed to allow security
> subsystems to use key create or update information.
What security module(s) do you expect to use this?
> security_key_alloc LSM hook that is currently available is not
> providing enough information about the key (the key payload,
> the target keyring, etc.). Also, this LSM hook is only available
> for key create.
>
> Changes made:
> Adding a new LSM hook for key key_create_or_update,
> security_key_create_or_update, which is called after
> => A newly created key is instantiated and linked to the target
> keyring (__key_instantiate_and_link).
> => An existing key is updated with a new payload (__key_update)
>
> security_key_create_or_update is passed the target keyring, key,
> cred, key creation flags, and a boolean flag indicating whether
> the key was newly created or updated.
>
> Security subsystems can use this hook for handling key create or update.
> For example, IMA subsystem can measure the key when it is created or
> updated.
IMA is not a Linux Security Module.
> Testing performed:
> * Booted the kernel with this change.
> * Executed keyctl tests from the Linux Test Project (LTP)
> * Added a new key to a keyring and verified "key create" code path.
> => In this case added a key to builtin_trusted_keys keyring.
> * Added the same key again and verified "key update" code path.
> => Add the same key to builtin_trusted_keys keyring.
> => find_key_to_update found the key and LSM hook was
> called for key update (create flag set to false).
> * Forced the LSM hook (security_key_create_or_update) to
> return an error and verified that the key was not added to
> the keyring ("keyctl list <keyring>" does not list the key).
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> include/linux/lsm_hooks.h | 13 +++++++
> include/linux/security.h | 10 +++++
> security/keys/key.c | 78 ++++++++++++++++++++++++++++++++++-----
> security/security.c | 8 ++++
You don't have a security module that provides this hook.
We don't accept interfaces without users.
> 4 files changed, 100 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index df1318d85f7d..2f2e95df62f3 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1066,6 +1066,15 @@
> *
> * Security hooks affecting all Key Management operations
> *
> + * @key_create_or_update:
> + * Notification of key create or update.
> + * @keyring points to the keyring to which the key belongs
> + * @key points to the key being created or updated
> + * @cred current cred
> + * @flags is the allocation flags
> + * @create flag set to true if the key was created.
> + * flag set to false if the key was updated.
> + * Return 0 if permission is granted, -ve error otherwise.
> * @key_alloc:
> * Permit allocation of a key and assign security data. Note that key does
> * not have a serial number assigned at this point.
> @@ -1781,6 +1790,9 @@ union security_list_options {
>
> /* key management security hooks */
> #ifdef CONFIG_KEYS
> + int (*key_create_or_update)(struct key *keyring, struct key *key,
> + const struct cred *cred,
> + unsigned long flags, bool create);
> int (*key_alloc)(struct key *key, const struct cred *cred,
> unsigned long flags);
> void (*key_free)(struct key *key);
> @@ -2026,6 +2038,7 @@ struct security_hook_heads {
> struct hlist_head xfrm_decode_session;
> #endif /* CONFIG_SECURITY_NETWORK_XFRM */
> #ifdef CONFIG_KEYS
> + struct hlist_head key_create_or_update;
> struct hlist_head key_alloc;
> struct hlist_head key_free;
> struct hlist_head key_permission;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5f7441abbf42..27e1c0a3057b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1672,6 +1672,9 @@ static inline int security_path_chroot(const struct path *path)
> #ifdef CONFIG_KEYS
> #ifdef CONFIG_SECURITY
>
> +int security_key_create_or_update(struct key *keyring, struct key *key,
> + const struct cred *cred,
> + unsigned long flags, bool create);
> int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
> void security_key_free(struct key *key);
> int security_key_permission(key_ref_t key_ref,
> @@ -1680,6 +1683,13 @@ int security_key_getsecurity(struct key *key, char **_buffer);
>
> #else
>
> +int security_key_create_or_update(struct key *keyring, struct key *key,
> + const struct cred *cred,
> + unsigned long flags, bool create)
> +{
> + return 0;
> +}
> +
> static inline int security_key_alloc(struct key *key,
> const struct cred *cred,
> unsigned long flags)
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 764f4c57913e..b913feaf196e 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -781,7 +781,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
> }
>
> /**
> - * key_create_or_update - Update or create and instantiate a key.
> + * __key_create_or_update - Update or create and instantiate a key.
> * @keyring_ref: A pointer to the destination keyring with possession flag.
> * @type: The type of key.
> * @description: The searchable description for the key.
> @@ -789,6 +789,8 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
> * @plen: The length of @payload.
> * @perm: The permissions mask for a new key.
> * @flags: The quota flags for a new key.
> + * @create: Set to true if the key was newly created.
> + * Set to false if the key was updated.
> *
> * Search the destination keyring for a key of the same description and if one
> * is found, update it, otherwise create and instantiate a new one and create a
> @@ -805,13 +807,14 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
> * On success, the possession flag from the keyring ref will be tacked on to
> * the key ref before it is returned.
> */
> -key_ref_t key_create_or_update(key_ref_t keyring_ref,
> - const char *type,
> - const char *description,
> - const void *payload,
> - size_t plen,
> - key_perm_t perm,
> - unsigned long flags)
> +static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
> + const char *type,
> + const char *description,
> + const void *payload,
> + size_t plen,
> + key_perm_t perm,
> + unsigned long flags,
> + bool *create)
> {
> struct keyring_index_key index_key = {
> .description = description,
> @@ -936,6 +939,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
> goto error_link_end;
> }
>
> + *create = true;
> key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
>
> error_link_end:
> @@ -948,7 +952,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
> error:
> return key_ref;
>
> - found_matching_key:
> +found_matching_key:
> /* we found a matching key, so we're going to try to update it
> * - we can drop the locks first as we have the key pinned
> */
> @@ -964,9 +968,65 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
> }
> }
>
> + *create = false;
> key_ref = __key_update(key_ref, &prep);
> goto error_free_prep;
> }
> +
> +/**
> + * key_create_or_update - Update or create and instantiate a key.
> + * @keyring_ref: A pointer to the destination keyring with possession flag.
> + * @type: The type of key.
> + * @description: The searchable description for the key.
> + * @payload: The data to use to instantiate or update the key.
> + * @plen: The length of @payload.
> + * @perm: The permissions mask for a new key.
> + * @flags: The quota flags for a new key.
> + *
> + * Calls the internal function __key_create_or_update.
> + * If successful calls the security LSM hook.
> + */
> +key_ref_t key_create_or_update(key_ref_t keyring_ref,
> + const char *type,
> + const char *description,
> + const void *payload,
> + size_t plen,
> + key_perm_t perm,
> + unsigned long flags)
> +{
> + key_ref_t key_ref;
> + struct key *keyring, *key = NULL;
> + int ret = 0;
> + bool create = false;
> +
> + key_ref = __key_create_or_update(keyring_ref, type, description,
> + payload, plen, perm, flags,
> + &create);
> + if (IS_ERR(key_ref))
> + goto out;
> +
> + keyring = key_ref_to_ptr(keyring_ref);
> + key = key_ref_to_ptr(key_ref);
> +
> + /* let the security module know about
> + * the created or updated key.
> + */
> + ret = security_key_create_or_update(keyring, key,
> + current_cred(),
> + flags, create);
> + if (ret < 0)
> + goto security_error;
> + else
> + goto out;
> +
> +security_error:
> + key_unlink(keyring, key);
> + key_put(key);
> + key_ref = ERR_PTR(ret);
> +
> +out:
> + return key_ref;
> +}
> EXPORT_SYMBOL(key_create_or_update);
>
> /**
> diff --git a/security/security.c b/security/security.c
> index 250ee2d76406..fc1e4984fb53 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2280,6 +2280,14 @@ EXPORT_SYMBOL(security_skb_classify_flow);
>
> #ifdef CONFIG_KEYS
>
> +int security_key_create_or_update(struct key *keyring, struct key *key,
> + const struct cred *cred,
> + unsigned long flags, bool create)
> +{
> + return call_int_hook(key_create_or_update, 0,
> + keyring, key, cred, flags, create);
> +}
> +
> int security_key_alloc(struct key *key, const struct cred *cred,
> unsigned long flags)
> {
^ permalink raw reply
* [PATCH v0] KEYS: Security LSM Hook for key_create_or_update
From: Lakshmi Ramasubramanian @ 2019-10-18 19:53 UTC (permalink / raw)
To: zohar, dhowells, linux-security-module, linux-integrity, keyrings,
sashal, jamorris
Cc: msft-linux-kernel, nramas, prsriva
Problem Statement:
key_create_or_update function currently does not have
a security LSM hook. The hook is needed to allow security
subsystems to use key create or update information.
security_key_alloc LSM hook that is currently available is not
providing enough information about the key (the key payload,
the target keyring, etc.). Also, this LSM hook is only available
for key create.
Changes made:
Adding a new LSM hook for key key_create_or_update,
security_key_create_or_update, which is called after
=> A newly created key is instantiated and linked to the target
keyring (__key_instantiate_and_link).
=> An existing key is updated with a new payload (__key_update)
security_key_create_or_update is passed the target keyring, key,
cred, key creation flags, and a boolean flag indicating whether
the key was newly created or updated.
Security subsystems can use this hook for handling key create or update.
For example, IMA subsystem can measure the key when it is created or
updated.
Testing performed:
* Booted the kernel with this change.
* Executed keyctl tests from the Linux Test Project (LTP)
* Added a new key to a keyring and verified "key create" code path.
=> In this case added a key to builtin_trusted_keys keyring.
* Added the same key again and verified "key update" code path.
=> Add the same key to builtin_trusted_keys keyring.
=> find_key_to_update found the key and LSM hook was
called for key update (create flag set to false).
* Forced the LSM hook (security_key_create_or_update) to
return an error and verified that the key was not added to
the keyring ("keyctl list <keyring>" does not list the key).
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
include/linux/lsm_hooks.h | 13 +++++++
include/linux/security.h | 10 +++++
security/keys/key.c | 78 ++++++++++++++++++++++++++++++++++-----
security/security.c | 8 ++++
4 files changed, 100 insertions(+), 9 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index df1318d85f7d..2f2e95df62f3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1066,6 +1066,15 @@
*
* Security hooks affecting all Key Management operations
*
+ * @key_create_or_update:
+ * Notification of key create or update.
+ * @keyring points to the keyring to which the key belongs
+ * @key points to the key being created or updated
+ * @cred current cred
+ * @flags is the allocation flags
+ * @create flag set to true if the key was created.
+ * flag set to false if the key was updated.
+ * Return 0 if permission is granted, -ve error otherwise.
* @key_alloc:
* Permit allocation of a key and assign security data. Note that key does
* not have a serial number assigned at this point.
@@ -1781,6 +1790,9 @@ union security_list_options {
/* key management security hooks */
#ifdef CONFIG_KEYS
+ int (*key_create_or_update)(struct key *keyring, struct key *key,
+ const struct cred *cred,
+ unsigned long flags, bool create);
int (*key_alloc)(struct key *key, const struct cred *cred,
unsigned long flags);
void (*key_free)(struct key *key);
@@ -2026,6 +2038,7 @@ struct security_hook_heads {
struct hlist_head xfrm_decode_session;
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
#ifdef CONFIG_KEYS
+ struct hlist_head key_create_or_update;
struct hlist_head key_alloc;
struct hlist_head key_free;
struct hlist_head key_permission;
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f7441abbf42..27e1c0a3057b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1672,6 +1672,9 @@ static inline int security_path_chroot(const struct path *path)
#ifdef CONFIG_KEYS
#ifdef CONFIG_SECURITY
+int security_key_create_or_update(struct key *keyring, struct key *key,
+ const struct cred *cred,
+ unsigned long flags, bool create);
int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
void security_key_free(struct key *key);
int security_key_permission(key_ref_t key_ref,
@@ -1680,6 +1683,13 @@ int security_key_getsecurity(struct key *key, char **_buffer);
#else
+int security_key_create_or_update(struct key *keyring, struct key *key,
+ const struct cred *cred,
+ unsigned long flags, bool create)
+{
+ return 0;
+}
+
static inline int security_key_alloc(struct key *key,
const struct cred *cred,
unsigned long flags)
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..b913feaf196e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -781,7 +781,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
}
/**
- * key_create_or_update - Update or create and instantiate a key.
+ * __key_create_or_update - Update or create and instantiate a key.
* @keyring_ref: A pointer to the destination keyring with possession flag.
* @type: The type of key.
* @description: The searchable description for the key.
@@ -789,6 +789,8 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
* @plen: The length of @payload.
* @perm: The permissions mask for a new key.
* @flags: The quota flags for a new key.
+ * @create: Set to true if the key was newly created.
+ * Set to false if the key was updated.
*
* Search the destination keyring for a key of the same description and if one
* is found, update it, otherwise create and instantiate a new one and create a
@@ -805,13 +807,14 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
* On success, the possession flag from the keyring ref will be tacked on to
* the key ref before it is returned.
*/
-key_ref_t key_create_or_update(key_ref_t keyring_ref,
- const char *type,
- const char *description,
- const void *payload,
- size_t plen,
- key_perm_t perm,
- unsigned long flags)
+static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
+ const char *type,
+ const char *description,
+ const void *payload,
+ size_t plen,
+ key_perm_t perm,
+ unsigned long flags,
+ bool *create)
{
struct keyring_index_key index_key = {
.description = description,
@@ -936,6 +939,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
goto error_link_end;
}
+ *create = true;
key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
error_link_end:
@@ -948,7 +952,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
error:
return key_ref;
- found_matching_key:
+found_matching_key:
/* we found a matching key, so we're going to try to update it
* - we can drop the locks first as we have the key pinned
*/
@@ -964,9 +968,65 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
}
+ *create = false;
key_ref = __key_update(key_ref, &prep);
goto error_free_prep;
}
+
+/**
+ * key_create_or_update - Update or create and instantiate a key.
+ * @keyring_ref: A pointer to the destination keyring with possession flag.
+ * @type: The type of key.
+ * @description: The searchable description for the key.
+ * @payload: The data to use to instantiate or update the key.
+ * @plen: The length of @payload.
+ * @perm: The permissions mask for a new key.
+ * @flags: The quota flags for a new key.
+ *
+ * Calls the internal function __key_create_or_update.
+ * If successful calls the security LSM hook.
+ */
+key_ref_t key_create_or_update(key_ref_t keyring_ref,
+ const char *type,
+ const char *description,
+ const void *payload,
+ size_t plen,
+ key_perm_t perm,
+ unsigned long flags)
+{
+ key_ref_t key_ref;
+ struct key *keyring, *key = NULL;
+ int ret = 0;
+ bool create = false;
+
+ key_ref = __key_create_or_update(keyring_ref, type, description,
+ payload, plen, perm, flags,
+ &create);
+ if (IS_ERR(key_ref))
+ goto out;
+
+ keyring = key_ref_to_ptr(keyring_ref);
+ key = key_ref_to_ptr(key_ref);
+
+ /* let the security module know about
+ * the created or updated key.
+ */
+ ret = security_key_create_or_update(keyring, key,
+ current_cred(),
+ flags, create);
+ if (ret < 0)
+ goto security_error;
+ else
+ goto out;
+
+security_error:
+ key_unlink(keyring, key);
+ key_put(key);
+ key_ref = ERR_PTR(ret);
+
+out:
+ return key_ref;
+}
EXPORT_SYMBOL(key_create_or_update);
/**
diff --git a/security/security.c b/security/security.c
index 250ee2d76406..fc1e4984fb53 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2280,6 +2280,14 @@ EXPORT_SYMBOL(security_skb_classify_flow);
#ifdef CONFIG_KEYS
+int security_key_create_or_update(struct key *keyring, struct key *key,
+ const struct cred *cred,
+ unsigned long flags, bool create)
+{
+ return call_int_hook(key_create_or_update, 0,
+ keyring, key, cred, flags, create);
+}
+
int security_key_alloc(struct key *key, const struct cred *cred,
unsigned long flags)
{
--
2.17.1
^ permalink raw reply related
* [PATCH] security/keyring: avoid pagefaults in keyring_read_iterator
From: Chris von Recklinghausen @ 2019-10-18 18:40 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E . Hallyn,
keyrings, linux-security-module, linux-kernel
Cc: Waiman Long, Chris von Recklinghausen
under a debug kernel, the following circular locking dependency was observed:
[ 5896.294840] ======================================================
[ 5896.294846] [ INFO: possible circular locking dependency detected ]
[ 5896.294852] 3.10.0-957.31.1.el7.ppc64le.debug #1 Tainted: G OE ------------ T
[ 5896.294857] -------------------------------------------------------
[ 5896.294863] keyctl/21719 is trying to acquire lock:
[ 5896.294867] (&mm->mmap_sem){++++++}, at: [<c000000000331db8>] might_fault+0x88/0xf0
[ 5896.294881]
[ 5896.294881] but task is already holding lock:
[ 5896.294886] (&type->lock_class){+++++.}, at: [<c0000000004ff504>] keyctl_read_key+0xb4/0x170
[ 5896.294899]
[ 5896.294899] which lock already depends on the new lock.
[ 5896.294899]
[ 5896.294905]
[ 5896.294905] the existing dependency chain (in reverse order) is:
[ 5896.294911]
-> #1 (&type->lock_class){+++++.}:
[ 5896.294920] [<c0000000001caaf4>] check_prevs_add+0x144/0x1d0
[ 5896.294929] [<c0000000001ce338>] lock_acquire+0xe38/0x16c0
[ 5896.294936] [<c000000000b8e5e4>] down_write+0x84/0x130
[ 5896.294943] [<c0000000004fd330>] key_link+0x90/0x2e0
[ 5896.294949] [<c000000000503f44>] call_sbin_request_key+0x154/0x640
[ 5896.294956] [<c000000000bb1424>] construct_key_and_link+0x38c/0x464
[ 5896.294964] [<c000000000504bb4>] request_key+0x214/0x230
[ 5896.294970] [<d0000000047e2490>] nfs_idmap_get_key+0x110/0x460 [nfsv4]
[ 5896.294986] [<d0000000047e3464>] nfs_map_name_to_uid+0x84/0x2f0 [nfsv4]
[ 5896.294999] [<d0000000047c3180>] decode_attr_owner+0x1d0/0x2c0 [nfsv4]
[ 5896.295010] [<d0000000047c6f18>] decode_getfattr_attrs+0x5a8/0xb80 [nfsv4]
[ 5896.295022] [<d0000000047c75cc>] decode_getfattr_generic.constprop.100+0xdc/0x200 [nfsv4]
[ 5896.295033] [<d0000000047c8048>] nfs4_xdr_dec_getattr+0xa8/0xb0 [nfsv4]
[ 5896.295044] [<d0000000035eff58>] rpcauth_unwrap_resp+0xf8/0x150 [sunrpc]
[ 5896.295060] [<d0000000035d357c>] call_decode+0x29c/0x910 [sunrpc]
[ 5896.295071] [<d0000000035eb940>] __rpc_execute+0xf0/0x870 [sunrpc]
[ 5896.295083] [<d0000000035d233c>] rpc_run_task+0x14c/0x1c0 [sunrpc]
[ 5896.295094] [<d0000000047a12f0>] nfs4_call_sync_sequence+0x70/0xb0 [nfsv4]
[ 5896.295105] [<d0000000047a2254>] _nfs4_proc_getattr+0xc4/0xf0 [nfsv4]
[ 5896.295115] [<d0000000047b9ee4>] nfs4_proc_getattr+0x84/0x220 [nfsv4]
[ 5896.295126] [<d00000000454519c>] __nfs_revalidate_inode+0x1cc/0x7a0 [nfs]
[ 5896.295138] [<d000000004546284>] nfs_revalidate_mapping+0x1f4/0x520 [nfs]
[ 5896.295150] [<d00000000453df98>] nfs_file_mmap+0x78/0xb0 [nfs]
[ 5896.295160] [<c000000000343df8>] mmap_region+0x518/0x780
[ 5896.295167] [<c000000000344488>] do_mmap+0x428/0x510
[ 5896.295173] [<c000000000317508>] vm_mmap_pgoff+0x108/0x150
[ 5896.295179] [<c000000000340f1c>] SyS_mmap_pgoff+0xec/0x2c0
[ 5896.295186] [<c0000000000173b8>] sys_mmap+0x78/0x90
[ 5896.295192] [<c00000000000a294>] system_call+0x3c/0x100
[ 5896.295199]
-> #0 (&mm->mmap_sem){++++++}:
[ 5896.295207] [<c0000000001ca990>] check_prev_add+0xa50/0xa70
[ 5896.295214] [<c0000000001caaf4>] check_prevs_add+0x144/0x1d0
[ 5896.295221] [<c0000000001ce338>] lock_acquire+0xe38/0x16c0
[ 5896.295228] [<c000000000331de4>] might_fault+0xb4/0xf0
[ 5896.295235] [<c0000000004fc644>] keyring_read_iterator+0x54/0xd0
[ 5896.295242] [<c00000000060fe98>] assoc_array_subtree_iterate+0x4d8/0x790
[ 5896.295249] [<c0000000004fbc00>] keyring_read+0x80/0xa0
[ 5896.295255] [<c0000000004ff5a4>] keyctl_read_key+0x154/0x170
[ 5896.295262] [<c00000000000a294>] system_call+0x3c/0x100
[ 5896.295269]
[ 5896.295269] other info that might help us debug this:
[ 5896.295275] Possible unsafe locking scenario:
[ 5896.295275]
[ 5896.295281] CPU0 CPU1
[ 5896.295285] ---- ----
[ 5896.295289] lock(&type->lock_class);
[ 5896.295294] lock(&mm->mmap_sem);
[ 5896.295301] lock(&type->lock_class);
[ 5896.295308] lock(&mm->mmap_sem);
[ 5896.295313]
[ 5896.295313] *** DEADLOCK ***
[ 5896.295313]
[ 5896.295320] 1 lock held by keyctl/21719:
[ 5896.295323] #0: (&type->lock_class){+++++.}, at: [<c0000000004ff504>] keyctl_read_key+0xb4/0x170
[ 5896.295337]
[ 5896.295337] stack backtrace:
[ 5896.295343] CPU: 1 PID: 21719 Comm: keyctl Kdump: loaded Tainted: G OE ------------ T 3.10.0-957.31.1.el7.ppc64le.debug #1
[ 5896.295351] Call Trace:
[ 5896.295355] [c00000016100f8e0] [c0000000000205d0] show_stack+0x90/0x390 (unreliable)
[ 5896.295363] [c00000016100f9a0] [c000000000bb37d0] dump_stack+0x30/0x44
[ 5896.295371] [c00000016100f9c0] [c000000000ba7f3c] print_circular_bug+0x36c/0x3a0
[ 5896.295379] [c00000016100fa60] [c0000000001ca990] check_prev_add+0xa50/0xa70
[ 5896.295386] [c00000016100fb60] [c0000000001caaf4] check_prevs_add+0x144/0x1d0
[ 5896.295393] [c00000016100fbb0] [c0000000001ce338] lock_acquire+0xe38/0x16c0
[ 5896.295400] [c00000016100fce0] [c000000000331de4] might_fault+0xb4/0xf0
[ 5896.295407] [c00000016100fd00] [c0000000004fc644] keyring_read_iterator+0x54/0xd0
[ 5896.295415] [c00000016100fd40] [c00000000060fe98] assoc_array_subtree_iterate+0x4d8/0x790
[ 5896.295423] [c00000016100fd90] [c0000000004fbc00] keyring_read+0x80/0xa0
[ 5896.295430] [c00000016100fde0] [c0000000004ff5a4] keyctl_read_key+0x154/0x170
[ 5896.295437] [c00000016100fe30] [c00000000000a294] system_call+0x3c/0x100
The put_user call from keyring_read_iterator caused a page fault which attempts
to lock mm->mmap_sem and type->lock_class (key->sem) in the reverse order that
keyring_read_iterator did, thus causing the circular locking dependency.
Remedy this by using access_ok and __put_user instead of put_user so we'll
return an error instead of faulting in the page.
Also to prevent potential changes in behavior to applications, pre-fault the
page(s) with the key in keyctl_read_key before taking the read semaphore to
ensure that the page is present by the time keyring_read_iterator is called.
Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: Chris von Recklinghausen <crecklin@redhat.com>
---
security/keys/keyctl.c | 10 ++++++++--
security/keys/keyring.c | 7 +++----
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c9..f8a2553 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -846,9 +846,15 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
can_read_key:
ret = -EOPNOTSUPP;
if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
- * to protect against the key being updated or revoked.
+ /*
+ * Read the data with the semaphore held (since we might sleep)
+ * to protect against the key being updated or revoked. The
+ * user buffer, if not mapped yet, will be faulted in to
+ * prevent read failure.
*/
+ key_serial_t tmp;
+
+ get_user(tmp, buffer); /* Prefault */
down_read(&key->sem);
ret = key_validate(key);
if (ret == 0)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c..7cac3c7 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,9 +466,9 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
+ if (!access_ok(ctx->buffer, sizeof(key->serial)) ||
+ __put_user(key->serial, ctx->buffer) < 0)
+ return -EFAULT;
ctx->buffer++;
ctx->count += sizeof(key->serial);
return 0;
--
1.8.3.1
^ permalink raw reply related
* Re: WARNING: refcount bug in find_key_to_update
From: David Howells @ 2019-10-18 16:46 UTC (permalink / raw)
To: Hillf Danton
Cc: dhowells, syzbot, jarkko.sakkinen, jmorris, keyrings,
linux-kernel, linux-security-module, serge, syzkaller-bugs
In-Reply-To: <20191017092428.7336-1-hdanton@sina.com>
Hillf Danton <hdanton@sina.com> wrote:
> - (1 << KEY_FLAG_REVOKED))) {
> + (1 << KEY_FLAG_REVOKED)) || !key_tryget(key)) {
> kleave(" = NULL [x]");
> return NULL;
> }
> - __key_get(key);
That should be ineffective and ought not to fix the bug.
David
^ permalink raw reply
* Re: WARNING: refcount bug in find_key_to_update
From: David Howells @ 2019-10-18 16:45 UTC (permalink / raw)
To: Tetsuo Handa
Cc: dhowells, Eric Biggers, Linus Torvalds, syzbot, keyrings,
LSM List, syzkaller-bugs
In-Reply-To: <b211005b-75de-7936-c97a-817f7100415a@I-love.SAKURA.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> I don't know about keys, but I rather suspect lack of serialization locks
> between "looking up for checking existing keys" versus "looking up for
> garbage collection".
The garbage collector holds key_serial_lock when walking key_serial_tree
looking for keys to destroy.
As the gc is the *only* thing that is permitted to remove a key from
key_serial_tree, it can safely keep a cursor pointer to the node it was
looking at when it drops the lock - and then resume scanning once it has taken
the lock again.
When find_key_to_update() is looking for a key that might be updated, the
caller *must* be holding the destination keyring lock and every key in the
keyring should have at least one ref on it held by the keyring - so none of
them should get destroyed by the garbage collector.
David
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox