* Re: [PATCH v2 08/79] docs: cgroup-v1: convert docs to ReST and rename to *.rst
From: Tejun Heo @ 2019-05-06 15:47 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86, Jens Axboe, Li Zefan, Johannes Weiner,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, James Morris, Serge E. Hallyn, linux-block,
cgroups, netdev, bpf, linux-security-module
In-Reply-To: <c6e79690c038fc6bbf9265a065c1f861d6e156fa.1555938375.git.mchehab+samsung@kernel.org>
On Mon, Apr 22, 2019 at 10:26:57AM -0300, Mauro Carvalho Chehab wrote:
> Convert the cgroup-v1 files to ReST format, in order to
> allow a later addition to the admin-guide.
>
> The conversion is actually:
> - add blank lines and identation in order to identify paragraphs;
> - fix tables markups;
> - add some lists markups;
> - mark literal blocks;
> - adjust title markups.
>
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Please feel free to route with other patches in the series.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] kexec_buffer measure
From: Mimi Zohar @ 2019-05-03 14:24 UTC (permalink / raw)
To: Tetsuo Handa, Casey Schaufler, prakhar srivastava
Cc: linux-integrity, linux-kernel, linux-security-module, Paul Moore,
John Johansen
In-Reply-To: <5490e443-b3ea-876e-a6b3-6a91005afe61@I-love.SAKURA.ne.jp>
On Fri, 2019-05-03 at 09:53 +0900, Tetsuo Handa wrote:
> On 2019/05/03 1:28, Casey Schaufler wrote:
> > On 5/2/2019 8:48 AM, Mimi Zohar wrote:
> >> [Cc'ing Paul, John, Casey]
> >>
> >> On Mon, 2019-04-22 at 20:18 -0400, Mimi Zohar wrote:
> >>> [Cc'ing LSM mailing list]
> >>>
> >>> On Fri, 2019-04-19 at 17:30 -0700, prakhar srivastava wrote:
> >>>
> >>>> 2) Adding a LSM hook
> >>>> We are doing both the command line and kernel version measurement in IMA.
> >>>> Can you please elaborate on how this can be used outside of the scenario?
> >>>> That will help me come back with a better design and code. I am
> >>>> neutral about this.
> >>> As I said previously, initially you might want to only measure the
> >>> kexec boot command line, but will you ever want to verify or audit log
> >>> the boot command line hash? Perhaps LSMs would be interested in the
> >>> boot command line. Should this be an LSM hook?
> >> From an LSM perspective, is there any interest in the boot command line?
> >
> > I can imagine an LSM that cares about the command line,
> > but I don't have interest in it for any work I have in progress.
> >
>
> Since the kernel command line controls which LSMs to enable, I doubt that
> an LSM which cares about the command line can detect that the kernel command
> line was tampered when the kernel command line was tampered...
As the subject line indicates, this is the kexec boot command line.
This wouldn't be any different than the existing
kernel_read_file_from_fd() and security_kernel_load_data() calls in
kernel/kexec_file.c and kernel/kexec.c, which provides the LSMs an
opportunity to comment on the kexec image and initramfs.
Mimi
^ permalink raw reply
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: Ard Biesheuvel @ 2019-05-03 14:23 UTC (permalink / raw)
To: joeyli
Cc: Peter Jones, Matthew Garrett, Lee, Chun-Yi, Mimi Zohar,
David Howells, James Morris, Serge E . Hallyn, Josh Boyer,
Nayna Jain, linux-efi, linux-security-module,
Linux Kernel Mailing List
In-Reply-To: <20190503085834.GQ11486@linux-l9pv.suse>
On Fri, 3 May 2019 at 10:59, joeyli <jlee@suse.com> wrote:
>
> On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> > On Fri, 3 May 2019 at 09:18, joeyli <jlee@suse.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> 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.
> > > > >
> > > > > v3.
> > > > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > > > [ 1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > > [ 1.501773] blacklist: Loaded blacklisting hash
> > > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > > >
> > > > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > > > >
> > > > > v2.
> > > > > Setting the MODSIGN messages 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>
> > > > > ---
> > > > > certs/blacklist.c | 3 +-
> > > > > security/integrity/platform_certs/load_uefi.c | 40 +++++++++++++++++++--------
> > > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > index 3a507b9e2568..f91437e39e44 100644
> > > > > --- a/certs/blacklist.c
> > > > > +++ b/certs/blacklist.c
> > > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > > if (IS_ERR(key)) {
> > > > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > > > > return PTR_ERR(key);
> > > > > - }
> > > > > + } else
> > > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > > @@ -1,5 +1,7 @@
> > > > > // SPDX-License-Identifier: GPL-2.0
> > > > >
> > > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > > +
> > > > > #include <linux/kernel.h>
> > > > > #include <linux/sched.h>
> > > > > #include <linux/cred.h>
> > > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > > return status == EFI_SUCCESS;
> > > > > }
> > > > >
> > > > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > > > +{
> > > > > + int i = 0;
> > > > > +
> > > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > > + str[i] = str16[i];
> > > > > + i++;
> > > > > + }
> > > > > +
> > > > > + str[i] = '\0';
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * Get a certificate list blob from the named EFI variable.
> > > > > */
> > > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > > > efi_status_t status;
> > > > > unsigned long lsize = 4;
> > > > > unsigned long tmpdb[4];
> > > > > + char namestr[16];
> > > > > void *db;
> > > > >
> > > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > > >
> > > > Please drop this (and the function above) - instead, just return NULL
> > > > if the variable is not found (without reporting an error).
> > > >
> > >
> > > This name string is for printing debug level message, not error message.
> > > This function already returns NULL when EFI_NOT_FOUND be returned by
> > > firmware.
> > >
> > > > > 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_debug("UEFI %s list doesn't exist\n", namestr);
> > > > > + else
> > > > > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > > > > + namestr, efi_status_to_str(status));
> > > > > return NULL;
> > >
> > > here returns NULL when EFI_NOT_FOUND. The message of existence is for
> > > debugging.
> > >
> >
> > I understand that. But I don't think we need it.
> >
>
> OK. I will remove the debug message.
>
> > > > > }
> > > > > + pr_debug("UEFI %s list exists\n", namestr);
> > > > >
> > > > > db = kmalloc(lsize, GFP_KERNEL);
> > > > > if (!db)
> > > > > @@ -59,7 +80,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 UEFI %s list: %s\n",
> > > > > + namestr, efi_status_to_str(status));
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source, const void *data,
> > > > > static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > > const void *data, size_t len)
> > > > > {
> > > > > + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> > > > > uefi_blacklist_hash(source, data, len, "tbs:", 4);
> > > > > }
> > > > >
> > > > > @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > > static __init void uefi_blacklist_binary(const char *source,
> > > > > const void *data, size_t len)
> > > > > {
> > > > > + pr_info("Blacklisting hash of an executable: %s\n", source);
> > > > > uefi_blacklist_hash(source, data, len, "bin:", 4);
> > > > > }
> > > > >
> > > >
> > > > These are separate changes - I don't have an opinion whether they are
> > > > appropriate or not, but they should be in a separate patch.
> > > >
> > >
> > > I will move the message of blacklising hash to other patch. Thanks!
> > >
> > > > > @@ -154,9 +178,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");
> > > > > - } else {
> > > > > + if (db) {
> > > > > rc = parse_efi_signature_list("UEFI:db",
> > > > > db, dbsize, get_handler_for_db);
> > > > > if (rc)
> > > > > @@ -167,9 +189,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");
> > > > > - } else {
> > > > > + if (mok) {
> > > > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > > > mok, moksize, get_handler_for_db);
> > > > > if (rc)
> > > > > @@ -178,9 +198,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");
> > > > > - } else {
> > > > > + if (dbx) {
> > > > > rc = parse_efi_signature_list("UEFI:dbx",
> > > > > dbx, dbxsize,
> > > > > get_handler_for_dbx);
> > > > > --
> > > > > 2.16.4
> > > > >
> > > >
> > > > I think we should consider carefully what it means if some of these
> > > > variables don't exist:
> > > > - if secure boot is enabled, db and dbx must exist, so if they don't,
> > > > something is wrong
> > >
> > > The existence of db/dbx is not related to secure boot. If manufacturer/user
> > > enrolled certificate/hash to db or dbx, then the variable will be created.
> > > If user didn't enroll anything to db/dbx, then variables will not show up.
> > >
> >
> > Yes, but if secure boot is enabled and db is empty, how could you have
> > booted in the first place?
> >
>
> I agree. When secure boot enabled, kernel can not be booted without db.
>
> > And what about the converse case: if secure boot is not enabled, why
> > should we trust the contents of db?
> >
>
> The db and dbx are authenticated variables that it protected by KEK.
> So it can be trusted even secure boot is disabled. Unless manufacturer
> or user's KEK is leaked.
>
Is that true for non-secureboot capable firmware? Of course, in that
case, we cannot be sure that the kernel itself is trusted, but we
shouldn't open up another hole either.
> > > > - secure boot might be enabled but we may be booting without shim.
> > >
> > > Shim always creates MokListRT no matter secure boot enabled or disabled.
> > >
> >
> > That is not my point. What happens if you booted with secure boot
> > enabled but without the help of shim?
> >
>
> Without shim, the signed EFI stub can still be booted by EFI boot manager.
> But the MokListRT will not be created for runtime. So MOK signed kernel
> module can not be verified. (or IMA can not verify MOK signed kernel image
> for kexec...)
>
So if we opt out of using shim, we lose kexec capability as well? That
doesn't make any sense.
In general, I am very uncomfortable with the way we have painted
ourselves into a corner with shim. If I install Suse on a system that
has the Suse cert in db or KEK, why would I need shim in the first
place?
> > > > - secure boot might be disabled.
> > > >
> > >
> > > It's not about secure boot, db/dbx/MokListRT are always available at
> > > runtime if user was enrolled something to those list.
> > >
> >
> > Yes, but again, depending on whether shim was involved, and/or whether
> > secure boot was enabled or not, the way we interpret these things may
> > be very different.
> >
> > I want the reasoning to be sound before merging any patches that deal
> > with these variables.
>
> Here is a simple summary:
>
> When secure boot is enabled:
> - db/dbx: Can be trusted because they are authenticated variables.
> (unless end user doesn't want to trust db/dbx)
> - MokListRT:
> - with shim: MokListRT will be created. It can be trusted.
> - without shim: MokListRT will not be created.
> MOK protected kernel module or file can not be
> verified.
>
So who polices that MoKlistRT is not created?
> When secure boot is disabled:
> - db/dbx: Can be trusted because they are authenticated variables.
> (unless end user doesn't want to trust db/dbx)
Is this true? Does non-secure boot capable firmware still guarantee
that db/dbx can't be created?
> - MokListRT:
> - with shim: MokListRT will be created. But it can not be trusted.
> MOK protected kernel module or file can not be
> verified.
> - without shim: MokListRT will not be created.
> MOK protected kernel module or file can not be
> verified.
>
Enabling this securely involves more than connecting the dots when it
comes to db, MokListRT etc. I would like for someone to convince me as
a maintainer, as well as the IMA maintainers, that the security
requirements are still met in all cases we care about. Just copying db
and MokListRT into a kernel keychain doesn't appear to do that, and if
it does, it needs more documentation to clarify that.
^ permalink raw reply
* [PATCH v4] apparmor: Use a memory pool instead per-CPU caches
From: Sebastian Andrzej Siewior @ 2019-05-03 14:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <56de7347-b119-155d-675c-23a227ffd516@i-love.sakura.ne.jp>
The get_buffers() macro may provide one or two buffers to the caller.
Those buffers are pre-allocated on init for each CPU. By default it
allocates
2* 2 * MAX_PATH * POSSIBLE_CPU
which equals 64KiB on a system with 4 CPUs or 1MiB with 64 CPUs and so
on.
Replace the per-CPU buffers with a common memory pool which is shared
across all CPUs. The pool grows on demand and never shrinks. The pool
starts with two (UP) or four (SMP) elements. By using this pool it is
possible to request a buffer and keeping preemption enabled which avoids
the hack in profile_transition().
It has been pointed out by Tetsuo Handa that GFP_KERNEL allocations for
small amount of memory do not fail. In order not to have an endless
retry, __GFP_RETRY_MAYFAIL is passed (so the memory allocation is not
repeated until success) and retried once hoping that in the meantime a
buffer has been returned to the pool. Since now NULL is possible all
allocation paths check the buffer pointer and return -ENOMEM on failure.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v3…v4:
- let aa_put_buffer() accept a NULL pointer, noticed by Tetsuo Handa
v2…v3:
- remove an unused flags variable.
v1…v2:
- Use max_t instead min_t for a minimal buffer.
- Pre-allocate 2 buffers on UP and 4 buffers on SMP systems.
- Use __GFP_RETRY_MAYFAIL | __GFP_NOWARN on memory allocation. The
allocation may fail under pressure and we will retry once.
- Add an error path to each caller of aa_get_buffer() if the memory
allocation fails.
security/apparmor/domain.c | 24 +++----
security/apparmor/file.c | 24 +++++--
security/apparmor/include/path.h | 49 +-------------
security/apparmor/lsm.c | 107 +++++++++++++++++++++++--------
security/apparmor/mount.c | 65 +++++++++++++++----
5 files changed, 161 insertions(+), 108 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index ca2dccf5b445e..cae0e619ff4fe 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -689,20 +689,9 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
} else if (COMPLAIN_MODE(profile)) {
/* no exec permission - learning mode */
struct aa_profile *new_profile = NULL;
- char *n = kstrdup(name, GFP_ATOMIC);
- if (n) {
- /* name is ptr into buffer */
- long pos = name - buffer;
- /* break per cpu buffer hold */
- put_buffers(buffer);
- new_profile = aa_new_null_profile(profile, false, n,
- GFP_KERNEL);
- get_buffers(buffer);
- name = buffer + pos;
- strcpy((char *)name, n);
- kfree(n);
- }
+ new_profile = aa_new_null_profile(profile, false, name,
+ GFP_KERNEL);
if (!new_profile) {
error = -ENOMEM;
info = "could not create null profile";
@@ -907,7 +896,12 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
ctx->nnp = aa_get_label(label);
/* buffer freed below, name is pointer into buffer */
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto done;
+ }
+
/* Test for onexec first as onexec override other x transitions. */
if (ctx->onexec)
new = handle_onexec(label, ctx->onexec, ctx->token,
@@ -979,7 +973,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
done:
aa_put_label(label);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index d0afed9ebd0ed..7b424e73a8c74 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -336,12 +336,14 @@ int aa_path_perm(const char *op, struct aa_label *label,
flags |= PATH_DELEGATE_DELETED | (S_ISDIR(cond->mode) ? PATH_IS_DIR :
0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
profile_path_perm(op, profile, path, buffer, request,
cond, flags, &perms));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -479,12 +481,18 @@ int aa_path_link(struct aa_label *label, struct dentry *old_dentry,
int error;
/* buffer freed below, lname is pointer in buffer */
- get_buffers(buffer, buffer2);
+ buffer = aa_get_buffer();
+ buffer2 = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !buffer2)
+ goto out;
+
error = fn_for_each_confined(label, profile,
profile_path_link(profile, &link, buffer, &target,
buffer2, &cond));
- put_buffers(buffer, buffer2);
-
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(buffer2);
return error;
}
@@ -528,7 +536,9 @@ static int __file_path_perm(const char *op, struct aa_label *label,
return 0;
flags = PATH_DELEGATE_DELETED | (S_ISDIR(cond.mode) ? PATH_IS_DIR : 0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
/* check every profile in task label not in current cache */
error = fn_for_each_not_in_set(flabel, label, profile,
@@ -557,7 +567,7 @@ static int __file_path_perm(const char *op, struct aa_label *label,
if (!error)
update_file_ctx(file_ctx(file), label, request);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
diff --git a/security/apparmor/include/path.h b/security/apparmor/include/path.h
index b6380c5f00972..b0b2ab85e42d8 100644
--- a/security/apparmor/include/path.h
+++ b/security/apparmor/include/path.h
@@ -15,7 +15,6 @@
#ifndef __AA_PATH_H
#define __AA_PATH_H
-
enum path_flags {
PATH_IS_DIR = 0x1, /* path is a directory */
PATH_CONNECT_PATH = 0x4, /* connect disconnected paths to / */
@@ -30,51 +29,7 @@ int aa_path_name(const struct path *path, int flags, char *buffer,
const char **name, const char **info,
const char *disconnected);
-#define MAX_PATH_BUFFERS 2
-
-/* Per cpu buffers used during mediation */
-/* preallocated buffers to use during path lookups */
-struct aa_buffers {
- char *buf[MAX_PATH_BUFFERS];
-};
-
-#include <linux/percpu.h>
-#include <linux/preempt.h>
-
-DECLARE_PER_CPU(struct aa_buffers, aa_buffers);
-
-#define ASSIGN(FN, A, X, N) ((X) = FN(A, N))
-#define EVAL1(FN, A, X) ASSIGN(FN, A, X, 0) /*X = FN(0)*/
-#define EVAL2(FN, A, X, Y...) \
- do { ASSIGN(FN, A, X, 1); EVAL1(FN, A, Y); } while (0)
-#define EVAL(FN, A, X...) CONCATENATE(EVAL, COUNT_ARGS(X))(FN, A, X)
-
-#define for_each_cpu_buffer(I) for ((I) = 0; (I) < MAX_PATH_BUFFERS; (I)++)
-
-#ifdef CONFIG_DEBUG_PREEMPT
-#define AA_BUG_PREEMPT_ENABLED(X) AA_BUG(preempt_count() <= 0, X)
-#else
-#define AA_BUG_PREEMPT_ENABLED(X) /* nop */
-#endif
-
-#define __get_buffer(C, N) ({ \
- AA_BUG_PREEMPT_ENABLED("__get_buffer without preempt disabled"); \
- (C)->buf[(N)]; })
-
-#define __get_buffers(C, X...) EVAL(__get_buffer, C, X)
-
-#define __put_buffers(X, Y...) ((void)&(X))
-
-#define get_buffers(X...) \
-do { \
- struct aa_buffers *__cpu_var = get_cpu_ptr(&aa_buffers); \
- __get_buffers(__cpu_var, X); \
-} while (0)
-
-#define put_buffers(X, Y...) \
-do { \
- __put_buffers(X, Y); \
- put_cpu_ptr(&aa_buffers); \
-} while (0)
+char *aa_get_buffer(void);
+void aa_put_buffer(char *buf);
#endif /* __AA_PATH_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92d..c5915a6853738 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -47,8 +47,13 @@
/* Flag indicating whether initialization completed */
int apparmor_initialized;
-DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
+union aa_buffer {
+ struct list_head list;
+ char buffer[1];
+};
+static LIST_HEAD(aa_global_buffers);
+static DEFINE_SPINLOCK(aa_buffers_lock);
/*
* LSM hook functions
@@ -1406,6 +1411,7 @@ static int param_set_aauint(const char *val, const struct kernel_param *kp)
return -EPERM;
error = param_set_uint(val, kp);
+ aa_g_path_max = max_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
return error;
@@ -1518,6 +1524,48 @@ static int param_set_mode(const char *val, const struct kernel_param *kp)
return 0;
}
+char *aa_get_buffer(void)
+{
+ union aa_buffer *aa_buf;
+ bool try_again = true;
+
+retry:
+ spin_lock(&aa_buffers_lock);
+ if (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ return &aa_buf->buffer[0];
+ }
+ spin_unlock(&aa_buffers_lock);
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL | __GFP_RETRY_MAYFAIL |
+ __GFP_NOWARN);
+ if (!aa_buf) {
+ if (try_again) {
+ try_again = false;
+ goto retry;
+ }
+ pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
+ return NULL;
+ }
+ return &aa_buf->buffer[0];
+}
+
+void aa_put_buffer(char *buf)
+{
+ union aa_buffer *aa_buf;
+
+ if (!buf)
+ return;
+ aa_buf = container_of(buf, union aa_buffer, buffer[0]);
+
+ spin_lock(&aa_buffers_lock);
+ list_add(&aa_buf->list, &aa_global_buffers);
+ spin_unlock(&aa_buffers_lock);
+}
+
/*
* AppArmor init functions
*/
@@ -1538,38 +1586,48 @@ static int __init set_init_ctx(void)
static void destroy_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- kfree(per_cpu(aa_buffers, i).buf[j]);
- per_cpu(aa_buffers, i).buf[j] = NULL;
- }
+ spin_lock(&aa_buffers_lock);
+ while (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ kfree(aa_buf);
+ spin_lock(&aa_buffers_lock);
}
+ spin_unlock(&aa_buffers_lock);
}
static int __init alloc_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
+ int i, num;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- char *buffer;
+ /*
+ * A function may require two buffers at once. Usually the buffers are
+ * used for a short period of time and are shared. On UP kernel buffers
+ * two should be enough, with more CPUs it is possible that more
+ * buffers will be used simultaneously. The preallocated pool may grow.
+ * This preallocation has also the side-effect that AppArmor will be
+ * disabled early at boot if aa_g_path_max is extremly high.
+ */
+ if (num_online_cpus() > 1)
+ num = 4;
+ else
+ num = 2;
- if (cpu_to_node(i) > num_online_nodes())
- /* fallback to kmalloc for offline nodes */
- buffer = kmalloc(aa_g_path_max, GFP_KERNEL);
- else
- buffer = kmalloc_node(aa_g_path_max, GFP_KERNEL,
- cpu_to_node(i));
- if (!buffer) {
- destroy_buffers();
- return -ENOMEM;
- }
- per_cpu(aa_buffers, i).buf[j] = buffer;
+ for (i = 0; i < num; i++) {
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL |
+ __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+ if (!aa_buf) {
+ destroy_buffers();
+ return -ENOMEM;
}
+ aa_put_buffer(&aa_buf->buffer[0]);
}
-
return 0;
}
@@ -1734,7 +1792,7 @@ static int __init apparmor_init(void)
error = alloc_buffers();
if (error) {
AA_ERROR("Unable to allocate work buffers\n");
- goto buffers_out;
+ goto alloc_out;
}
error = set_init_ctx();
@@ -1759,7 +1817,6 @@ static int __init apparmor_init(void)
buffers_out:
destroy_buffers();
-
alloc_out:
aa_destroy_aafs();
aa_teardown_dfa_engine();
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 8c3787399356b..267a26fba14e1 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -412,11 +412,13 @@ int aa_remount(struct aa_label *label, const struct path *path,
binary = path->dentry->d_sb->s_type->fs_flags & FS_BINARY_MOUNTDATA;
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, data, binary));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -441,11 +443,18 @@ int aa_bind_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || old_buffer)
+ goto out;
+
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, flags, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -465,11 +474,13 @@ int aa_mount_change_type(struct aa_label *label, const struct path *path,
flags &= (MS_REC | MS_SILENT | MS_SHARED | MS_PRIVATE | MS_SLAVE |
MS_UNBINDABLE);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, NULL, false));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -492,11 +503,17 @@ int aa_move_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !old_buffer)
+ goto out;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, MS_MOVE, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -537,17 +554,29 @@ int aa_new_mount(struct aa_label *label, const char *dev_name,
}
}
- get_buffers(buffer, dev_buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
if (dev_path) {
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, dev_path, dev_buffer,
type, flags, data, binary));
} else {
+ dev_buffer = aa_get_buffer();
+ if (!dev_buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
error = fn_for_each_confined(label, profile,
match_mnt_path_str(profile, path, buffer, dev_name,
type, flags, data, binary, NULL));
}
- put_buffers(buffer, dev_buffer);
+
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(dev_buffer);
if (dev_path)
path_put(dev_path);
@@ -595,10 +624,13 @@ int aa_umount(struct aa_label *label, struct vfsmount *mnt, int flags)
AA_BUG(!label);
AA_BUG(!mnt);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
+
error = fn_for_each_confined(label, profile,
profile_umount(profile, &path, buffer));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -671,7 +703,11 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
AA_BUG(!old_path);
AA_BUG(!new_path);
- get_buffers(old_buffer, new_buffer);
+ old_buffer = aa_get_buffer();
+ new_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!old_buffer || !new_buffer)
+ goto out;
target = fn_label_build(label, profile, GFP_ATOMIC,
build_pivotroot(profile, new_path, new_buffer,
old_path, old_buffer));
@@ -690,7 +726,8 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
/* already audited error */
error = PTR_ERR(target);
out:
- put_buffers(old_buffer, new_buffer);
+ aa_put_buffer(old_buffer);
+ aa_put_buffer(new_buffer);
return error;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3] apparmor: Use a memory pool instead per-CPU caches
From: Tetsuo Handa @ 2019-05-03 12:41 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <20190503115145.anv7z4kk7okydthm@linutronix.de>
On 2019/05/03 20:51, Sebastian Andrzej Siewior wrote:
> +void aa_put_buffer(char *buf)
> +{
> + union aa_buffer *aa_buf;
> +
buf == NULL if aa_put_buffer() was called due to aa_get_buffer() == NULL.
> + aa_buf = container_of(buf, union aa_buffer, buffer[0]);
> +
> + spin_lock(&aa_buffers_lock);
> + list_add(&aa_buf->list, &aa_global_buffers);
> + spin_unlock(&aa_buffers_lock);
> +}
> +
> /*
> * AppArmor init functions
> */
^ permalink raw reply
* [PATCH v3] apparmor: Use a memory pool instead per-CPU caches
From: Sebastian Andrzej Siewior @ 2019-05-03 11:51 UTC (permalink / raw)
To: Tetsuo Handa
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <20190503114827.yky7r2cjq7zy4dfm@linutronix.de>
The get_buffers() macro may provide one or two buffers to the caller.
Those buffers are pre-allocated on init for each CPU. By default it
allocates
2* 2 * MAX_PATH * POSSIBLE_CPU
which equals 64KiB on a system with 4 CPUs or 1MiB with 64 CPUs and so
on.
Replace the per-CPU buffers with a common memory pool which is shared
across all CPUs. The pool grows on demand and never shrinks. The pool
starts with two (UP) or four (SMP) elements. By using this pool it is
possible to request a buffer and keeping preemption enabled which avoids
the hack in profile_transition().
It has been pointed out by Tetsuo Handa that GFP_KERNEL allocations for
small amount of memory do not fail. In order not to have an endless
retry, __GFP_RETRY_MAYFAIL is passed (so the memory allocation is not
repeated until success) and retried once hoping that in the meantime a
buffer has been returned to the pool. Since now NULL is possible all
allocation paths check the buffer pointer and return -ENOMEM on failure.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3:
- remove an unused flags variable.
v1…v2:
- Use max_t instead min_t for a minimal buffer.
- Pre-allocate 2 buffers on UP and 4 buffers on SMP systems.
- Use __GFP_RETRY_MAYFAIL | __GFP_NOWARN on memory allocation. The
allocation may fail under pressure and we will retry once.
- Add an error path to each caller of aa_get_buffer() if the memory
allocation fails.
security/apparmor/domain.c | 24 +++----
security/apparmor/file.c | 24 ++++---
security/apparmor/include/path.h | 49 +--------------
security/apparmor/lsm.c | 105 +++++++++++++++++++++++--------
security/apparmor/mount.c | 65 ++++++++++++++-----
5 files changed, 159 insertions(+), 108 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index ca2dccf5b445e..cae0e619ff4fe 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -689,20 +689,9 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
} else if (COMPLAIN_MODE(profile)) {
/* no exec permission - learning mode */
struct aa_profile *new_profile = NULL;
- char *n = kstrdup(name, GFP_ATOMIC);
- if (n) {
- /* name is ptr into buffer */
- long pos = name - buffer;
- /* break per cpu buffer hold */
- put_buffers(buffer);
- new_profile = aa_new_null_profile(profile, false, n,
- GFP_KERNEL);
- get_buffers(buffer);
- name = buffer + pos;
- strcpy((char *)name, n);
- kfree(n);
- }
+ new_profile = aa_new_null_profile(profile, false, name,
+ GFP_KERNEL);
if (!new_profile) {
error = -ENOMEM;
info = "could not create null profile";
@@ -907,7 +896,12 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
ctx->nnp = aa_get_label(label);
/* buffer freed below, name is pointer into buffer */
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto done;
+ }
+
/* Test for onexec first as onexec override other x transitions. */
if (ctx->onexec)
new = handle_onexec(label, ctx->onexec, ctx->token,
@@ -979,7 +973,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
done:
aa_put_label(label);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index d0afed9ebd0ed..7b424e73a8c74 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -336,12 +336,14 @@ int aa_path_perm(const char *op, struct aa_label *label,
flags |= PATH_DELEGATE_DELETED | (S_ISDIR(cond->mode) ? PATH_IS_DIR :
0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
profile_path_perm(op, profile, path, buffer, request,
cond, flags, &perms));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -479,12 +481,18 @@ int aa_path_link(struct aa_label *label, struct dentry *old_dentry,
int error;
/* buffer freed below, lname is pointer in buffer */
- get_buffers(buffer, buffer2);
+ buffer = aa_get_buffer();
+ buffer2 = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !buffer2)
+ goto out;
+
error = fn_for_each_confined(label, profile,
profile_path_link(profile, &link, buffer, &target,
buffer2, &cond));
- put_buffers(buffer, buffer2);
-
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(buffer2);
return error;
}
@@ -528,7 +536,9 @@ static int __file_path_perm(const char *op, struct aa_label *label,
return 0;
flags = PATH_DELEGATE_DELETED | (S_ISDIR(cond.mode) ? PATH_IS_DIR : 0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
/* check every profile in task label not in current cache */
error = fn_for_each_not_in_set(flabel, label, profile,
@@ -557,7 +567,7 @@ static int __file_path_perm(const char *op, struct aa_label *label,
if (!error)
update_file_ctx(file_ctx(file), label, request);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
diff --git a/security/apparmor/include/path.h b/security/apparmor/include/path.h
index b6380c5f00972..b0b2ab85e42d8 100644
--- a/security/apparmor/include/path.h
+++ b/security/apparmor/include/path.h
@@ -15,7 +15,6 @@
#ifndef __AA_PATH_H
#define __AA_PATH_H
-
enum path_flags {
PATH_IS_DIR = 0x1, /* path is a directory */
PATH_CONNECT_PATH = 0x4, /* connect disconnected paths to / */
@@ -30,51 +29,7 @@ int aa_path_name(const struct path *path, int flags, char *buffer,
const char **name, const char **info,
const char *disconnected);
-#define MAX_PATH_BUFFERS 2
-
-/* Per cpu buffers used during mediation */
-/* preallocated buffers to use during path lookups */
-struct aa_buffers {
- char *buf[MAX_PATH_BUFFERS];
-};
-
-#include <linux/percpu.h>
-#include <linux/preempt.h>
-
-DECLARE_PER_CPU(struct aa_buffers, aa_buffers);
-
-#define ASSIGN(FN, A, X, N) ((X) = FN(A, N))
-#define EVAL1(FN, A, X) ASSIGN(FN, A, X, 0) /*X = FN(0)*/
-#define EVAL2(FN, A, X, Y...) \
- do { ASSIGN(FN, A, X, 1); EVAL1(FN, A, Y); } while (0)
-#define EVAL(FN, A, X...) CONCATENATE(EVAL, COUNT_ARGS(X))(FN, A, X)
-
-#define for_each_cpu_buffer(I) for ((I) = 0; (I) < MAX_PATH_BUFFERS; (I)++)
-
-#ifdef CONFIG_DEBUG_PREEMPT
-#define AA_BUG_PREEMPT_ENABLED(X) AA_BUG(preempt_count() <= 0, X)
-#else
-#define AA_BUG_PREEMPT_ENABLED(X) /* nop */
-#endif
-
-#define __get_buffer(C, N) ({ \
- AA_BUG_PREEMPT_ENABLED("__get_buffer without preempt disabled"); \
- (C)->buf[(N)]; })
-
-#define __get_buffers(C, X...) EVAL(__get_buffer, C, X)
-
-#define __put_buffers(X, Y...) ((void)&(X))
-
-#define get_buffers(X...) \
-do { \
- struct aa_buffers *__cpu_var = get_cpu_ptr(&aa_buffers); \
- __get_buffers(__cpu_var, X); \
-} while (0)
-
-#define put_buffers(X, Y...) \
-do { \
- __put_buffers(X, Y); \
- put_cpu_ptr(&aa_buffers); \
-} while (0)
+char *aa_get_buffer(void);
+void aa_put_buffer(char *buf);
#endif /* __AA_PATH_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92d..d8a3b8fe4de00 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -47,8 +47,13 @@
/* Flag indicating whether initialization completed */
int apparmor_initialized;
-DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
+union aa_buffer {
+ struct list_head list;
+ char buffer[1];
+};
+static LIST_HEAD(aa_global_buffers);
+static DEFINE_SPINLOCK(aa_buffers_lock);
/*
* LSM hook functions
@@ -1406,6 +1411,7 @@ static int param_set_aauint(const char *val, const struct kernel_param *kp)
return -EPERM;
error = param_set_uint(val, kp);
+ aa_g_path_max = max_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
return error;
@@ -1518,6 +1524,46 @@ static int param_set_mode(const char *val, const struct kernel_param *kp)
return 0;
}
+char *aa_get_buffer(void)
+{
+ union aa_buffer *aa_buf;
+ bool try_again = true;
+
+retry:
+ spin_lock(&aa_buffers_lock);
+ if (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ return &aa_buf->buffer[0];
+ }
+ spin_unlock(&aa_buffers_lock);
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL | __GFP_RETRY_MAYFAIL |
+ __GFP_NOWARN);
+ if (!aa_buf) {
+ if (try_again) {
+ try_again = false;
+ goto retry;
+ }
+ pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
+ return NULL;
+ }
+ return &aa_buf->buffer[0];
+}
+
+void aa_put_buffer(char *buf)
+{
+ union aa_buffer *aa_buf;
+
+ aa_buf = container_of(buf, union aa_buffer, buffer[0]);
+
+ spin_lock(&aa_buffers_lock);
+ list_add(&aa_buf->list, &aa_global_buffers);
+ spin_unlock(&aa_buffers_lock);
+}
+
/*
* AppArmor init functions
*/
@@ -1538,38 +1584,48 @@ static int __init set_init_ctx(void)
static void destroy_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- kfree(per_cpu(aa_buffers, i).buf[j]);
- per_cpu(aa_buffers, i).buf[j] = NULL;
- }
+ spin_lock(&aa_buffers_lock);
+ while (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ kfree(aa_buf);
+ spin_lock(&aa_buffers_lock);
}
+ spin_unlock(&aa_buffers_lock);
}
static int __init alloc_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
+ int i, num;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- char *buffer;
+ /*
+ * A function may require two buffers at once. Usually the buffers are
+ * used for a short period of time and are shared. On UP kernel buffers
+ * two should be enough, with more CPUs it is possible that more
+ * buffers will be used simultaneously. The preallocated pool may grow.
+ * This preallocation has also the side-effect that AppArmor will be
+ * disabled early at boot if aa_g_path_max is extremly high.
+ */
+ if (num_online_cpus() > 1)
+ num = 4;
+ else
+ num = 2;
- if (cpu_to_node(i) > num_online_nodes())
- /* fallback to kmalloc for offline nodes */
- buffer = kmalloc(aa_g_path_max, GFP_KERNEL);
- else
- buffer = kmalloc_node(aa_g_path_max, GFP_KERNEL,
- cpu_to_node(i));
- if (!buffer) {
- destroy_buffers();
- return -ENOMEM;
- }
- per_cpu(aa_buffers, i).buf[j] = buffer;
+ for (i = 0; i < num; i++) {
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL |
+ __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+ if (!aa_buf) {
+ destroy_buffers();
+ return -ENOMEM;
}
+ aa_put_buffer(&aa_buf->buffer[0]);
}
-
return 0;
}
@@ -1734,7 +1790,7 @@ static int __init apparmor_init(void)
error = alloc_buffers();
if (error) {
AA_ERROR("Unable to allocate work buffers\n");
- goto buffers_out;
+ goto alloc_out;
}
error = set_init_ctx();
@@ -1759,7 +1815,6 @@ static int __init apparmor_init(void)
buffers_out:
destroy_buffers();
-
alloc_out:
aa_destroy_aafs();
aa_teardown_dfa_engine();
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 8c3787399356b..267a26fba14e1 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -412,11 +412,13 @@ int aa_remount(struct aa_label *label, const struct path *path,
binary = path->dentry->d_sb->s_type->fs_flags & FS_BINARY_MOUNTDATA;
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, data, binary));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -441,11 +443,18 @@ int aa_bind_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || old_buffer)
+ goto out;
+
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, flags, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -465,11 +474,13 @@ int aa_mount_change_type(struct aa_label *label, const struct path *path,
flags &= (MS_REC | MS_SILENT | MS_SHARED | MS_PRIVATE | MS_SLAVE |
MS_UNBINDABLE);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, NULL, false));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -492,11 +503,17 @@ int aa_move_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !old_buffer)
+ goto out;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, MS_MOVE, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -537,17 +554,29 @@ int aa_new_mount(struct aa_label *label, const char *dev_name,
}
}
- get_buffers(buffer, dev_buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
if (dev_path) {
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, dev_path, dev_buffer,
type, flags, data, binary));
} else {
+ dev_buffer = aa_get_buffer();
+ if (!dev_buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
error = fn_for_each_confined(label, profile,
match_mnt_path_str(profile, path, buffer, dev_name,
type, flags, data, binary, NULL));
}
- put_buffers(buffer, dev_buffer);
+
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(dev_buffer);
if (dev_path)
path_put(dev_path);
@@ -595,10 +624,13 @@ int aa_umount(struct aa_label *label, struct vfsmount *mnt, int flags)
AA_BUG(!label);
AA_BUG(!mnt);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
+
error = fn_for_each_confined(label, profile,
profile_umount(profile, &path, buffer));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -671,7 +703,11 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
AA_BUG(!old_path);
AA_BUG(!new_path);
- get_buffers(old_buffer, new_buffer);
+ old_buffer = aa_get_buffer();
+ new_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!old_buffer || !new_buffer)
+ goto out;
target = fn_label_build(label, profile, GFP_ATOMIC,
build_pivotroot(profile, new_path, new_buffer,
old_path, old_buffer));
@@ -690,7 +726,8 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
/* already audited error */
error = PTR_ERR(target);
out:
- put_buffers(old_buffer, new_buffer);
+ aa_put_buffer(old_buffer);
+ aa_put_buffer(new_buffer);
return error;
--
2.20.1
^ permalink raw reply related
* [PATCH v2] apparmor: Use a memory pool instead per-CPU caches
From: Sebastian Andrzej Siewior @ 2019-05-03 11:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <001f651f-c544-c3fa-c0c2-f2a2b1ed565a@i-love.sakura.ne.jp>
The get_buffers() macro may provide one or two buffers to the caller.
Those buffers are pre-allocated on init for each CPU. By default it
allocates
2* 2 * MAX_PATH * POSSIBLE_CPU
which equals 64KiB on a system with 4 CPUs or 1MiB with 64 CPUs and so
on.
Replace the per-CPU buffers with a common memory pool which is shared
across all CPUs. The pool grows on demand and never shrinks. The pool
starts with two (UP) or four (SMP) elements. By using this pool it is
possible to request a buffer and keeping preemption enabled which avoids
the hack in profile_transition().
It has been pointed out by Tetsuo Handa that GFP_KERNEL allocations for
small amount of memory do not fail. In order not to have an endless
retry, __GFP_RETRY_MAYFAIL is passed (so the memory allocation is not
repeated until success) and retried once hoping that in the meantime a
buffer has been returned to the pool. Since now NULL is possible all
allocation paths check the buffer pointer and return -ENOMEM on failure.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
- Use max_t instead min_t for a minimal buffer.
- Pre-allocate 2 buffers on UP and 4 buffers on SMP systems.
- Use __GFP_RETRY_MAYFAIL | __GFP_NOWARN on memory allocation. The
allocation may fail under pressure and we will retry once.
- Add an error path to each caller of aa_get_buffer() if the memory
allocation fails.
security/apparmor/domain.c | 24 +++----
security/apparmor/file.c | 24 +++++--
security/apparmor/include/path.h | 49 +-------------
security/apparmor/lsm.c | 106 +++++++++++++++++++++++--------
security/apparmor/mount.c | 65 +++++++++++++++----
5 files changed, 160 insertions(+), 108 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index ca2dccf5b445e..cae0e619ff4fe 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -689,20 +689,9 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
} else if (COMPLAIN_MODE(profile)) {
/* no exec permission - learning mode */
struct aa_profile *new_profile = NULL;
- char *n = kstrdup(name, GFP_ATOMIC);
- if (n) {
- /* name is ptr into buffer */
- long pos = name - buffer;
- /* break per cpu buffer hold */
- put_buffers(buffer);
- new_profile = aa_new_null_profile(profile, false, n,
- GFP_KERNEL);
- get_buffers(buffer);
- name = buffer + pos;
- strcpy((char *)name, n);
- kfree(n);
- }
+ new_profile = aa_new_null_profile(profile, false, name,
+ GFP_KERNEL);
if (!new_profile) {
error = -ENOMEM;
info = "could not create null profile";
@@ -907,7 +896,12 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
ctx->nnp = aa_get_label(label);
/* buffer freed below, name is pointer into buffer */
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto done;
+ }
+
/* Test for onexec first as onexec override other x transitions. */
if (ctx->onexec)
new = handle_onexec(label, ctx->onexec, ctx->token,
@@ -979,7 +973,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
done:
aa_put_label(label);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index d0afed9ebd0ed..7b424e73a8c74 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -336,12 +336,14 @@ int aa_path_perm(const char *op, struct aa_label *label,
flags |= PATH_DELEGATE_DELETED | (S_ISDIR(cond->mode) ? PATH_IS_DIR :
0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
profile_path_perm(op, profile, path, buffer, request,
cond, flags, &perms));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -479,12 +481,18 @@ int aa_path_link(struct aa_label *label, struct dentry *old_dentry,
int error;
/* buffer freed below, lname is pointer in buffer */
- get_buffers(buffer, buffer2);
+ buffer = aa_get_buffer();
+ buffer2 = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !buffer2)
+ goto out;
+
error = fn_for_each_confined(label, profile,
profile_path_link(profile, &link, buffer, &target,
buffer2, &cond));
- put_buffers(buffer, buffer2);
-
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(buffer2);
return error;
}
@@ -528,7 +536,9 @@ static int __file_path_perm(const char *op, struct aa_label *label,
return 0;
flags = PATH_DELEGATE_DELETED | (S_ISDIR(cond.mode) ? PATH_IS_DIR : 0);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
/* check every profile in task label not in current cache */
error = fn_for_each_not_in_set(flabel, label, profile,
@@ -557,7 +567,7 @@ static int __file_path_perm(const char *op, struct aa_label *label,
if (!error)
update_file_ctx(file_ctx(file), label, request);
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
diff --git a/security/apparmor/include/path.h b/security/apparmor/include/path.h
index b6380c5f00972..b0b2ab85e42d8 100644
--- a/security/apparmor/include/path.h
+++ b/security/apparmor/include/path.h
@@ -15,7 +15,6 @@
#ifndef __AA_PATH_H
#define __AA_PATH_H
-
enum path_flags {
PATH_IS_DIR = 0x1, /* path is a directory */
PATH_CONNECT_PATH = 0x4, /* connect disconnected paths to / */
@@ -30,51 +29,7 @@ int aa_path_name(const struct path *path, int flags, char *buffer,
const char **name, const char **info,
const char *disconnected);
-#define MAX_PATH_BUFFERS 2
-
-/* Per cpu buffers used during mediation */
-/* preallocated buffers to use during path lookups */
-struct aa_buffers {
- char *buf[MAX_PATH_BUFFERS];
-};
-
-#include <linux/percpu.h>
-#include <linux/preempt.h>
-
-DECLARE_PER_CPU(struct aa_buffers, aa_buffers);
-
-#define ASSIGN(FN, A, X, N) ((X) = FN(A, N))
-#define EVAL1(FN, A, X) ASSIGN(FN, A, X, 0) /*X = FN(0)*/
-#define EVAL2(FN, A, X, Y...) \
- do { ASSIGN(FN, A, X, 1); EVAL1(FN, A, Y); } while (0)
-#define EVAL(FN, A, X...) CONCATENATE(EVAL, COUNT_ARGS(X))(FN, A, X)
-
-#define for_each_cpu_buffer(I) for ((I) = 0; (I) < MAX_PATH_BUFFERS; (I)++)
-
-#ifdef CONFIG_DEBUG_PREEMPT
-#define AA_BUG_PREEMPT_ENABLED(X) AA_BUG(preempt_count() <= 0, X)
-#else
-#define AA_BUG_PREEMPT_ENABLED(X) /* nop */
-#endif
-
-#define __get_buffer(C, N) ({ \
- AA_BUG_PREEMPT_ENABLED("__get_buffer without preempt disabled"); \
- (C)->buf[(N)]; })
-
-#define __get_buffers(C, X...) EVAL(__get_buffer, C, X)
-
-#define __put_buffers(X, Y...) ((void)&(X))
-
-#define get_buffers(X...) \
-do { \
- struct aa_buffers *__cpu_var = get_cpu_ptr(&aa_buffers); \
- __get_buffers(__cpu_var, X); \
-} while (0)
-
-#define put_buffers(X, Y...) \
-do { \
- __put_buffers(X, Y); \
- put_cpu_ptr(&aa_buffers); \
-} while (0)
+char *aa_get_buffer(void);
+void aa_put_buffer(char *buf);
#endif /* __AA_PATH_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92d..942702dc3c0a2 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -47,8 +47,13 @@
/* Flag indicating whether initialization completed */
int apparmor_initialized;
-DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
+union aa_buffer {
+ struct list_head list;
+ char buffer[1];
+};
+static LIST_HEAD(aa_global_buffers);
+static DEFINE_SPINLOCK(aa_buffers_lock);
/*
* LSM hook functions
@@ -1406,6 +1411,7 @@ static int param_set_aauint(const char *val, const struct kernel_param *kp)
return -EPERM;
error = param_set_uint(val, kp);
+ aa_g_path_max = max_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
return error;
@@ -1518,6 +1524,46 @@ static int param_set_mode(const char *val, const struct kernel_param *kp)
return 0;
}
+char *aa_get_buffer(void)
+{
+ union aa_buffer *aa_buf;
+ bool try_again = true;
+
+retry:
+ spin_lock(&aa_buffers_lock);
+ if (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ return &aa_buf->buffer[0];
+ }
+ spin_unlock(&aa_buffers_lock);
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL | __GFP_RETRY_MAYFAIL |
+ __GFP_NOWARN);
+ if (!aa_buf) {
+ if (try_again) {
+ try_again = false;
+ goto retry;
+ }
+ pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
+ return NULL;
+ }
+ return &aa_buf->buffer[0];
+}
+
+void aa_put_buffer(char *buf)
+{
+ union aa_buffer *aa_buf;
+
+ aa_buf = container_of(buf, union aa_buffer, buffer[0]);
+
+ spin_lock(&aa_buffers_lock);
+ list_add(&aa_buf->list, &aa_global_buffers);
+ spin_unlock(&aa_buffers_lock);
+}
+
/*
* AppArmor init functions
*/
@@ -1538,38 +1584,49 @@ static int __init set_init_ctx(void)
static void destroy_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- kfree(per_cpu(aa_buffers, i).buf[j]);
- per_cpu(aa_buffers, i).buf[j] = NULL;
- }
+ spin_lock(&aa_buffers_lock);
+ while (!list_empty(&aa_global_buffers)) {
+ aa_buf = list_first_entry(&aa_global_buffers, union aa_buffer,
+ list);
+ list_del(&aa_buf->list);
+ spin_unlock(&aa_buffers_lock);
+ kfree(aa_buf);
+ spin_lock(&aa_buffers_lock);
}
+ spin_unlock(&aa_buffers_lock);
}
static int __init alloc_buffers(void)
{
- u32 i, j;
+ union aa_buffer *aa_buf;
+ int i, num;
- for_each_possible_cpu(i) {
- for_each_cpu_buffer(j) {
- char *buffer;
+ /*
+ * A function may require two buffers at once. Usually the buffers are
+ * used for a short period of time and are shared. On UP kernel buffers
+ * two should be enough, with more CPUs it is possible that more
+ * buffers will be used simultaneously. The preallocated pool may grow.
+ * This preallocation has also the side-effect that AppArmor will be
+ * disabled early at boot if aa_g_path_max is extremly high.
+ */
+ if (num_online_cpus() > 1)
+ num = 4;
+ else
+ num = 2;
- if (cpu_to_node(i) > num_online_nodes())
- /* fallback to kmalloc for offline nodes */
- buffer = kmalloc(aa_g_path_max, GFP_KERNEL);
- else
- buffer = kmalloc_node(aa_g_path_max, GFP_KERNEL,
- cpu_to_node(i));
- if (!buffer) {
- destroy_buffers();
- return -ENOMEM;
- }
- per_cpu(aa_buffers, i).buf[j] = buffer;
+ for (i = 0; i < num; i++) {
+ gfp_t flags;
+
+ aa_buf = kmalloc(aa_g_path_max, GFP_KERNEL |
+ __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+ if (!aa_buf) {
+ destroy_buffers();
+ return -ENOMEM;
}
+ aa_put_buffer(&aa_buf->buffer[0]);
}
-
return 0;
}
@@ -1734,7 +1791,7 @@ static int __init apparmor_init(void)
error = alloc_buffers();
if (error) {
AA_ERROR("Unable to allocate work buffers\n");
- goto buffers_out;
+ goto alloc_out;
}
error = set_init_ctx();
@@ -1759,7 +1816,6 @@ static int __init apparmor_init(void)
buffers_out:
destroy_buffers();
-
alloc_out:
aa_destroy_aafs();
aa_teardown_dfa_engine();
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 8c3787399356b..267a26fba14e1 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -412,11 +412,13 @@ int aa_remount(struct aa_label *label, const struct path *path,
binary = path->dentry->d_sb->s_type->fs_flags & FS_BINARY_MOUNTDATA;
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, data, binary));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -441,11 +443,18 @@ int aa_bind_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || old_buffer)
+ goto out;
+
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, flags, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -465,11 +474,13 @@ int aa_mount_change_type(struct aa_label *label, const struct path *path,
flags &= (MS_REC | MS_SILENT | MS_SHARED | MS_PRIVATE | MS_SLAVE |
MS_UNBINDABLE);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, NULL, NULL, NULL,
flags, NULL, false));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -492,11 +503,17 @@ int aa_move_mount(struct aa_label *label, const struct path *path,
if (error)
return error;
- get_buffers(buffer, old_buffer);
+ buffer = aa_get_buffer();
+ old_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!buffer || !old_buffer)
+ goto out;
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, &old_path, old_buffer,
NULL, MS_MOVE, NULL, false));
- put_buffers(buffer, old_buffer);
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(old_buffer);
path_put(&old_path);
return error;
@@ -537,17 +554,29 @@ int aa_new_mount(struct aa_label *label, const char *dev_name,
}
}
- get_buffers(buffer, dev_buffer);
+ buffer = aa_get_buffer();
+ if (!buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
if (dev_path) {
error = fn_for_each_confined(label, profile,
match_mnt(profile, path, buffer, dev_path, dev_buffer,
type, flags, data, binary));
} else {
+ dev_buffer = aa_get_buffer();
+ if (!dev_buffer) {
+ error = -ENOMEM;
+ goto out;
+ }
error = fn_for_each_confined(label, profile,
match_mnt_path_str(profile, path, buffer, dev_name,
type, flags, data, binary, NULL));
}
- put_buffers(buffer, dev_buffer);
+
+out:
+ aa_put_buffer(buffer);
+ aa_put_buffer(dev_buffer);
if (dev_path)
path_put(dev_path);
@@ -595,10 +624,13 @@ int aa_umount(struct aa_label *label, struct vfsmount *mnt, int flags)
AA_BUG(!label);
AA_BUG(!mnt);
- get_buffers(buffer);
+ buffer = aa_get_buffer();
+ if (!buffer)
+ return -ENOMEM;
+
error = fn_for_each_confined(label, profile,
profile_umount(profile, &path, buffer));
- put_buffers(buffer);
+ aa_put_buffer(buffer);
return error;
}
@@ -671,7 +703,11 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
AA_BUG(!old_path);
AA_BUG(!new_path);
- get_buffers(old_buffer, new_buffer);
+ old_buffer = aa_get_buffer();
+ new_buffer = aa_get_buffer();
+ error = -ENOMEM;
+ if (!old_buffer || !new_buffer)
+ goto out;
target = fn_label_build(label, profile, GFP_ATOMIC,
build_pivotroot(profile, new_path, new_buffer,
old_path, old_buffer));
@@ -690,7 +726,8 @@ int aa_pivotroot(struct aa_label *label, const struct path *old_path,
/* already audited error */
error = PTR_ERR(target);
out:
- put_buffers(old_buffer, new_buffer);
+ aa_put_buffer(old_buffer);
+ aa_put_buffer(new_buffer);
return error;
--
2.20.1
^ permalink raw reply related
* [ANNOUNCE][CFP] Linux Security Summit Europe 2019
From: Elena Reshetova @ 2019-05-03 11:46 UTC (permalink / raw)
To: linux-kernel, linux-crypto, selinux, linux-security-module,
linux-integrity
==============================================================================
ANNOUNCEMENT AND CALL FOR PARTICIPATION
LINUX SECURITY SUMMIT EUROPE 2019
31 October – 1 November
Lyon, France
==============================================================================
DESCRIPTION
The Linux Security Summit (LSS) is a technical forum for collaboration
between Linux developers, researchers, and end users. Its primary aim is to
foster community efforts in analyzing and solving Linux security challenges.
This year, for the second time, the Linux Security Summit is going to
be also held in Europe (LSS-EU) in order to facilitate broader participation
in Linux security development.
Similar to LSS-North America, LSS-EU provides a unique opportunity for to have
discussions and networking opportunities with key people in the Linux kernel
security community, present your work and ideas and affect the future direction
of Linux security.
The program committee currently seeks proposals for:
* Refereed Presentations:
45 minutes in length.
* Panel Discussion Topics:
45 minutes in length.
* Short Topics:
30 minutes in total, including at least 10 minutes discussion.
* BoF Sessions.
* Tutorials *NEW for 2019*:
90 minutes in length.
Tutorial sessions should be focused on advanced Linux security defense
topics within areas such as the kernel, compiler, and security-related
libraries. Priority will be given to tutorials created for
this conference.
Topic areas include, but are not limited to:
* Kernel self-protection
* Access control
* Cryptography and key management
* Integrity control
* Hardware Security
* Iot and embedded security
* Virtualization and containers
* System-specific system hardening
* Case studies
* Security tools
* Security UX
* Emerging technologies, threats & techniques
Proposals should be submitted via:
https://linuxfoundation.smapply.io/prog/lss_eu_2019/
DATES
* CFP Close: Wednesday , July 31, 2019
* CFP Notifications: Friday, August 9, 2019
* Schedule Announced: Monday, August 19, 2019
* Event: October 31 – November 1, 2019
WHO SHOULD ATTEND
We're seeking a diverse range of attendees, and welcome participation by
people involved in Linux security development, operations, and research.
The LSS is a unique global event which provides the opportunity to present
and discuss your work or research with key Linux security community members
and maintainers. It?s also useful for those who wish to keep up with the
latest in Linux security development, and to provide input to the
development process.
WEB SITE
https://events.linuxfoundation.org/events/linux-security-summit-europe-2019/
TWITTER
For event updates and announcements, follow:
https://twitter.com/LinuxSecSummit
PROGRAM COMMITTEE
The program committee for LSS-EU 2019 is:
* Elena Reshetova, Intel
* James Morris, Microsoft
* Serge Hallyn, Cisco
* Paul Moore, Cisco
* Stephen Smalley, NSA
* John Johansen, Canonical
* Kees Cook, Google
* Casey Schaufler, Intel
* Mimi Zohar, IBM
* David A. Wheeler, Institute for Defense Analyses
The program committee may be contacted as a group via email:
lss-pc () lists.linuxfoundation.org
^ permalink raw reply
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: joeyli @ 2019-05-03 8:58 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Peter Jones, Matthew Garrett, Lee, Chun-Yi, Mimi Zohar,
David Howells, James Morris, Serge E . Hallyn, Josh Boyer,
Nayna Jain, linux-efi, linux-security-module,
Linux Kernel Mailing List
In-Reply-To: <CAKv+Gu9CdWYMELxBz9WqaB4BSRbRx81iR-x4P+2OANAcfLUhUQ@mail.gmail.com>
On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> On Fri, 3 May 2019 at 09:18, joeyli <jlee@suse.com> wrote:
> >
> > Hi Ard,
> >
> > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> 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.
> > > >
> > > > v3.
> > > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > > [ 1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > [ 1.501773] blacklist: Loaded blacklisting hash
> > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > >
> > > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > > >
> > > > v2.
> > > > Setting the MODSIGN messages 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>
> > > > ---
> > > > certs/blacklist.c | 3 +-
> > > > security/integrity/platform_certs/load_uefi.c | 40 +++++++++++++++++++--------
> > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > index 3a507b9e2568..f91437e39e44 100644
> > > > --- a/certs/blacklist.c
> > > > +++ b/certs/blacklist.c
> > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > if (IS_ERR(key)) {
> > > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > > > return PTR_ERR(key);
> > > > - }
> > > > + } else
> > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > return 0;
> > > > }
> > > >
> > > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > @@ -1,5 +1,7 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > >
> > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > +
> > > > #include <linux/kernel.h>
> > > > #include <linux/sched.h>
> > > > #include <linux/cred.h>
> > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > return status == EFI_SUCCESS;
> > > > }
> > > >
> > > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > > +{
> > > > + int i = 0;
> > > > +
> > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > + str[i] = str16[i];
> > > > + i++;
> > > > + }
> > > > +
> > > > + str[i] = '\0';
> > > > +}
> > > > +
> > > > /*
> > > > * Get a certificate list blob from the named EFI variable.
> > > > */
> > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > > efi_status_t status;
> > > > unsigned long lsize = 4;
> > > > unsigned long tmpdb[4];
> > > > + char namestr[16];
> > > > void *db;
> > > >
> > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > >
> > > Please drop this (and the function above) - instead, just return NULL
> > > if the variable is not found (without reporting an error).
> > >
> >
> > This name string is for printing debug level message, not error message.
> > This function already returns NULL when EFI_NOT_FOUND be returned by
> > firmware.
> >
> > > > 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_debug("UEFI %s list doesn't exist\n", namestr);
> > > > + else
> > > > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > > > + namestr, efi_status_to_str(status));
> > > > return NULL;
> >
> > here returns NULL when EFI_NOT_FOUND. The message of existence is for
> > debugging.
> >
>
> I understand that. But I don't think we need it.
>
OK. I will remove the debug message.
> > > > }
> > > > + pr_debug("UEFI %s list exists\n", namestr);
> > > >
> > > > db = kmalloc(lsize, GFP_KERNEL);
> > > > if (!db)
> > > > @@ -59,7 +80,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 UEFI %s list: %s\n",
> > > > + namestr, efi_status_to_str(status));
> > > > return NULL;
> > > > }
> > > >
> > > > @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source, const void *data,
> > > > static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > const void *data, size_t len)
> > > > {
> > > > + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> > > > uefi_blacklist_hash(source, data, len, "tbs:", 4);
> > > > }
> > > >
> > > > @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > static __init void uefi_blacklist_binary(const char *source,
> > > > const void *data, size_t len)
> > > > {
> > > > + pr_info("Blacklisting hash of an executable: %s\n", source);
> > > > uefi_blacklist_hash(source, data, len, "bin:", 4);
> > > > }
> > > >
> > >
> > > These are separate changes - I don't have an opinion whether they are
> > > appropriate or not, but they should be in a separate patch.
> > >
> >
> > I will move the message of blacklising hash to other patch. Thanks!
> >
> > > > @@ -154,9 +178,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");
> > > > - } else {
> > > > + if (db) {
> > > > rc = parse_efi_signature_list("UEFI:db",
> > > > db, dbsize, get_handler_for_db);
> > > > if (rc)
> > > > @@ -167,9 +189,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");
> > > > - } else {
> > > > + if (mok) {
> > > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > > mok, moksize, get_handler_for_db);
> > > > if (rc)
> > > > @@ -178,9 +198,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");
> > > > - } else {
> > > > + if (dbx) {
> > > > rc = parse_efi_signature_list("UEFI:dbx",
> > > > dbx, dbxsize,
> > > > get_handler_for_dbx);
> > > > --
> > > > 2.16.4
> > > >
> > >
> > > I think we should consider carefully what it means if some of these
> > > variables don't exist:
> > > - if secure boot is enabled, db and dbx must exist, so if they don't,
> > > something is wrong
> >
> > The existence of db/dbx is not related to secure boot. If manufacturer/user
> > enrolled certificate/hash to db or dbx, then the variable will be created.
> > If user didn't enroll anything to db/dbx, then variables will not show up.
> >
>
> Yes, but if secure boot is enabled and db is empty, how could you have
> booted in the first place?
>
I agree. When secure boot enabled, kernel can not be booted without db.
> And what about the converse case: if secure boot is not enabled, why
> should we trust the contents of db?
>
The db and dbx are authenticated variables that it protected by KEK.
So it can be trusted even secure boot is disabled. Unless manufacturer
or user's KEK is leaked.
> > > - secure boot might be enabled but we may be booting without shim.
> >
> > Shim always creates MokListRT no matter secure boot enabled or disabled.
> >
>
> That is not my point. What happens if you booted with secure boot
> enabled but without the help of shim?
>
Without shim, the signed EFI stub can still be booted by EFI boot manager.
But the MokListRT will not be created for runtime. So MOK signed kernel
module can not be verified. (or IMA can not verify MOK signed kernel image
for kexec...)
> > > - secure boot might be disabled.
> > >
> >
> > It's not about secure boot, db/dbx/MokListRT are always available at
> > runtime if user was enrolled something to those list.
> >
>
> Yes, but again, depending on whether shim was involved, and/or whether
> secure boot was enabled or not, the way we interpret these things may
> be very different.
>
> I want the reasoning to be sound before merging any patches that deal
> with these variables.
Here is a simple summary:
When secure boot is enabled:
- db/dbx: Can be trusted because they are authenticated variables.
(unless end user doesn't want to trust db/dbx)
- MokListRT:
- with shim: MokListRT will be created. It can be trusted.
- without shim: MokListRT will not be created.
MOK protected kernel module or file can not be
verified.
When secure boot is disabled:
- db/dbx: Can be trusted because they are authenticated variables.
(unless end user doesn't want to trust db/dbx)
- MokListRT:
- with shim: MokListRT will be created. But it can not be trusted.
MOK protected kernel module or file can not be
verified.
- without shim: MokListRT will not be created.
MOK protected kernel module or file can not be
verified.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: Ard Biesheuvel @ 2019-05-03 8:07 UTC (permalink / raw)
To: joeyli, Peter Jones, Matthew Garrett
Cc: Lee, Chun-Yi, Mimi Zohar, David Howells, James Morris,
Serge E . Hallyn, Josh Boyer, Nayna Jain, linux-efi,
linux-security-module, Linux Kernel Mailing List
In-Reply-To: <20190503071819.GN11486@linux-l9pv.suse>
On Fri, 3 May 2019 at 09:18, joeyli <jlee@suse.com> wrote:
>
> Hi Ard,
>
> On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> 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.
> > >
> > > v3.
> > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > [ 1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > [ 1.501773] blacklist: Loaded blacklisting hash
> > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > >
> > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > >
> > > v2.
> > > Setting the MODSIGN messages 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>
> > > ---
> > > certs/blacklist.c | 3 +-
> > > security/integrity/platform_certs/load_uefi.c | 40 +++++++++++++++++++--------
> > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > index 3a507b9e2568..f91437e39e44 100644
> > > --- a/certs/blacklist.c
> > > +++ b/certs/blacklist.c
> > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > if (IS_ERR(key)) {
> > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > > return PTR_ERR(key);
> > > - }
> > > + } else
> > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > return 0;
> > > }
> > >
> > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > index 81b19c52832b..6b6996e5bc27 100644
> > > --- a/security/integrity/platform_certs/load_uefi.c
> > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > @@ -1,5 +1,7 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > >
> > > +#define pr_fmt(fmt) "EFI: "fmt
> > > +
> > > #include <linux/kernel.h>
> > > #include <linux/sched.h>
> > > #include <linux/cred.h>
> > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > return status == EFI_SUCCESS;
> > > }
> > >
> > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > +{
> > > + int i = 0;
> > > +
> > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > + str[i] = str16[i];
> > > + i++;
> > > + }
> > > +
> > > + str[i] = '\0';
> > > +}
> > > +
> > > /*
> > > * Get a certificate list blob from the named EFI variable.
> > > */
> > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > efi_status_t status;
> > > unsigned long lsize = 4;
> > > unsigned long tmpdb[4];
> > > + char namestr[16];
> > > void *db;
> > >
> > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> >
> > Please drop this (and the function above) - instead, just return NULL
> > if the variable is not found (without reporting an error).
> >
>
> This name string is for printing debug level message, not error message.
> This function already returns NULL when EFI_NOT_FOUND be returned by
> firmware.
>
> > > 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_debug("UEFI %s list doesn't exist\n", namestr);
> > > + else
> > > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > > + namestr, efi_status_to_str(status));
> > > return NULL;
>
> here returns NULL when EFI_NOT_FOUND. The message of existence is for
> debugging.
>
I understand that. But I don't think we need it.
> > > }
> > > + pr_debug("UEFI %s list exists\n", namestr);
> > >
> > > db = kmalloc(lsize, GFP_KERNEL);
> > > if (!db)
> > > @@ -59,7 +80,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 UEFI %s list: %s\n",
> > > + namestr, efi_status_to_str(status));
> > > return NULL;
> > > }
> > >
> > > @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source, const void *data,
> > > static __init void uefi_blacklist_x509_tbs(const char *source,
> > > const void *data, size_t len)
> > > {
> > > + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> > > uefi_blacklist_hash(source, data, len, "tbs:", 4);
> > > }
> > >
> > > @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
> > > static __init void uefi_blacklist_binary(const char *source,
> > > const void *data, size_t len)
> > > {
> > > + pr_info("Blacklisting hash of an executable: %s\n", source);
> > > uefi_blacklist_hash(source, data, len, "bin:", 4);
> > > }
> > >
> >
> > These are separate changes - I don't have an opinion whether they are
> > appropriate or not, but they should be in a separate patch.
> >
>
> I will move the message of blacklising hash to other patch. Thanks!
>
> > > @@ -154,9 +178,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");
> > > - } else {
> > > + if (db) {
> > > rc = parse_efi_signature_list("UEFI:db",
> > > db, dbsize, get_handler_for_db);
> > > if (rc)
> > > @@ -167,9 +189,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");
> > > - } else {
> > > + if (mok) {
> > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > mok, moksize, get_handler_for_db);
> > > if (rc)
> > > @@ -178,9 +198,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");
> > > - } else {
> > > + if (dbx) {
> > > rc = parse_efi_signature_list("UEFI:dbx",
> > > dbx, dbxsize,
> > > get_handler_for_dbx);
> > > --
> > > 2.16.4
> > >
> >
> > I think we should consider carefully what it means if some of these
> > variables don't exist:
> > - if secure boot is enabled, db and dbx must exist, so if they don't,
> > something is wrong
>
> The existence of db/dbx is not related to secure boot. If manufacturer/user
> enrolled certificate/hash to db or dbx, then the variable will be created.
> If user didn't enroll anything to db/dbx, then variables will not show up.
>
Yes, but if secure boot is enabled and db is empty, how could you have
booted in the first place?
And what about the converse case: if secure boot is not enabled, why
should we trust the contents of db?
> > - secure boot might be enabled but we may be booting without shim.
>
> Shim always creates MokListRT no matter secure boot enabled or disabled.
>
That is not my point. What happens if you booted with secure boot
enabled but without the help of shim?
> > - secure boot might be disabled.
> >
>
> It's not about secure boot, db/dbx/MokListRT are always available at
> runtime if user was enrolled something to those list.
>
Yes, but again, depending on whether shim was involved, and/or whether
secure boot was enabled or not, the way we interpret these things may
be very different.
I want the reasoning to be sound before merging any patches that deal
with these variables.
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status value to string
From: joeyli @ 2019-05-03 8:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Lee, Chun-Yi, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar, linux-efi,
linux-security-module, Linux Kernel Mailing List, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <CAKv+Gu9KGE3dpkZ1J9B0_VX2AsiTnE3Zbs1kAcz-xeOcXfxYog@mail.gmail.com>
On Fri, May 03, 2019 at 08:16:42AM +0200, Ard Biesheuvel wrote:
> On Fri, 3 May 2019 at 08:15, joeyli <jlee@suse.com> wrote:
> >
> > On Thu, May 02, 2019 at 10:53:31AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > > >
> > > > This function can be used to convert EFI status value to string
> > > > for printing out debug message. Using this function can improve
> > > > the readability of log.
> > > >
> > > > v2.
> > >
> > > Please move the changelog out of the commit log (move it below the ---
> > > further down)
> > >
> >
> > OK! I will moved the changelog out of the commit log.
> >
> > > > - Changed the wording in subject and description.
> > > > - Moved the marco immediately after the status value definitions.
> > > > - Turned into a proper function instead of inline.
> > > >
> > >
> > > You missed my point here. A proper function means the function in a .c
> > > file, and only the declaration in a .h file. This way, you are still
> > > duplicating the literal strings into every object file that references
> > > this function.
> > >
> >
> > Sorry for I missunderstood. I will move the function to load_uefi.c.
> >
>
> No, please move it to a file that is shared between all EFI code.
I see! I will move the function to the position that it is just behind
efi_status_to_str() in drivers/firmware/efi/efi.c. Please let me know
if the position is not suitable.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: joeyli @ 2019-05-03 7:18 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Lee, Chun-Yi, Mimi Zohar, David Howells, James Morris,
Serge E . Hallyn, Josh Boyer, Nayna Jain, linux-efi,
linux-security-module, Linux Kernel Mailing List
In-Reply-To: <CAKv+Gu8MESd3BXCKR=EH7Z1kWegm9XjTP38jBsizpgDAuyA3YQ@mail.gmail.com>
Hi Ard,
On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> 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.
> >
> > v3.
> > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > [ 1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > [ 1.501773] blacklist: Loaded blacklisting hash
> > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> >
> > - Setting messages for the existence of db/mok/dbx lists to debug level.
> >
> > v2.
> > Setting the MODSIGN messages 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>
> > ---
> > certs/blacklist.c | 3 +-
> > security/integrity/platform_certs/load_uefi.c | 40 +++++++++++++++++++--------
> > 2 files changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > index 3a507b9e2568..f91437e39e44 100644
> > --- a/certs/blacklist.c
> > +++ b/certs/blacklist.c
> > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > if (IS_ERR(key)) {
> > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > return PTR_ERR(key);
> > - }
> > + } else
> > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > return 0;
> > }
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..6b6996e5bc27 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -1,5 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > +#define pr_fmt(fmt) "EFI: "fmt
> > +
> > #include <linux/kernel.h>
> > #include <linux/sched.h>
> > #include <linux/cred.h>
> > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > return status == EFI_SUCCESS;
> > }
> >
> > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > +{
> > + int i = 0;
> > +
> > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > + str[i] = str16[i];
> > + i++;
> > + }
> > +
> > + str[i] = '\0';
> > +}
> > +
> > /*
> > * Get a certificate list blob from the named EFI variable.
> > */
> > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > efi_status_t status;
> > unsigned long lsize = 4;
> > unsigned long tmpdb[4];
> > + char namestr[16];
> > void *db;
> >
> > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
>
> Please drop this (and the function above) - instead, just return NULL
> if the variable is not found (without reporting an error).
>
This name string is for printing debug level message, not error message.
This function already returns NULL when EFI_NOT_FOUND be returned by
firmware.
> > 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_debug("UEFI %s list doesn't exist\n", namestr);
> > + else
> > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > + namestr, efi_status_to_str(status));
> > return NULL;
here returns NULL when EFI_NOT_FOUND. The message of existence is for
debugging.
> > }
> > + pr_debug("UEFI %s list exists\n", namestr);
> >
> > db = kmalloc(lsize, GFP_KERNEL);
> > if (!db)
> > @@ -59,7 +80,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 UEFI %s list: %s\n",
> > + namestr, efi_status_to_str(status));
> > return NULL;
> > }
> >
> > @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source, const void *data,
> > static __init void uefi_blacklist_x509_tbs(const char *source,
> > const void *data, size_t len)
> > {
> > + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> > uefi_blacklist_hash(source, data, len, "tbs:", 4);
> > }
> >
> > @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
> > static __init void uefi_blacklist_binary(const char *source,
> > const void *data, size_t len)
> > {
> > + pr_info("Blacklisting hash of an executable: %s\n", source);
> > uefi_blacklist_hash(source, data, len, "bin:", 4);
> > }
> >
>
> These are separate changes - I don't have an opinion whether they are
> appropriate or not, but they should be in a separate patch.
>
I will move the message of blacklising hash to other patch. Thanks!
> > @@ -154,9 +178,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");
> > - } else {
> > + if (db) {
> > rc = parse_efi_signature_list("UEFI:db",
> > db, dbsize, get_handler_for_db);
> > if (rc)
> > @@ -167,9 +189,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");
> > - } else {
> > + if (mok) {
> > rc = parse_efi_signature_list("UEFI:MokListRT",
> > mok, moksize, get_handler_for_db);
> > if (rc)
> > @@ -178,9 +198,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");
> > - } else {
> > + if (dbx) {
> > rc = parse_efi_signature_list("UEFI:dbx",
> > dbx, dbxsize,
> > get_handler_for_dbx);
> > --
> > 2.16.4
> >
>
> I think we should consider carefully what it means if some of these
> variables don't exist:
> - if secure boot is enabled, db and dbx must exist, so if they don't,
> something is wrong
The existence of db/dbx is not related to secure boot. If manufacturer/user
enrolled certificate/hash to db or dbx, then the variable will be created.
If user didn't enroll anything to db/dbx, then variables will not show up.
> - secure boot might be enabled but we may be booting without shim.
Shim always creates MokListRT no matter secure boot enabled or disabled.
> - secure boot might be disabled.
>
It's not about secure boot, db/dbx/MokListRT are always available at
runtime if user was enrolled something to those list.
> Tweaking the severity of error messages without having a clear idea of
> the policy we are aiming to implement is likely to cause trouble down
> the road, so perhaps someone could explain what this code does, and
> how it should behave in the above circumstances.
Thanks
Joey Lee
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status value to string
From: Ard Biesheuvel @ 2019-05-03 6:16 UTC (permalink / raw)
To: joeyli
Cc: Lee, Chun-Yi, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar, linux-efi,
linux-security-module, Linux Kernel Mailing List, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <20190503061518.GM11486@linux-l9pv.suse>
On Fri, 3 May 2019 at 08:15, joeyli <jlee@suse.com> wrote:
>
> On Thu, May 02, 2019 at 10:53:31AM +0200, Ard Biesheuvel wrote:
> > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > >
> > > This function can be used to convert EFI status value to string
> > > for printing out debug message. Using this function can improve
> > > the readability of log.
> > >
> > > v2.
> >
> > Please move the changelog out of the commit log (move it below the ---
> > further down)
> >
>
> OK! I will moved the changelog out of the commit log.
>
> > > - Changed the wording in subject and description.
> > > - Moved the marco immediately after the status value definitions.
> > > - Turned into a proper function instead of inline.
> > >
> >
> > You missed my point here. A proper function means the function in a .c
> > file, and only the declaration in a .h file. This way, you are still
> > duplicating the literal strings into every object file that references
> > this function.
> >
>
> Sorry for I missunderstood. I will move the function to load_uefi.c.
>
No, please move it to a file that is shared between all EFI code.
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status value to string
From: joeyli @ 2019-05-03 6:15 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Lee, Chun-Yi, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar, linux-efi,
linux-security-module, Linux Kernel Mailing List, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <CAKv+Gu9mjtNEWN-w4ix7VJMZ_kk-Qf6FfYFRu2mCosaAjMA4Vg@mail.gmail.com>
On Thu, May 02, 2019 at 10:53:31AM +0200, Ard Biesheuvel wrote:
> On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > This function can be used to convert EFI status value to string
> > for printing out debug message. Using this function can improve
> > the readability of log.
> >
> > v2.
>
> Please move the changelog out of the commit log (move it below the ---
> further down)
>
OK! I will moved the changelog out of the commit log.
> > - Changed the wording in subject and description.
> > - Moved the marco immediately after the status value definitions.
> > - Turned into a proper function instead of inline.
> >
>
> You missed my point here. A proper function means the function in a .c
> file, and only the declaration in a .h file. This way, you are still
> duplicating the literal strings into every object file that references
> this function.
>
Sorry for I missunderstood. I will move the function to load_uefi.c.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Jarkko Sakkinen @ 2019-05-03 6:12 UTC (permalink / raw)
To: Ingo Molnar
Cc: Matthew Garrett, Ard Biesheuvel, Bartosz Szczepanek,
linux-integrity, Peter Huewe, Jason Gunthorpe, Roberto Sassu,
linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <20190503060218.GA28048@gmail.com>
On Fri, May 03, 2019 at 08:02:18AM +0200, Ingo Molnar wrote:
>
> * Matthew Garrett <mjg59@google.com> wrote:
>
> > On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > (+ Ingo)
> > >
> > > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett <mjg59@google.com> wrote:
> > > >
> > > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek <bsz@semihalf.com> wrote:
> > > > >
> > > > > I may be a little late with this comment, but I've just tested these
> > > > > patches on aarch64 platform (from the top of jjs/master) and got
> > > > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > > > mail). I think there's problem with below call to
> > > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > > > passed as (void *) and never remapped:
> > > >
> > > > Yes, it looks like this is just broken. Can you try with the attached patch?
> > >
> > > I'm a bit uncomfortable with EFI code that is obviously broken and
> > > untested being queued for the next merge window in another tree.
> >
> > The patchset was Cc:ed to linux-efi@. Is there anything else I should
> > have done to ensure you picked it up rather than Jarkko?
>
> That's not the workflow rule the Linux kernel is using, if Cc:-ing a
> patchset was the only condition for upstream inclusion then we'd have a
> *LOT* of crap in the Linux kernel.
>
> Just applying those EFI changes without even as much as an Acked-by from
> the EFI maintainers is a *totally* unacceptable workflow.
>
> Please revert/rebase and re-try this on the proper submission channels.
>
> Meanwhile the broken code is NAK-ed by me:
>
> Nacked-by: Ingo Molnar <mingo@kernel.org>
There must be some kind of misconception here. None of the changes have
been submitted so far. They are only in my master branch. They briefly
went to linux-next through my next branch but as soon as issues were
reported I wiped them off from there (which happened like 2-3 weeks
ago). They haven't been part off any of my PR's.
There is nothing to revert.
/Jarkko
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Ingo Molnar @ 2019-05-03 6:02 UTC (permalink / raw)
To: Matthew Garrett
Cc: Ard Biesheuvel, Jarkko Sakkinen, Bartosz Szczepanek,
linux-integrity, Peter Huewe, Jason Gunthorpe, Roberto Sassu,
linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <CACdnJuvDuw0X9iwEqOu7EjM5ca1f+n7f=xqzrTPS9PyrmqKNHQ@mail.gmail.com>
* Matthew Garrett <mjg59@google.com> wrote:
> On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > (+ Ingo)
> >
> > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett <mjg59@google.com> wrote:
> > >
> > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek <bsz@semihalf.com> wrote:
> > > >
> > > > I may be a little late with this comment, but I've just tested these
> > > > patches on aarch64 platform (from the top of jjs/master) and got
> > > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > > mail). I think there's problem with below call to
> > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > > passed as (void *) and never remapped:
> > >
> > > Yes, it looks like this is just broken. Can you try with the attached patch?
> >
> > I'm a bit uncomfortable with EFI code that is obviously broken and
> > untested being queued for the next merge window in another tree.
>
> The patchset was Cc:ed to linux-efi@. Is there anything else I should
> have done to ensure you picked it up rather than Jarkko?
That's not the workflow rule the Linux kernel is using, if Cc:-ing a
patchset was the only condition for upstream inclusion then we'd have a
*LOT* of crap in the Linux kernel.
Just applying those EFI changes without even as much as an Acked-by from
the EFI maintainers is a *totally* unacceptable workflow.
Please revert/rebase and re-try this on the proper submission channels.
Meanwhile the broken code is NAK-ed by me:
Nacked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Jarkko Sakkinen @ 2019-05-03 5:51 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Matthew Garrett, Ingo Molnar, Bartosz Szczepanek, linux-integrity,
Peter Huewe, Jason Gunthorpe, Roberto Sassu, linux-efi, LSM List,
Linux Kernel Mailing List, Thiébaud Weksteen
In-Reply-To: <CAKv+Gu9PF4u=-7QL4e36Q3S5kC4+5Z=yLYHLT9jE+eNY7YUV7A@mail.gmail.com>
On Thu, May 02, 2019 at 09:14:49AM +0200, Ard Biesheuvel wrote:
> (+ Ingo)
>
> On Tue, 30 Apr 2019 at 21:52, Matthew Garrett <mjg59@google.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek <bsz@semihalf.com> wrote:
> > >
> > > I may be a little late with this comment, but I've just tested these
> > > patches on aarch64 platform (from the top of jjs/master) and got
> > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > mail). I think there's problem with below call to
> > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > passed as (void *) and never remapped:
> >
> > Yes, it looks like this is just broken. Can you try with the attached patch?
>
> I'm a bit uncomfortable with EFI code that is obviously broken and
> untested being queued for the next merge window in another tree.
>
> What is currently queued there? Can we revert this change for the time
> being, and resubmit it via the EFI tree for v5.3?
Nothing ATM. The broken code is in my master branch ATM. Nothing
in my next branch nor have I included anything to my PRs. Should
be nothing to worry about in that sense :-)
/Jarkko
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Jarkko Sakkinen @ 2019-05-03 5:49 UTC (permalink / raw)
To: Matthew Garrett
Cc: Bartosz Szczepanek, linux-integrity, Peter Huewe, Jason Gunthorpe,
Roberto Sassu, linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <CACdnJuv1vwi7hgyGb-7a0dKfRhwdXTgj2baxO=qzpB_JJ1a9XQ@mail.gmail.com>
On Thu, May 02, 2019 at 11:03:08AM -0700, Matthew Garrett wrote:
> On Thu, May 2, 2019 at 1:32 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 03:07:09PM +0200, Bartosz Szczepanek wrote:
> > > I may be a little late with this comment, but I've just tested these
> > > patches on aarch64 platform (from the top of jjs/master) and got
> > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > mail). I think there's problem with below call to
> > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > passed as (void *) and never remapped:
> >
> > Not late. This is not part of any PR yet. Thank you for the
> > feedback!
> >
> > Matthew, can you send an updated version of the whole patch set
> > with fixes to this issue and also reordering of the includes?
>
> Yes, I'll resend and let's do this again for 5.3.
Agreed, better not rush but get it right once.
/Jarkko
^ permalink raw reply
* Re: [PATCH] kexec_buffer measure
From: Tetsuo Handa @ 2019-05-03 0:53 UTC (permalink / raw)
To: Casey Schaufler, Mimi Zohar, prakhar srivastava
Cc: linux-integrity, linux-kernel, linux-security-module, Paul Moore,
John Johansen
In-Reply-To: <7af61ebe-28a8-799c-fe47-d72f247494ed@schaufler-ca.com>
On 2019/05/03 1:28, Casey Schaufler wrote:
> On 5/2/2019 8:48 AM, Mimi Zohar wrote:
>> [Cc'ing Paul, John, Casey]
>>
>> On Mon, 2019-04-22 at 20:18 -0400, Mimi Zohar wrote:
>>> [Cc'ing LSM mailing list]
>>>
>>> On Fri, 2019-04-19 at 17:30 -0700, prakhar srivastava wrote:
>>>
>>>> 2) Adding a LSM hook
>>>> We are doing both the command line and kernel version measurement in IMA.
>>>> Can you please elaborate on how this can be used outside of the scenario?
>>>> That will help me come back with a better design and code. I am
>>>> neutral about this.
>>> As I said previously, initially you might want to only measure the
>>> kexec boot command line, but will you ever want to verify or audit log
>>> the boot command line hash? Perhaps LSMs would be interested in the
>>> boot command line. Should this be an LSM hook?
>> From an LSM perspective, is there any interest in the boot command line?
>
> I can imagine an LSM that cares about the command line,
> but I don't have interest in it for any work I have in progress.
>
Since the kernel command line controls which LSMs to enable, I doubt that
an LSM which cares about the command line can detect that the kernel command
line was tampered when the kernel command line was tampered...
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: Andy Lutomirski @ 2019-05-03 0:34 UTC (permalink / raw)
To: James Morris
Cc: Matthew Garrett, LSM List, Linux Kernel Mailing List,
David Howells, Linux API, Andy Lutomirski
In-Reply-To: <alpine.LRH.2.21.1905030901180.7491@namei.org>
> On May 2, 2019, at 4:19 PM, James Morris <jmorris@namei.org> wrote:
>
>> On Thu, 2 May 2019, Matthew Garrett wrote:
>>
>>> On Thu, May 2, 2019 at 2:07 PM James Morris <jmorris@namei.org> wrote:
>>> One possible direction is to (as previously mentioned) assign IDs to each
>>> callsite and be able to check this ID against a simple policy array
>>> (allow/deny). The default policy choices could be reduced to 'all' or
>>> 'none' during kconfig, and allow a custom policy to be loaded later if
>>> desired.
>>
>> Ok. My primary concern around this is that it's very difficult to use
>> correctly in anything other than the "all" or "none" modes. If a new
>> kernel feature is added with integrated lockdown support, if an admin
>> is simply setting the flags of things they wish to block then this
>> will be left enabled - and may violate the admin's expectations around
>> integrity. On the other hand, if an admin is simply setting the flags
>> of things they wish to permit, then adding lockdown support to an
>> existing kernel feature may result in that feature suddenly being
>> disabled, which may also violate the admin's expectations around the
>> flags providing a stable set of behaviour.
>
> Understood. Most uses will likely be either a distro or an embedded
> system, who I'm assuming would provide a useful policy by default, and
> perhaps a high-level abstraction for modification.
>
>> Given that, would you prefer such a policy expression to look like?
>
> Perhaps a write-once policy, injected from userspace during early boot?
>
> The policy could be simply a list of:
>
> lockdown_feature true|false
>
I’m not convinced this is worthwhile. As I see it, there really are only two privileges here: root can read kernel memory, and root can corrupt kernel state. A policy that root can’t corrupt kernel memory except using, say, eBPF is useless — it gives warm fuzzy feelings but nothing else.
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: James Morris @ 2019-05-02 23:19 UTC (permalink / raw)
To: Matthew Garrett
Cc: LSM List, Linux Kernel Mailing List, David Howells, Linux API,
Andy Lutomirski
In-Reply-To: <CACdnJuusGU2DMXaPAjH3+QOcSj-9q6njbxxG-9s2PweDKognvw@mail.gmail.com>
On Thu, 2 May 2019, Matthew Garrett wrote:
> On Thu, May 2, 2019 at 2:07 PM James Morris <jmorris@namei.org> wrote:
> > One possible direction is to (as previously mentioned) assign IDs to each
> > callsite and be able to check this ID against a simple policy array
> > (allow/deny). The default policy choices could be reduced to 'all' or
> > 'none' during kconfig, and allow a custom policy to be loaded later if
> > desired.
>
> Ok. My primary concern around this is that it's very difficult to use
> correctly in anything other than the "all" or "none" modes. If a new
> kernel feature is added with integrated lockdown support, if an admin
> is simply setting the flags of things they wish to block then this
> will be left enabled - and may violate the admin's expectations around
> integrity. On the other hand, if an admin is simply setting the flags
> of things they wish to permit, then adding lockdown support to an
> existing kernel feature may result in that feature suddenly being
> disabled, which may also violate the admin's expectations around the
> flags providing a stable set of behaviour.
Understood. Most uses will likely be either a distro or an embedded
system, who I'm assuming would provide a useful policy by default, and
perhaps a high-level abstraction for modification.
> Given that, would you prefer such a policy expression to look like?
Perhaps a write-once policy, injected from userspace during early boot?
The policy could be simply a list of:
lockdown_feature true|false
>
> > Within the policy check hook, we could add a new LSM hook, which would
> > allow an LSM to restrictively override the lockdown policy with its own
>
> Ok, that makes sense. If we take this approach, does there need to be
> a separate policy mechanism at all? Users who want fine-grained
> control would be able to set the behaviour to "None" and then use
> their choice of LSM to express more fine-grained control.
Right, and there could be a stackable LSM which just does fine-grained
policy (per above).
>
> > This doesn't really address the completeness / maintenance issue (i.e. "do
> > we have everything covered and how do we ensure this on an ongoing
> > basis?", and "what will this new lockdown feature break?"), although it
> > should make it easier to add new lockdown callsites as they don't have to
> > be enabled by the user.
>
> I can start on this.
Cool!
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: Matthew Garrett @ 2019-05-02 21:15 UTC (permalink / raw)
To: James Morris
Cc: LSM List, Linux Kernel Mailing List, David Howells, Linux API,
Andy Lutomirski
In-Reply-To: <alpine.LRH.2.21.1905030653480.32502@namei.org>
On Thu, May 2, 2019 at 2:07 PM James Morris <jmorris@namei.org> wrote:
> One possible direction is to (as previously mentioned) assign IDs to each
> callsite and be able to check this ID against a simple policy array
> (allow/deny). The default policy choices could be reduced to 'all' or
> 'none' during kconfig, and allow a custom policy to be loaded later if
> desired.
Ok. My primary concern around this is that it's very difficult to use
correctly in anything other than the "all" or "none" modes. If a new
kernel feature is added with integrated lockdown support, if an admin
is simply setting the flags of things they wish to block then this
will be left enabled - and may violate the admin's expectations around
integrity. On the other hand, if an admin is simply setting the flags
of things they wish to permit, then adding lockdown support to an
existing kernel feature may result in that feature suddenly being
disabled, which may also violate the admin's expectations around the
flags providing a stable set of behaviour.
Given that, would you prefer such a policy expression to look like?
> Within the policy check hook, we could add a new LSM hook, which would
> allow an LSM to restrictively override the lockdown policy with its own
Ok, that makes sense. If we take this approach, does there need to be
a separate policy mechanism at all? Users who want fine-grained
control would be able to set the behaviour to "None" and then use
their choice of LSM to express more fine-grained control.
> This doesn't really address the completeness / maintenance issue (i.e. "do
> we have everything covered and how do we ensure this on an ongoing
> basis?", and "what will this new lockdown feature break?"), although it
> should make it easier to add new lockdown callsites as they don't have to
> be enabled by the user.
I can start on this.
^ permalink raw reply
* Re: [RFC PATCH 2/7] x86/sci: add core implementation for system call isolation
From: Robert O'Callahan @ 2019-05-02 21:07 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, Mike Rapoport, LKML, Alexandre Chartre,
Borislav Petkov, Dave Hansen, H. Peter Anvin, Ingo Molnar,
James Bottomley, Jonathan Adams, Kees Cook, Paul Turner,
Peter Zijlstra, Thomas Gleixner, Linux-MM, LSM List, X86 ML,
Linus Torvalds, Peter Zijlstra, Andrew Morton
In-Reply-To: <20190502152016.GA51567@gmail.com>
On Fri, May 3, 2019 at 3:20 AM Ingo Molnar <mingo@kernel.org> wrote:
> So what might work better is if we defined a Rust dialect that used C
> syntax. I.e. the end result would be something like the 'c2rust' or
> 'citrus' projects, where code like this would be directly translatable to
> Rust:
>
> void gz_compress(FILE * in, gzFile out)
> {
> char buf[BUFLEN];
> int len;
> int err;
>
> for (;;) {
> len = fread(buf, 1, sizeof(buf), in);
> if (ferror(in)) {
> perror("fread");
> exit(1);
> }
> if (len == 0)
> break;
> if (gzwrite(out, buf, (unsigned)len) != len)
> error(gzerror(out, &err));
> }
> fclose(in);
>
> if (gzclose(out) != Z_OK)
> error("failed gzclose");
> }
>
>
> #[no_mangle]
> pub unsafe extern "C" fn gz_compress(mut in_: *mut FILE, mut out: gzFile) {
> let mut buf: [i8; 16384];
> let mut len;
> let mut err;
> loop {
> len = fread(buf, 1, std::mem::size_of_val(&buf), in_);
> if ferror(in_) != 0 { perror("fread"); exit(1); }
> if len == 0 { break ; }
> if gzwrite(out, buf, len as c_uint) != len {
> error(gzerror(out, &mut err));
> };
> }
> fclose(in_);
> if gzclose(out) != Z_OK { error("failed gzclose"); };
> }
>
> Example taken from:
>
> https://gitlab.com/citrus-rs/citrus
>
> Does this make sense?
Are you saying you want a tool like c2rust/citrus that translates some
new "looks like C, but really Rust" language into actual Rust at build
time? I guess that might work, but I suspect your "looks like C"
language isn't going to end up being much like C (e.g. it's going to
need Rust-style enums-with-fields, Rust polymorphism, Rust traits, and
Rust lifetimes), so it may not be beneficial, because you've just
created a new language no-one knows, and that has some real downsides.
If you're inspired by the dream of transitioning to safer languages,
then I think the first practical step would be to identify some part
of the kernel where the payoff of converting code would be highest.
This is probably something small, relatively isolated, that's not well
tested, generally suspicious, but still in use. Then do an experiment,
converting it to Rust (or something else) using off-the-shelf tools
and manual labor, and see where the pain points are and what benefits
accrue, if any. (Work like https://github.com/tsgates/rust.ko might be
a helpful starting point.) Then you'd have some data to start thinking
about how to reduce the costs, increase the benefits, and sell it to
the kernel community. If you reached out to the Rust community you
might find some volunteers to help with this.
Rob
--
Su ot deraeppa sah dna Rehtaf eht htiw saw hcihw, efil lanrete eht uoy
ot mialcorp ew dna, ti ot yfitset dna ti nees evah ew; deraeppa efil
eht. Efil fo Drow eht gninrecnoc mialcorp ew siht - dehcuot evah sdnah
ruo dna ta dekool evah ew hcihw, seye ruo htiw nees evah ew hcihw,
draeh evah ew hcihw, gninnigeb eht morf saw hcihw taht.
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: James Morris @ 2019-05-02 21:07 UTC (permalink / raw)
To: Matthew Garrett
Cc: LSM List, Linux Kernel Mailing List, David Howells, Linux API,
Andy Lutomirski
In-Reply-To: <CACdnJus-+VTy0uOWg982SgZr55Lp7Xot653dJb_tO5T=J6D8nw@mail.gmail.com>
On Mon, 29 Apr 2019, Matthew Garrett wrote:
> Hi James,
>
> What's the best way forward with this? I'm still not entirely clear on
> how it can be implemented purely as an LSM, but if you have ideas on
> what sort of implementation you'd prefer I'm happy to work on that.
It can't be implemented purely as an LSM.
The concerns I have are:
o Mixing of mechanism and policy (they are hardcoded together)
o Too-coarse policy (all or nothing, which will lead many to choose
nothing)
o Lack of integration with LSM
o Completeness
o Maintenance (including adding new lockdowns without breaking existing
userspace)
One possible direction is to (as previously mentioned) assign IDs to each
callsite and be able to check this ID against a simple policy array
(allow/deny). The default policy choices could be reduced to 'all' or
'none' during kconfig, and allow a custom policy to be loaded later if
desired.
Within the policy check hook, we could add a new LSM hook, which would
allow an LSM to restrictively override the lockdown policy with its own
(so e.g. SELinux could utilize the context of the current process to
determine if a lockdown feature should be enforced).
This doesn't really address the completeness / maintenance issue (i.e. "do
we have everything covered and how do we ensure this on an ongoing
basis?", and "what will this new lockdown feature break?"), although it
should make it easier to add new lockdown callsites as they don't have to
be enabled by the user.
Thoughts?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Ard Biesheuvel @ 2019-05-02 20:56 UTC (permalink / raw)
To: Matthew Garrett
Cc: Jarkko Sakkinen, Ingo Molnar, Bartosz Szczepanek, linux-integrity,
Peter Huewe, Jason Gunthorpe, Roberto Sassu, linux-efi, LSM List,
Linux Kernel Mailing List, Thiébaud Weksteen
In-Reply-To: <CACdnJuvDuw0X9iwEqOu7EjM5ca1f+n7f=xqzrTPS9PyrmqKNHQ@mail.gmail.com>
On Thu, 2 May 2019 at 20:04, Matthew Garrett <mjg59@google.com> wrote:
>
> On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > (+ Ingo)
> >
> > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett <mjg59@google.com> wrote:
> > >
> > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek <bsz@semihalf.com> wrote:
> > > >
> > > > I may be a little late with this comment, but I've just tested these
> > > > patches on aarch64 platform (from the top of jjs/master) and got
> > > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > > mail). I think there's problem with below call to
> > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > > passed as (void *) and never remapped:
> > >
> > > Yes, it looks like this is just broken. Can you try with the attached patch?
> >
> > I'm a bit uncomfortable with EFI code that is obviously broken and
> > untested being queued for the next merge window in another tree.
>
> The patchset was Cc:ed to linux-efi@. Is there anything else I should
> have done to ensure you picked it up rather than Jarkko?
No, I am not saying it was you who did anything wrong - Jarkko and I
should probably have aligned better. But my own testing wouldn't have
caught this particular issue either (I am still in the process of
getting access to ARM machines with a TPM), so it wouldn't have made a
huge difference in any case.
^ 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