* Re: Linux 5.1-rc2
From: Tetsuo Handa @ 2019-03-27 21:05 UTC (permalink / raw)
To: Kees Cook
Cc: James Morris, Randy Dunlap, Linus Torvalds,
Linux List Kernel Mailing, linux-security-module, Jakub Kicinski
In-Reply-To: <CAGXu5jKWECKxksJWPpCkBKG+wB26DhYK=nYBpuuoS+Pv9AsNwQ@mail.gmail.com>
On 2019/03/28 5:45, Kees Cook wrote:
> On Wed, Mar 27, 2019 at 1:30 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> On 2019/03/28 4:16, Kees Cook wrote:
>>> The part I don't understand is what you've said about TOMOYO being
>>> primary and not wanting the others stackable? That kind of goes
>>> against the point, but I'm happy to do that if you want it that way.
>>
>> Automatically enabling multiple legacy major LSMs might result in a confusion like
>> Jakub encountered.
>
> The confusion wasn't multiple enabled: it was a change of what was
> enabled (due to ignoring the old config). (My very first suggested
> patch fixed this...)
Someone else might get confused when TOMOYO is automatically enabled
despite they did not specify TOMOYO in lsm= or security= or CONFIG_LSM.
>
>> For a few releases from 5.1 (about one year or so?), since
>> CONFIG_DEFAULT_SECURITY_* will be ignored after CONFIG_LSM is once defined in
>> their kernel configs, I guess that it is better not to enable TOMOYO automatically
>> until most people complete migrating from CONFIG_DEFAULT_SECURITY_* to CONFIG_LSM
>> and get used to use lsm= kernel command line option rather than security= kernel
>> command line option.
>
> It sounds like you want TOMOYO to stay an exclusive LSM? Should we
> revert a5e2fe7ede12 ("TOMOYO: Update LSM flags to no longer be
> exclusive") instead? (I'm against this idea, but defer to you. I think
> it should stay stackable since the goal is to entirely remove the
> concept of exclusive LSMs.)
I never want to revert a5e2fe7ede12. For transition period, I just don't
want to automatically enable TOMOYO when people did not specify TOMOYO.
>
> I don't see problems for an exclusive LSM user (AA, SELinux, Smack)
> also initializing TOMOYO, though. It should be a no-op. Is there some
> situation where this is not true?
There should be no problem except some TOMOYO messages are printed.
^ permalink raw reply
* Re: Linux 5.1-rc2
From: Kees Cook @ 2019-03-27 20:45 UTC (permalink / raw)
To: Tetsuo Handa
Cc: James Morris, Randy Dunlap, Linus Torvalds,
Linux List Kernel Mailing, linux-security-module
In-Reply-To: <98289cd2-095a-f0cd-e405-887ecbba0030@i-love.sakura.ne.jp>
On Wed, Mar 27, 2019 at 1:30 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/03/28 4:16, Kees Cook wrote:
> > The part I don't understand is what you've said about TOMOYO being
> > primary and not wanting the others stackable? That kind of goes
> > against the point, but I'm happy to do that if you want it that way.
>
> Automatically enabling multiple legacy major LSMs might result in a confusion like
> Jakub encountered.
The confusion wasn't multiple enabled: it was a change of what was
enabled (due to ignoring the old config). (My very first suggested
patch fixed this...)
> For a few releases from 5.1 (about one year or so?), since
> CONFIG_DEFAULT_SECURITY_* will be ignored after CONFIG_LSM is once defined in
> their kernel configs, I guess that it is better not to enable TOMOYO automatically
> until most people complete migrating from CONFIG_DEFAULT_SECURITY_* to CONFIG_LSM
> and get used to use lsm= kernel command line option rather than security= kernel
> command line option.
It sounds like you want TOMOYO to stay an exclusive LSM? Should we
revert a5e2fe7ede12 ("TOMOYO: Update LSM flags to no longer be
exclusive") instead? (I'm against this idea, but defer to you. I think
it should stay stackable since the goal is to entirely remove the
concept of exclusive LSMs.)
I don't see problems for an exclusive LSM user (AA, SELinux, Smack)
also initializing TOMOYO, though. It should be a no-op. Is there some
situation where this is not true?
The situation you helped me see was that a TOMOYO user with
CONFIG_DEFAULT_SECURITY_TOMOYO would not want to see any exclusive LSM
also initialized, since that may NOT be a no-op.
So, AFAICT, my proposal fixes both Jakub's issue
(CONFIG_DEFAULT_SECURITY_* oldconfig entirely ignored) and Randy's
issue (subset of Jakub's: choosing DAC should mean no legacy major
initializes), and the "TOMOYO user surprised to see an exclusive LSM
also initialized". If you're happy with the proposed change in my
prior email, I'll send it properly to James. If not, what do you see
that needs changing?
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply
* Re: Linux 5.1-rc2
From: Tetsuo Handa @ 2019-03-27 20:30 UTC (permalink / raw)
To: Kees Cook
Cc: James Morris, Randy Dunlap, Linus Torvalds,
Linux List Kernel Mailing, linux-security-module
In-Reply-To: <CAGXu5jJZ1H1RFMFBxPsoBW+T17ya+50-d7-31Gr9ciRgpCOaDA@mail.gmail.com>
On 2019/03/28 4:16, Kees Cook wrote:
> The part I don't understand is what you've said about TOMOYO being
> primary and not wanting the others stackable? That kind of goes
> against the point, but I'm happy to do that if you want it that way.
Automatically enabling multiple legacy major LSMs might result in a confusion like
Jakub encountered. For a few releases from 5.1 (about one year or so?), since
CONFIG_DEFAULT_SECURITY_* will be ignored after CONFIG_LSM is once defined in
their kernel configs, I guess that it is better not to enable TOMOYO automatically
until most people complete migrating from CONFIG_DEFAULT_SECURITY_* to CONFIG_LSM
and get used to use lsm= kernel command line option rather than security= kernel
command line option.
^ permalink raw reply
* [PATCH AUTOSEL 5.0 010/262] apparmor: fix double free when unpack of secmark rules fails
From: Sasha Levin @ 2019-03-27 17:57 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20190327180158.10245-1-sashal@kernel.org>
From: John Johansen <john.johansen@canonical.com>
[ Upstream commit d8dbb581d4f86a2ac669c056fc71a28ebeb367f4 ]
if secmark rules fail to unpack a double free happens resulting in
the following oops
[ 1295.584074] audit: type=1400 audit(1549970525.256:51): apparmor="STATUS" info="failed to unpack profile secmark rules" error=-71 profile="unconfined" name="/root/test" pid=29882 comm="apparmor_parser" name="/root/test" offset=120
[ 1374.042334] ------------[ cut here ]------------
[ 1374.042336] kernel BUG at mm/slub.c:294!
[ 1374.042404] invalid opcode: 0000 [#1] SMP PTI
[ 1374.042436] CPU: 0 PID: 29921 Comm: apparmor_parser Not tainted 4.20.7-042007-generic #201902061234
[ 1374.042461] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 1374.042489] RIP: 0010:kfree+0x164/0x180
[ 1374.042502] Code: 74 05 41 0f b6 72 51 4c 89 d7 e8 37 cd f8 ff eb 8b 41 b8 01 00 00 00 48 89 d9 48 89 da 4c 89 d6 e8 11 f6 ff ff e9 72 ff ff ff <0f> 0b 49 8b 42 08 a8 01 75 c2 0f 0b 48 8b 3d a9 f4 19 01 e9 c5 fe
[ 1374.042552] RSP: 0018:ffffaf7b812d7b90 EFLAGS: 00010246
[ 1374.042568] RAX: ffff91e437679200 RBX: ffff91e437679200 RCX: ffff91e437679200
[ 1374.042589] RDX: 00000000000088b6 RSI: ffff91e43da27060 RDI: ffff91e43d401a80
[ 1374.042609] RBP: ffffaf7b812d7ba8 R08: 0000000000027080 R09: ffffffffa6627a6d
[ 1374.042629] R10: ffffd3af41dd9e40 R11: ffff91e43a1740dc R12: ffff91e3f52e8000
[ 1374.042650] R13: ffffffffa6627a6d R14: ffffffffffffffb9 R15: 0000000000000001
[ 1374.042675] FS: 00007f928df77740(0000) GS:ffff91e43da00000(0000) knlGS:0000000000000000
[ 1374.042697] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1374.042714] CR2: 000055a0c3ab6b50 CR3: 0000000079ed8004 CR4: 0000000000360ef0
[ 1374.042737] Call Trace:
[ 1374.042750] kzfree+0x2d/0x40
[ 1374.042763] aa_free_profile+0x12b/0x270
[ 1374.042776] unpack_profile+0xc1/0xf10
[ 1374.042790] aa_unpack+0x115/0x4e0
[ 1374.042802] aa_replace_profiles+0x8e/0xcc0
[ 1374.042817] ? kvmalloc_node+0x6d/0x80
[ 1374.042831] ? __check_object_size+0x166/0x192
[ 1374.042845] policy_update+0xcf/0x1b0
[ 1374.042858] profile_load+0x7d/0xa0
[ 1374.042871] __vfs_write+0x3a/0x190
[ 1374.042883] ? apparmor_file_permission+0x1a/0x20
[ 1374.042899] ? security_file_permission+0x31/0xc0
[ 1374.042918] ? _cond_resched+0x19/0x30
[ 1374.042931] vfs_write+0xab/0x1b0
[ 1374.042963] ksys_write+0x55/0xc0
[ 1374.043004] __x64_sys_write+0x1a/0x20
[ 1374.043046] do_syscall_64+0x5a/0x110
[ 1374.043087] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: 9caafbe2b4cf ("apparmor: Parse secmark policy")
Reported-by: Alex Murray <alex.murray@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/policy_unpack.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 379682e2a8d5..f6c2bcb2ab14 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -579,6 +579,7 @@ static bool unpack_secmark(struct aa_ext *e, struct aa_profile *profile)
kfree(profile->secmark[i].label);
kfree(profile->secmark);
profile->secmark_count = 0;
+ profile->secmark = NULL;
}
e->pos = pos;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 2/2 v2] efi: print appropriate status message when loading certificates
From: Mimi Zohar @ 2019-03-27 19:23 UTC (permalink / raw)
To: Lee, Chun-Yi, Ard Biesheuvel, James Morris, Serge E . Hallyn,
David Howells, Josh Boyer, Nayna Jain
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi
In-Reply-To: <20190324002621.3551-2-jlee@suse.com>
On Sun, 2019-03-24 at 08:26 +0800, Lee, Chun-Yi wrote:
> When loading certificates list from UEFI variable, the original error
> message direct shows the efi status code from UEFI firmware. It looks
> ugly:
>
> [ 2.335031] Couldn't get size: 0x800000000000000e
> [ 2.335032] Couldn't get UEFI MokListRT
> [ 2.339985] Couldn't get size: 0x800000000000000e
> [ 2.339987] Couldn't get UEFI dbx list
>
> So, this patch shows the status string instead of status code.
>
> On the other hand, the "Couldn't get UEFI" message doesn't need
> to be exposed when db/dbx/mok variable do not exist. So, this
> patch set the message level to debug.
>
> v2.
> Setting the MODSIGN messagse level to debug.
>
> Link: https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> Cc: James Morris <jmorris@namei.org>
> Cc: Serge E. Hallyn" <serge@hallyn.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Nayna Jain <nayna@linux.ibm.com>
> Cc: Josh Boyer <jwboyer@fedoraproject.org>
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> security/integrity/platform_certs/load_uefi.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832b..e65244b31f04 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>
> status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> if (status != EFI_BUFFER_TOO_SMALL) {
> - pr_err("Couldn't get size: 0x%lx\n", status);
> + if (status != EFI_NOT_FOUND)
> + pr_err("Couldn't get size: %s\n",
> + efi_status_to_str(status));
> return NULL;
> }
>
> @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> status = efi.get_variable(name, guid, NULL, &lsize, db);
> if (status != EFI_SUCCESS) {
> kfree(db);
> - pr_err("Error reading db var: 0x%lx\n", status);
> + pr_err("Error reading db var: %s\n",
> + efi_status_to_str(status));
> return NULL;
> }
>
> @@ -155,7 +158,7 @@ static int __init load_uefi_certs(void)
> if (!uefi_check_ignore_db()) {
> db = get_cert_list(L"db", &secure_var, &dbsize);
> if (!db) {
> - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> + pr_debug("MODSIGN: Couldn't get UEFI db list\n");
Sure, this is fine.
> } else {
> rc = parse_efi_signature_list("UEFI:db",
> db, dbsize, get_handler_for_db);
> @@ -168,7 +171,7 @@ static int __init load_uefi_certs(void)
>
> mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> if (!mok) {
> - pr_info("Couldn't get UEFI MokListRT\n");
> + pr_debug("Couldn't get UEFI MokListRT\n");
This is fine too.
> } else {
> rc = parse_efi_signature_list("UEFI:MokListRT",
> mok, moksize, get_handler_for_db);
> @@ -179,7 +182,7 @@ static int __init load_uefi_certs(void)
>
> dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> if (!dbx) {
> - pr_info("Couldn't get UEFI dbx list\n");
> + pr_debug("Couldn't get UEFI dbx list\n");
If there isn't a db or moklist, then this is fine. My concern is not
having an indication that the dbx wasn't installed, when it should
have been.
Perhaps similar to the "Loading compiled-in X.509 certificates"
informational message there should informational messages for db, mok,
and dbx as well.
Mimi
> } else {
> rc = parse_efi_signature_list("UEFI:dbx",
> dbx, dbxsize,
^ permalink raw reply
* Re: Linux 5.1-rc2
From: Kees Cook @ 2019-03-27 19:16 UTC (permalink / raw)
To: Tetsuo Handa
Cc: James Morris, Randy Dunlap, Linus Torvalds,
Linux List Kernel Mailing, linux-security-module
In-Reply-To: <8811b2e4-28e1-2f01-024b-fb7d0196483f@i-love.sakura.ne.jp>
On Mon, Mar 25, 2019 at 2:06 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/03/26 4:08, James Morris wrote:
> > On Sun, 24 Mar 2019, Randy Dunlap wrote:
> >
> >> On 3/24/19 2:26 PM, Linus Torvalds wrote:
> >>> Well, we're a week away from the merge window close, and here's rc2.
> >>> Things look fairly normal, but honestly, rc2 is usually too early to
> >>> tell. People haven't necessarily had time to notice problems yet.
> >>> Which is just another way of saying "please test harder".
> >>>
> >>> Nothing particularly stands out. Yes, we had some fixes for the new
> >>> io_ring code for issues that were discussed when merging it. Other
> >>> than that, worth noting is that the bulk of the patches are for
> >>> tooling, not the core kernel. In fact, about two thirds of the patch
> >>> is just for the tools/ subdirectory, most of it due to some late perf
> >>> tool updates. The people involved promise they're done.
> >>
> >> Hmph. I'm still looking for the patch that restores the various
> >> CONFIG_DEFAULT_<security> kconfig options to be merged.
> >>
> >> https://lore.kernel.org/linux-security-module/2bf23acd-22c4-a260-7648-845887a409d5@i-love.sakura.ne.jp/
> >>
> >> since commit 70b62c25665f636c9f6c700b26af7df296b0887e dropped them somehow.
> >
> > AFAICT we don't have a finalized version of the patch yet.
> >
> > Kees?
Sorry for the delay -- back from travel now.
> As far as I can tell, Kees's comment
>
> It breaks the backward-compat for the "security=" line. If a system is
> booted with CONFIG_LSM="minors...,apparmor" and "security=selinux",
> neither apparmor nor selinux will be initialized. The logic on
> "security=..." depends on the other LSMs being present in the list.
>
> was just a confusion
Yes, you are correct here. This is what I get for drive-by comments
while travelling. :) However, I don't like that it creates an
incomplete LSM list for no reason. I'd like CONFIG_LSM to be built in
a way that future stack-enabling will Just Work. Leaving off LSMs
means it won't. My original patch doesn't change the behavior relative
to the old configs (i.e. the CONFIG_DEFAULT_SECURITY_* will still be
selected and turn off the others) but does allow the other LSMs to be
initialized in the future once earlier ones in the list become
stackable.
The part I don't understand is what you've said about TOMOYO being
primary and not wanting the others stackable? That kind of goes
against the point, but I'm happy to do that if you want it that way.
If so, my current proposal would be:
config LSM
string "Ordered list of enabled LSMs"
+ default
"yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if
DEFAULT_SECURITY_SMACK
+ default
"yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if
DEFAULT_SECURITY_APPARMOR
+ default "yama,loadpin,safesetid,integrity,tomoyo" if
DEFAULT_SECURITY_TOMOYO
+ default "yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
default "yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
Note that the last default line holds for both "new build" and
"selinux chosen". The other change from my earlier patch is that _DAC
must turn off all the legacy major LSMs to get the behavior Randy was
expecting. Shall I send a patch that does the above, or is there
another wrinkle?
Thanks!
-Kees
> the finalized version.
>
> From 72f5f21b800c87f9ec3600f6e3acfb654690d8f0 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 26 Mar 2019 05:56:30 +0900
> Subject: [PATCH] LSM: Revive CONFIG_DEFAULT_SECURITY_* for "make oldconfig"
>
> Commit 70b62c25665f636c ("LoadPin: Initialize as ordered LSM") removed
> CONFIG_DEFAULT_SECURITY_{SELINUX,SMACK,TOMOYO,APPARMOR,DAC} from
> security/Kconfig and changed CONFIG_LSM to provide a fixed ordering as a
> default value. That commit expected that existing users (upgrading from
> Linux 5.0 and earlier) will edit CONFIG_LSM value in accordance with
> their CONFIG_DEFAULT_SECURITY_* choice in their old kernel configs. But
> since users might forget to edit CONFIG_LSM value, this patch revives
> the choice (only for providing the default value for CONFIG_LSM) in order
> to make sure that CONFIG_LSM reflects CONFIG_DEFAULT_SECURITY_* from their
> old kernel configs.
>
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> security/Kconfig | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 1d6463f..2f29805 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -239,9 +239,44 @@ source "security/safesetid/Kconfig"
>
> source "security/integrity/Kconfig"
>
> +choice
> + prompt "Default security module [superseded by 'Ordered list of enabled LSMs' below]"
> + default DEFAULT_SECURITY_SELINUX if SECURITY_SELINUX
> + default DEFAULT_SECURITY_SMACK if SECURITY_SMACK
> + default DEFAULT_SECURITY_TOMOYO if SECURITY_TOMOYO
> + default DEFAULT_SECURITY_APPARMOR if SECURITY_APPARMOR
> + default DEFAULT_SECURITY_DAC
> +
> + help
> + This choice is there only for converting CONFIG_DEFAULT_SECURITY in old
> + kernel config to CONFIG_LSM in new kernel config. Don't change this choice
> + unless you are creating a fresh kernel config, for this choice will be
> + ignored after CONFIG_LSM is once defined.
> +
> + config DEFAULT_SECURITY_SELINUX
> + bool "SELinux" if SECURITY_SELINUX=y
> +
> + config DEFAULT_SECURITY_SMACK
> + bool "Simplified Mandatory Access Control" if SECURITY_SMACK=y
> +
> + config DEFAULT_SECURITY_TOMOYO
> + bool "TOMOYO" if SECURITY_TOMOYO=y
> +
> + config DEFAULT_SECURITY_APPARMOR
> + bool "AppArmor" if SECURITY_APPARMOR=y
> +
> + config DEFAULT_SECURITY_DAC
> + bool "Unix Discretionary Access Controls"
> +
> +endchoice
> +
> config LSM
> string "Ordered list of enabled LSMs"
> - default "yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> + default "yama,loadpin,safesetid,integrity,selinux" if DEFAULT_SECURITY_SELINUX
> + default "yama,loadpin,safesetid,integrity,smack" if DEFAULT_SECURITY_SMACK
> + default "yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> + default "yama,loadpin,safesetid,integrity,apparmor" if DEFAULT_SECURITY_APPARMOR
> + default "yama,loadpin,safesetid,integrity"
> help
> A comma-separated list of LSMs, in initialization order.
> Any LSMs left off this list will be ignored. This can be
> --
> 1.8.3.1
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 1/2] efi: add a function for transferring status to string
From: Mimi Zohar @ 2019-03-27 19:04 UTC (permalink / raw)
To: Ard Biesheuvel, Lee, Chun-Yi
Cc: James Morris, Serge E . Hallyn, David Howells, Josh Boyer,
Nayna Jain, linux-efi, linux-security-module,
Linux Kernel Mailing List, Lee, Chun-Yi, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <CAKv+Gu9B6h3zGVTctepd1GE5LaaMaT5x14Le1v05AOhbH36bpQ@mail.gmail.com>
On Wed, 2019-03-27 at 19:58 +0100, Ard Biesheuvel wrote:
> On Sun, 24 Mar 2019 at 01:26, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > This function can be used to transfer EFI status code to string
> > for printing out debug message. Using this function can improve
> > the readability of log.
Maybe instead of "for transferring status" use "to convert the status
value to a" in the Subject line and here in the patch description.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Anton Vorontsov <anton@enomsg.org>
> > Cc: Colin Cross <ccross@android.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > ---
> > include/linux/efi.h | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 54357a258b35..a43cb0dc37af 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1768,4 +1768,32 @@ struct linux_efi_memreserve {
> > #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
> > / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
> >
> > +#define EFI_STATUS_STR(_status) \
> > +case EFI_##_status: \
> > + return "EFI_" __stringify(_status);
> > +
> > +static inline char *
> > +efi_status_to_str(efi_status_t status)
> > +{
> > + switch (status) {
> > + EFI_STATUS_STR(SUCCESS)
> > + EFI_STATUS_STR(LOAD_ERROR)
> > + EFI_STATUS_STR(INVALID_PARAMETER)
> > + EFI_STATUS_STR(UNSUPPORTED)
> > + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> > + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> > + EFI_STATUS_STR(NOT_READY)
> > + EFI_STATUS_STR(DEVICE_ERROR)
> > + EFI_STATUS_STR(WRITE_PROTECTED)
> > + EFI_STATUS_STR(OUT_OF_RESOURCES)
> > + EFI_STATUS_STR(NOT_FOUND)
> > + EFI_STATUS_STR(ABORTED)
> > + EFI_STATUS_STR(SECURITY_VIOLATION)
> > + default:
> > + pr_warn("Unknown efi status: 0x%lx", status);
> > + }
> > +
> > + return "Unknown efi status";
> > +}
> > +
> > #endif /* _LINUX_EFI_H */
> > --
> > 2.16.4
> >
>
> Please turn this into a proper function so that not every calling
> object has to duplicate all these strings.
Hi Ard,
Keeping the status values and strings in sync is difficult. I was
going to suggest moving the macro immediately after the status value
definitions.
Mimi
^ permalink raw reply
* Re: [PATCH 1/2] efi: add a function for transferring status to string
From: Ard Biesheuvel @ 2019-03-27 18:58 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: James Morris, Serge E . Hallyn, David Howells, Josh Boyer,
Nayna Jain, Mimi Zohar, linux-efi, linux-security-module,
Linux Kernel Mailing List, Lee, Chun-Yi, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <20190324002621.3551-1-jlee@suse.com>
On Sun, 24 Mar 2019 at 01:26, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>
> This function can be used to transfer EFI status code to string
> for printing out debug message. Using this function can improve
> the readability of log.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> include/linux/efi.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 54357a258b35..a43cb0dc37af 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1768,4 +1768,32 @@ struct linux_efi_memreserve {
> #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
> / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
>
> +#define EFI_STATUS_STR(_status) \
> +case EFI_##_status: \
> + return "EFI_" __stringify(_status);
> +
> +static inline char *
> +efi_status_to_str(efi_status_t status)
> +{
> + switch (status) {
> + EFI_STATUS_STR(SUCCESS)
> + EFI_STATUS_STR(LOAD_ERROR)
> + EFI_STATUS_STR(INVALID_PARAMETER)
> + EFI_STATUS_STR(UNSUPPORTED)
> + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> + EFI_STATUS_STR(NOT_READY)
> + EFI_STATUS_STR(DEVICE_ERROR)
> + EFI_STATUS_STR(WRITE_PROTECTED)
> + EFI_STATUS_STR(OUT_OF_RESOURCES)
> + EFI_STATUS_STR(NOT_FOUND)
> + EFI_STATUS_STR(ABORTED)
> + EFI_STATUS_STR(SECURITY_VIOLATION)
> + default:
> + pr_warn("Unknown efi status: 0x%lx", status);
> + }
> +
> + return "Unknown efi status";
> +}
> +
> #endif /* _LINUX_EFI_H */
> --
> 2.16.4
>
Please turn this into a proper function so that not every calling
object has to duplicate all these strings.
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 18:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Linux API, Matthew Garrett
In-Reply-To: <CALCETrUW=r=xyr4pGiGnn9zUEevf=z6PP3111Ds6qguV5SjYUQ@mail.gmail.com>
On Wed, Mar 27, 2019 at 10:39:53AM -0700, Andy Lutomirski wrote:
> On Tue, Mar 26, 2019 at 10:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Mar 26, 2019 at 10:29:41PM -0700, Andy Lutomirski wrote:
> > >
> > >
> > > > On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > >> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> > > >>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >>>
> > > >>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > > >>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > > >>>> <matthewgarrett@google.com> wrote:
> > > >>>>>
> > > >>>>> From: Matthew Garrett <mjg59@google.com>
> > > >>>>>
> > > >>>>> debugfs has not been meaningfully audited in terms of ensuring that
> > > >>>>> userland cannot trample over the kernel. At Greg's request, disable
> > > >>>>> access to it entirely when the kernel is locked down. This is done at
> > > >>>>> open() time rather than init time as the kernel lockdown status may be
> > > >>>>> made stricter at runtime.
> > > >>>>
> > > >>>> Ugh. Some of those files are very useful. Could this perhaps still
> > > >>>> allow O_RDONLY if we're in INTEGRITY mode?
> > > >>>
> > > >>> Useful for what? Debugging, sure, but for "normal operation", no kernel
> > > >>> functionality should ever require debugfs. If it does, that's a bug and
> > > >>> should be fixed.
> > > >>>
> > > >>
> > > >> I semi-regularly read files in debugfs to diagnose things, and I think
> > > >> it would be good for this to work on distro kernels.
> > > >
> > > > Doing that for debugging is wonderful. People who want this type of
> > > > "lock down" are trading potential security for diagnositic ability.
> > > >
> > >
> > > I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
> >
> > Reading a debugfs file can expose loads of things that can help take
> > over a kernel, or at least make it easier. Pointer addresses, internal
> > system state, loads of other fun things. And before 4.14 or so, it was
> > pretty trivial to use it to oops the kernel as well (not an issue here
> > anymore, but people are right to be nervous).
> >
> > Personally, I think these are all just "confidentiality" type things,
> > but who really knows given the wild-west nature of debugfs (which is as
> > designed). And given that I think this patch series just crazy anyway,
> > I really don't care :)
> >
>
> As far as I'm concerned, preventing root from crashing the system
> should not be a design goal of lockdown at all. And I think that the
> "integrity" mode should be as non-annoying as possible, so I think we
> should allow reading from debugfs.
Sorry, the "crash the system" is not the issue here. The issue is if
everyone can "ensure" that "everything" in debugfs is "safe" for this
mode of "lock down". Given that no one has any idea of what really is
in debugfs, and to try to compare that with the design goals of what
"lock down" really is trying to achive, I think the goal of just giving
up and restricting access is fine if that makes people feel better about
this whole thing.
If this is locked down, it is going to cause distros more pain in
debugging user's issues, but that's their choice, not mine :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 18:29 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andy Lutomirski, James Morris, LSM List, LKML, David Howells,
Linux API
In-Reply-To: <CACdnJus=zXgmNas+XVkWfe+ohAHEGMYLtfuYe0TH-eE=4p0oRw@mail.gmail.com>
On Wed, Mar 27, 2019 at 10:42:18AM -0700, Matthew Garrett wrote:
> On Wed, Mar 27, 2019 at 10:40 AM Andy Lutomirski <luto@kernel.org> wrote:
> > As far as I'm concerned, preventing root from crashing the system
> > should not be a design goal of lockdown at all. And I think that the
> > "integrity" mode should be as non-annoying as possible, so I think we
> > should allow reading from debugfs.
>
> I have no horse in this game - I'm happy to bring back the previous
> approach for integrity mode and block reads entirely in
> confidentiality mode, but I'd rather not spend another release cycle
> arguing about it.
I really do not care either way about any of this :)
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Matthew Garrett @ 2019-03-27 17:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg KH, James Morris, LSM List, LKML, David Howells, Linux API
In-Reply-To: <CALCETrUW=r=xyr4pGiGnn9zUEevf=z6PP3111Ds6qguV5SjYUQ@mail.gmail.com>
On Wed, Mar 27, 2019 at 10:40 AM Andy Lutomirski <luto@kernel.org> wrote:
> As far as I'm concerned, preventing root from crashing the system
> should not be a design goal of lockdown at all. And I think that the
> "integrity" mode should be as non-annoying as possible, so I think we
> should allow reading from debugfs.
I have no horse in this game - I'm happy to bring back the previous
approach for integrity mode and block reads entirely in
confidentiality mode, but I'd rather not spend another release cycle
arguing about it.
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 17:39 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327053342.GA17484@kroah.com>
On Tue, Mar 26, 2019 at 10:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2019 at 10:29:41PM -0700, Andy Lutomirski wrote:
> >
> >
> > > On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > >> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> > >>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >>>
> > >>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > >>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > >>>> <matthewgarrett@google.com> wrote:
> > >>>>>
> > >>>>> From: Matthew Garrett <mjg59@google.com>
> > >>>>>
> > >>>>> debugfs has not been meaningfully audited in terms of ensuring that
> > >>>>> userland cannot trample over the kernel. At Greg's request, disable
> > >>>>> access to it entirely when the kernel is locked down. This is done at
> > >>>>> open() time rather than init time as the kernel lockdown status may be
> > >>>>> made stricter at runtime.
> > >>>>
> > >>>> Ugh. Some of those files are very useful. Could this perhaps still
> > >>>> allow O_RDONLY if we're in INTEGRITY mode?
> > >>>
> > >>> Useful for what? Debugging, sure, but for "normal operation", no kernel
> > >>> functionality should ever require debugfs. If it does, that's a bug and
> > >>> should be fixed.
> > >>>
> > >>
> > >> I semi-regularly read files in debugfs to diagnose things, and I think
> > >> it would be good for this to work on distro kernels.
> > >
> > > Doing that for debugging is wonderful. People who want this type of
> > > "lock down" are trading potential security for diagnositic ability.
> > >
> >
> > I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
>
> Reading a debugfs file can expose loads of things that can help take
> over a kernel, or at least make it easier. Pointer addresses, internal
> system state, loads of other fun things. And before 4.14 or so, it was
> pretty trivial to use it to oops the kernel as well (not an issue here
> anymore, but people are right to be nervous).
>
> Personally, I think these are all just "confidentiality" type things,
> but who really knows given the wild-west nature of debugfs (which is as
> designed). And given that I think this patch series just crazy anyway,
> I really don't care :)
>
As far as I'm concerned, preventing root from crashing the system
should not be a design goal of lockdown at all. And I think that the
"integrity" mode should be as non-annoying as possible, so I think we
should allow reading from debugfs.
^ permalink raw reply
* Re: [PATCH v3] KEYS: trusted: allow trusted.ko to initialize w/o a TPM
From: Dan Williams @ 2019-03-27 17:15 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, linux-security-module, James Morris,
James Bottomley, Mimi Zohar, David Howells, Serge E. Hallyn,
open list:KEYS-TRUSTED, open list
In-Reply-To: <20190326121158.13499-1-jarkko.sakkinen@linux.intel.com>
On Tue, Mar 26, 2019 at 5:13 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> Allow trusted.ko to initialize w/o a TPM. This commit adds checks to the
> key type callbacks and exported functions to fail when a TPM is not
> available.
>
> Cc: James Morris <jmorris@namei.org>
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Tested-by: Dan Williams <dan.j.williams@intel.com>
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> v3:
> - remove unnecessary check for chip in init_trusted()
v3 also tests ok here.
^ permalink raw reply
* Re: [PATCH V31 19/25] x86/mmiotrace: Lock down the testmmiotrace module
From: Matthew Garrett @ 2019-03-27 16:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells,
Linux API, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86
In-Reply-To: <20190327115749.5770a102@gandalf.local.home>
On Wed, Mar 27, 2019 at 8:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 26 Mar 2019 11:27:35 -0700
> Matthew Garrett <matthewgarrett@google.com> wrote:
>
> > From: David Howells <dhowells@redhat.com>
> >
> > The testmmiotrace module shouldn't be permitted when the kernel is locked
> > down as it can be used to arbitrarily read and write MMIO space. This is
> > a runtime check rather than buildtime in order to allow configurations
> > where the same kernel may be run in both locked down or permissive modes
> > depending on local policy.
> >
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> I'm curious. Should there be a mode to lockdown the tracefs directory
> too? As that can expose addresses.
That sounds like a reasonable thing to do in the confidentiality mode,
I don't think it'd be necessary in the integrity mode.
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: James Morris @ 2019-03-27 16:53 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Andy Lutomirski, Matthew Garrett, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327053342.GA17484@kroah.com>
On Wed, 27 Mar 2019, Greg KH wrote:
> Personally, I think these are all just "confidentiality" type things,
> but who really knows given the wild-west nature of debugfs (which is as
> designed). And given that I think this patch series just crazy anyway,
> I really don't care :)
Why do you think it's crazy?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V31 19/25] x86/mmiotrace: Lock down the testmmiotrace module
From: Steven Rostedt @ 2019-03-27 15:57 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, dhowells, linux-api,
luto, Thomas Gleixner, Matthew Garrett, Ingo Molnar,
H. Peter Anvin, x86
In-Reply-To: <20190326182742.16950-20-matthewgarrett@google.com>
On Tue, 26 Mar 2019 11:27:35 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> From: David Howells <dhowells@redhat.com>
>
> The testmmiotrace module shouldn't be permitted when the kernel is locked
> down as it can be used to arbitrarily read and write MMIO space. This is
> a runtime check rather than buildtime in order to allow configurations
> where the same kernel may be run in both locked down or permissive modes
> depending on local policy.
>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
I'm curious. Should there be a mode to lockdown the tracefs directory
too? As that can expose addresses.
-- Steve
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: David Howells <dhowells@redhat.com
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Steven Rostedt <rostedt@goodmis.org>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: "H. Peter Anvin" <hpa@zytor.com>
> cc: x86@kernel.org
> ---
> arch/x86/mm/testmmiotrace.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c
> index f6ae6830b341..9e8ad665f354 100644
> --- a/arch/x86/mm/testmmiotrace.c
> +++ b/arch/x86/mm/testmmiotrace.c
> @@ -115,6 +115,9 @@ static int __init init(void)
> {
> unsigned long size = (read_far) ? (8 << 20) : (16 << 10);
>
> + if (kernel_is_locked_down("MMIO trace testing", LOCKDOWN_INTEGRITY))
> + return -EPERM;
> +
> if (mmio_address == 0) {
> pr_err("you have to use the module argument mmio_address.\n");
> pr_err("DO NOT LOAD THIS MODULE UNLESS YOU REALLY KNOW WHAT YOU ARE DOING!\n");
^ permalink raw reply
* [PATCH] keys: safe concurrent user->{session,uid}_keyring access
From: Jann Horn @ 2019-03-27 15:55 UTC (permalink / raw)
To: David Howells, jannh
Cc: James Morris, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
The current code can perform concurrent updates and reads on
user->session_keyring and user->uid_keyring. Add a comment to
struct user_struct to document the nontrivial locking semantics, and use
READ_ONCE() for unlocked readers and smp_store_release() for writers to
prevent memory ordering issues.
Fixes: 69664cf16af4 ("keys: don't generate user and user session keyrings unless they're accessed")
Signed-off-by: Jann Horn <jannh@google.com>
---
include/linux/sched/user.h | 7 +++++++
security/keys/process_keys.c | 31 +++++++++++++++++--------------
security/keys/request_key.c | 5 +++--
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index c7b5f86b91a1..468d2565a9fe 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -31,6 +31,13 @@ struct user_struct {
atomic_long_t pipe_bufs; /* how many pages are allocated in pipe buffers */
#ifdef CONFIG_KEYS
+ /*
+ * These pointers can only change from NULL to a non-NULL value once.
+ * Writes are protected by key_user_keyring_mutex.
+ * Unlocked readers should use READ_ONCE() unless they know that
+ * install_user_keyrings() has been called successfully (which sets
+ * these members to non-NULL values, preventing further modifications).
+ */
struct key *uid_keyring; /* UID specific keyring */
struct key *session_keyring; /* UID's default session keyring */
#endif
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index bd7243cb4c85..f05f7125a7d5 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -58,7 +58,7 @@ int install_user_keyrings(void)
kenter("%p{%u}", user, uid);
- if (user->uid_keyring && user->session_keyring) {
+ if (READ_ONCE(user->uid_keyring) && READ_ONCE(user->session_keyring)) {
kleave(" = 0 [exist]");
return 0;
}
@@ -111,8 +111,10 @@ int install_user_keyrings(void)
}
/* install the keyrings */
- user->uid_keyring = uid_keyring;
- user->session_keyring = session_keyring;
+ /* paired with READ_ONCE() */
+ smp_store_release(&user->uid_keyring, uid_keyring);
+ /* paired with READ_ONCE() */
+ smp_store_release(&user->session_keyring, session_keyring);
}
mutex_unlock(&key_user_keyring_mutex);
@@ -340,6 +342,7 @@ void key_fsgid_changed(struct task_struct *tsk)
key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
{
key_ref_t key_ref, ret, err;
+ const struct cred *cred = ctx->cred;
/* we want to return -EAGAIN or -ENOKEY if any of the keyrings were
* searchable, but we failed to find a key or we found a negative key;
@@ -353,9 +356,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
err = ERR_PTR(-EAGAIN);
/* search the thread keyring first */
- if (ctx->cred->thread_keyring) {
+ if (cred->thread_keyring) {
key_ref = keyring_search_aux(
- make_key_ref(ctx->cred->thread_keyring, 1), ctx);
+ make_key_ref(cred->thread_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -371,9 +374,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
}
/* search the process keyring second */
- if (ctx->cred->process_keyring) {
+ if (cred->process_keyring) {
key_ref = keyring_search_aux(
- make_key_ref(ctx->cred->process_keyring, 1), ctx);
+ make_key_ref(cred->process_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -392,9 +395,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
}
/* search the session keyring */
- if (ctx->cred->session_keyring) {
+ if (cred->session_keyring) {
key_ref = keyring_search_aux(
- make_key_ref(ctx->cred->session_keyring, 1), ctx);
+ make_key_ref(cred->session_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -413,9 +416,9 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
}
}
/* or search the user-session keyring */
- else if (ctx->cred->user->session_keyring) {
+ else if (READ_ONCE(cred->user->session_keyring)) {
key_ref = keyring_search_aux(
- make_key_ref(ctx->cred->user->session_keyring, 1),
+ make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -602,7 +605,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
goto error;
goto reget_creds;
} else if (ctx.cred->session_keyring ==
- ctx.cred->user->session_keyring &&
+ READ_ONCE(ctx.cred->user->session_keyring) &&
lflags & KEY_LOOKUP_CREATE) {
ret = join_session_keyring(NULL);
if (ret < 0)
@@ -616,7 +619,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
break;
case KEY_SPEC_USER_KEYRING:
- if (!ctx.cred->user->uid_keyring) {
+ if (!READ_ONCE(ctx.cred->user->uid_keyring)) {
ret = install_user_keyrings();
if (ret < 0)
goto error;
@@ -628,7 +631,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
break;
case KEY_SPEC_USER_SESSION_KEYRING:
- if (!ctx.cred->user->session_keyring) {
+ if (!READ_ONCE(ctx.cred->user->session_keyring)) {
ret = install_user_keyrings();
if (ret < 0)
goto error;
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index db72dc4d7639..75d87f9e0f49 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -293,11 +293,12 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
/* fall through */
case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:
dest_keyring =
- key_get(cred->user->session_keyring);
+ key_get(READ_ONCE(cred->user->session_keyring));
break;
case KEY_REQKEY_DEFL_USER_KEYRING:
- dest_keyring = key_get(cred->user->uid_keyring);
+ dest_keyring =
+ key_get(READ_ONCE(cred->user->uid_keyring));
break;
case KEY_REQKEY_DEFL_GROUP_KEYRING:
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH] security: don't use RCU accessors for cred->session_keyring
From: Jann Horn @ 2019-03-27 15:39 UTC (permalink / raw)
To: David Howells, jannh
Cc: James Morris, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
sparse complains that a bunch of places in kernel/cred.c access
cred->session_keyring without the RCU helpers required by the __rcu
annotation.
cred->session_keyring is written in the following places:
- prepare_kernel_cred() [in a new cred struct]
- keyctl_session_to_parent() [in a new cred struct]
- prepare_creds [in a new cred struct, via memcpy]
- install_session_keyring_to_cred()
- from install_session_keyring() on new creds
- from join_session_keyring() on new creds [twice]
- from umh_keys_init()
- from call_usermodehelper_exec_async() on new creds
All of these writes are before the creds are committed; therefore,
cred->session_keyring doesn't need RCU protection.
Remove the __rcu annotation and fix up all existing users that use __rcu.
Signed-off-by: Jann Horn <jannh@google.com>
---
include/linux/cred.h | 2 +-
security/keys/process_keys.c | 12 ++++--------
security/keys/request_key.c | 9 ++-------
3 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index ddd45bb74887..efb6edf32de7 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -138,7 +138,7 @@ struct cred {
#ifdef CONFIG_KEYS
unsigned char jit_keyring; /* default keyring to attach requested
* keys to */
- struct key __rcu *session_keyring; /* keyring inherited over fork */
+ struct key *session_keyring; /* keyring inherited over fork */
struct key *process_keyring; /* keyring private to this process */
struct key *thread_keyring; /* keyring private to this thread */
struct key *request_key_auth; /* assumed request_key authority */
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 9320424c4a46..bd7243cb4c85 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -227,6 +227,7 @@ static int install_process_keyring(void)
* Install the given keyring as the session keyring of the given credentials
* struct, replacing the existing one if any. If the given keyring is NULL,
* then install a new anonymous session keyring.
+ * @cred can not be in use by any task yet.
*
* Return: 0 on success; -errno on failure.
*/
@@ -254,7 +255,7 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring)
/* install the keyring */
old = cred->session_keyring;
- rcu_assign_pointer(cred->session_keyring, keyring);
+ cred->session_keyring = keyring;
if (old)
key_put(old);
@@ -392,11 +393,8 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
/* search the session keyring */
if (ctx->cred->session_keyring) {
- rcu_read_lock();
key_ref = keyring_search_aux(
- make_key_ref(rcu_dereference(ctx->cred->session_keyring), 1),
- ctx);
- rcu_read_unlock();
+ make_key_ref(ctx->cred->session_keyring, 1), ctx);
if (!IS_ERR(key_ref))
goto found;
@@ -612,10 +610,8 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
goto reget_creds;
}
- rcu_read_lock();
- key = rcu_dereference(ctx.cred->session_keyring);
+ key = ctx.cred->session_keyring;
__key_get(key);
- rcu_read_unlock();
key_ref = make_key_ref(key, 1);
break;
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 2f17d84d46f1..db72dc4d7639 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -142,12 +142,10 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
prkey = cred->process_keyring->serial;
sprintf(keyring_str[1], "%d", prkey);
- rcu_read_lock();
- session = rcu_dereference(cred->session_keyring);
+ session = cred->session_keyring;
if (!session)
session = cred->user->session_keyring;
sskey = session->serial;
- rcu_read_unlock();
sprintf(keyring_str[2], "%d", sskey);
@@ -287,10 +285,7 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
/* fall through */
case KEY_REQKEY_DEFL_SESSION_KEYRING:
- rcu_read_lock();
- dest_keyring = key_get(
- rcu_dereference(cred->session_keyring));
- rcu_read_unlock();
+ dest_keyring = key_get(cred->session_keyring);
if (dest_keyring)
break;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: [PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
From: Mimi Zohar @ 2019-03-27 15:04 UTC (permalink / raw)
To: Paul Moore
Cc: Richard Guy Briggs, linux-integrity, linux-security-module,
Linux-Audit Mailing List, LKML, sgrubb, omosnace, Eric Paris,
Serge Hallyn, mjg59
In-Reply-To: <CAHC9VhS1e0+1t=ZUsKoJcFvO+JdewL4uqwO4dqw=tOOZdJbLYA@mail.gmail.com>
On Tue, 2019-03-26 at 19:58 -0400, Paul Moore wrote:
> On Tue, Mar 26, 2019 at 4:40 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > Hi Richard, Paul,
> >
> > On Tue, 2019-03-26 at 14:49 -0400, Richard Guy Briggs wrote:
> > > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > > verified xattrs"), the call to audit_log_start() is missing a context to
> > > link it to an audit event. Since this event is in user context, add
> > > the process' syscall context to the record.
> > >
> > > In addition, the orphaned keyword "locked" appears in the record.
> > > Normalize this by changing it to logging the locking string "." as any
> > > other user input in the "xattr=" field.
> > >
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/109
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> >
> > Paul, were you planning on upstreaming this patch?
>
> Yep, unless you would rather do it?
No, that's fine. Thanks!
Mimi
^ permalink raw reply
* Re: [PATCH] Yama: mark local symbols as static
From: Mukesh Ojha @ 2019-03-27 12:42 UTC (permalink / raw)
To: Jann Horn, Kees Cook
Cc: linux-kernel, James Morris, Serge E. Hallyn,
linux-security-module
In-Reply-To: <20190326230841.87834-1-jannh@google.com>
On 3/27/2019 4:38 AM, Jann Horn wrote:
> sparse complains that Yama defines functions and a variable as non-static
> even though they don't exist in any header. Fix it by making them static.
>
> Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
> ---
> security/yama/yama_lsm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 57cc60722dd3..06b14a57b0a4 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -206,7 +206,7 @@ static void yama_ptracer_del(struct task_struct *tracer,
> * yama_task_free - check for task_pid to remove from exception list
> * @task: task being removed
> */
> -void yama_task_free(struct task_struct *task)
> +static void yama_task_free(struct task_struct *task)
> {
> yama_ptracer_del(task, task);
> }
> @@ -401,7 +401,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
> *
> * Returns 0 if following the ptrace is allowed, -ve on error.
> */
> -int yama_ptrace_traceme(struct task_struct *parent)
> +static int yama_ptrace_traceme(struct task_struct *parent)
> {
> int rc = 0;
>
> @@ -452,7 +452,7 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
> static int zero;
> static int max_scope = YAMA_SCOPE_NO_ATTACH;
>
> -struct ctl_path yama_sysctl_path[] = {
> +static struct ctl_path yama_sysctl_path[] = {
> { .procname = "kernel", },
> { .procname = "yama", },
> { }
^ permalink raw reply
* [PATCH] Yama: mark function as static
From: Mukesh Ojha @ 2019-03-27 7:50 UTC (permalink / raw)
To: keescook, jannh, linux-kernel, jmorris, serge,
linux-security-module
Cc: Mukesh Ojha
Sparse complains yama_task_prctl can be static. Fix it by making
it static.
Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
---
security/yama/yama_lsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 57cc607..9c5a15b 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -222,7 +222,7 @@ void yama_task_free(struct task_struct *task)
* Return 0 on success, -ve on error. -ENOSYS is returned when Yama
* does not handle the given option.
*/
-int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5)
{
int rc = -ENOSYS;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 5:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <16124107-70D3-4CA0-9766-36FC6DC10128@amacapital.net>
On Tue, Mar 26, 2019 at 10:29:41PM -0700, Andy Lutomirski wrote:
>
>
> > On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> >> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> >>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>
> >>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> >>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> >>>> <matthewgarrett@google.com> wrote:
> >>>>>
> >>>>> From: Matthew Garrett <mjg59@google.com>
> >>>>>
> >>>>> debugfs has not been meaningfully audited in terms of ensuring that
> >>>>> userland cannot trample over the kernel. At Greg's request, disable
> >>>>> access to it entirely when the kernel is locked down. This is done at
> >>>>> open() time rather than init time as the kernel lockdown status may be
> >>>>> made stricter at runtime.
> >>>>
> >>>> Ugh. Some of those files are very useful. Could this perhaps still
> >>>> allow O_RDONLY if we're in INTEGRITY mode?
> >>>
> >>> Useful for what? Debugging, sure, but for "normal operation", no kernel
> >>> functionality should ever require debugfs. If it does, that's a bug and
> >>> should be fixed.
> >>>
> >>
> >> I semi-regularly read files in debugfs to diagnose things, and I think
> >> it would be good for this to work on distro kernels.
> >
> > Doing that for debugging is wonderful. People who want this type of
> > "lock down" are trading potential security for diagnositic ability.
> >
>
> I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
Reading a debugfs file can expose loads of things that can help take
over a kernel, or at least make it easier. Pointer addresses, internal
system state, loads of other fun things. And before 4.14 or so, it was
pretty trivial to use it to oops the kernel as well (not an issue here
anymore, but people are right to be nervous).
Personally, I think these are all just "confidentiality" type things,
but who really knows given the wild-west nature of debugfs (which is as
designed). And given that I think this patch series just crazy anyway,
I really don't care :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 5:29 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327050615.GA548@kroah.com>
> On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
>>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
>>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
>>>> <matthewgarrett@google.com> wrote:
>>>>>
>>>>> From: Matthew Garrett <mjg59@google.com>
>>>>>
>>>>> debugfs has not been meaningfully audited in terms of ensuring that
>>>>> userland cannot trample over the kernel. At Greg's request, disable
>>>>> access to it entirely when the kernel is locked down. This is done at
>>>>> open() time rather than init time as the kernel lockdown status may be
>>>>> made stricter at runtime.
>>>>
>>>> Ugh. Some of those files are very useful. Could this perhaps still
>>>> allow O_RDONLY if we're in INTEGRITY mode?
>>>
>>> Useful for what? Debugging, sure, but for "normal operation", no kernel
>>> functionality should ever require debugfs. If it does, that's a bug and
>>> should be fixed.
>>>
>>
>> I semi-regularly read files in debugfs to diagnose things, and I think
>> it would be good for this to work on distro kernels.
>
> Doing that for debugging is wonderful. People who want this type of
> "lock down" are trading potential security for diagnositic ability.
>
I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 5:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Linux API, Matthew Garrett
In-Reply-To: <CALCETrVDDf4dj3Md1Nxc5P_NCToXGnAbEO3bGfwgKSfeVmU4Kw@mail.gmail.com>
On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > > On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > > <matthewgarrett@google.com> wrote:
> > > >
> > > > From: Matthew Garrett <mjg59@google.com>
> > > >
> > > > debugfs has not been meaningfully audited in terms of ensuring that
> > > > userland cannot trample over the kernel. At Greg's request, disable
> > > > access to it entirely when the kernel is locked down. This is done at
> > > > open() time rather than init time as the kernel lockdown status may be
> > > > made stricter at runtime.
> > >
> > > Ugh. Some of those files are very useful. Could this perhaps still
> > > allow O_RDONLY if we're in INTEGRITY mode?
> >
> > Useful for what? Debugging, sure, but for "normal operation", no kernel
> > functionality should ever require debugfs. If it does, that's a bug and
> > should be fixed.
> >
>
> I semi-regularly read files in debugfs to diagnose things, and I think
> it would be good for this to work on distro kernels.
Doing that for debugging is wonderful. People who want this type of
"lock down" are trading potential security for diagnositic ability.
good luck!
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 4:29 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327003057.GA27311@kroah.com>
On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > <matthewgarrett@google.com> wrote:
> > >
> > > From: Matthew Garrett <mjg59@google.com>
> > >
> > > debugfs has not been meaningfully audited in terms of ensuring that
> > > userland cannot trample over the kernel. At Greg's request, disable
> > > access to it entirely when the kernel is locked down. This is done at
> > > open() time rather than init time as the kernel lockdown status may be
> > > made stricter at runtime.
> >
> > Ugh. Some of those files are very useful. Could this perhaps still
> > allow O_RDONLY if we're in INTEGRITY mode?
>
> Useful for what? Debugging, sure, but for "normal operation", no kernel
> functionality should ever require debugfs. If it does, that's a bug and
> should be fixed.
>
I semi-regularly read files in debugfs to diagnose things, and I think
it would be good for this to work on distro kernels.
^ 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