* Re: [GIT PULL] keys: Namespacing
From: James Morris @ 2019-05-01 17:07 UTC (permalink / raw)
To: David Howells
Cc: dwalsh, vgoyal, keyrings, linux-security-module, linux-nfs,
linux-fsdevel, linux-kernel, netdev
In-Reply-To: <561.1556663960@warthog.procyon.org.uk>
On Tue, 30 Apr 2019, David Howells wrote:
> Hi James,
>
> Can you pull this set of patches into the security tree and pass them along
> to Linus in the next merge window? The primary thrust is to add
> namespacing to keyrings.
Not for this merge window, it's too close. Something like this would need
to be in -rc2 or so.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [GIT PULL] keys: Namespacing
From: David Howells @ 2019-05-01 17:18 UTC (permalink / raw)
To: James Morris
Cc: dhowells, dwalsh, vgoyal, keyrings, linux-security-module,
linux-nfs, linux-fsdevel, linux-kernel, netdev
In-Reply-To: <alpine.LRH.2.21.1905020306290.14696@namei.org>
James Morris <jmorris@namei.org> wrote:
> > Can you pull this set of patches into the security tree and pass them along
> > to Linus in the next merge window? The primary thrust is to add
> > namespacing to keyrings.
>
> Not for this merge window, it's too close. Something like this would need
> to be in -rc2 or so.
Okay.
David
^ permalink raw reply
* Re: [PATCH 1/2] apparmor: Use a memory pool instead per-CPU caches
From: John Johansen @ 2019-05-01 21:29 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-security-module, James Morris, Serge E. Hallyn, tglx
In-Reply-To: <20190430144725.gd6r3aketxuqdyir@linutronix.de>
On 4/30/19 7:47 AM, Sebastian Andrzej Siewior wrote:
> On 2019-04-28 16:56:59 [-0700], John Johansen wrote:
>> So digging into why the history of the per cpu buffers in apparmor.
>> We used to do buffer allocations via kmalloc and there were a few reasons
>> for the switch
>>
>> * speed/lockless: speaks for it self, mediation is already slow enough
>
> it is shared among all CPUs but it is a small/quick operation to
> add/return a buffer.
>
I wouldn't exactly call taking a lock speedy. Getting an available buffer
or returning it is indeed quick. The allocation fall back not so much.
>> * some buffer allocations had to be done with GFP_ATOMIC, making them
>> more likely to fail. Since we fail closed that means failure would
>> block access. This actually became a serious problem in a couple
>> places. Switching to per cpu buffers and blocking pre-empt was
>> the solution.
>
> GFP_KERNEL is allowed to use IO/SWAP and ATOMIC has emergency pools. The
> new approach won't return a NULL pointer, simply spin to either allocate
> new memory or get one which was just returned.
>
yeah, I am not really a fan of a potential infinite loop trying to allocate
memory. It may be worth retrying once or twice but potentially infinitely
spinning on failed allocation really isn't acceptable.
>> * in heavy use cases we would see a lot of buffers being allocated
>> and freed. Which resulted in locking slow downs and also buffer
>> allocation failures. So having the buffers preallocated allowed us
>> to bound this potential problem.
>>
>> This was all 6 years ago. Going to a mem pool certainly could help,
>> reduce the memory foot print, and would definitely help with
>> preempt/real time kernels.
>>
>> A big concern with this patchset is reverting back to GFP_KERNEL
>> for everything. We definitely were getting failures due to allocations
>> in atomic context. There have been lots of changes in the kernel over
>> the last six years so it possible these cases don't exist anymore. I
>> went through and built some kernels with this patchset and have run
>> through some testing without tripping that problem but I don't think
>> it has seen enough testing yet.
>
> Do you want apply #1 now and #2 later? I audited the ATOMIC->KERNEL
> changes manually and I didn't see any atomic context. It looked like the
> only reason for ATOMIC was the preempt_disable() due to the memory pool.
>
Indeed most if not all (I'd have to dig to be sure) the changes made in #2
were original done because of the move to the per cpu buffers and blocking
pre-emption.
The problem was with the allocation of the buffer needing to be GFP_ATOMIC
some times.
^ permalink raw reply
* [PATCH 1/2 v2] efi: add a function to convert the status value to string
From: Lee, Chun-Yi @ 2019-05-02 4:04 UTC (permalink / raw)
To: Ard Biesheuvel, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck
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.
- Changed the wording in subject and description.
- Moved the marco immediately after the status value definitions.
- Turned into a proper function instead of inline.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
include/linux/efi.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 54357a258b35..6f3f89a32eef 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -42,6 +42,34 @@
#define EFI_ABORTED (21 | (1UL << (BITS_PER_LONG-1)))
#define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_STATUS_STR(_status) \
+case EFI_##_status: \
+ return "EFI_" __stringify(_status);
+
+static __attribute__((unused)) char *
+efi_status_to_str(unsigned long status)
+{
+ switch (status) {
+ EFI_STATUS_STR(SUCCESS)
+ EFI_STATUS_STR(LOAD_ERROR)
+ EFI_STATUS_STR(INVALID_PARAMETER)
+ EFI_STATUS_STR(UNSUPPORTED)
+ EFI_STATUS_STR(BAD_BUFFER_SIZE)
+ EFI_STATUS_STR(BUFFER_TOO_SMALL)
+ EFI_STATUS_STR(NOT_READY)
+ EFI_STATUS_STR(DEVICE_ERROR)
+ EFI_STATUS_STR(WRITE_PROTECTED)
+ EFI_STATUS_STR(OUT_OF_RESOURCES)
+ EFI_STATUS_STR(NOT_FOUND)
+ EFI_STATUS_STR(ABORTED)
+ EFI_STATUS_STR(SECURITY_VIOLATION)
+ default:
+ pr_warn("Unknown efi status: 0x%lx", status);
+ }
+
+ return "Unknown efi status";
+}
+
typedef unsigned long efi_status_t;
typedef u8 efi_bool_t;
typedef u16 efi_char16_t; /* UNICODE character */
--
2.16.4
^ permalink raw reply related
* [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: Lee, Chun-Yi @ 2019-05-02 4:04 UTC (permalink / raw)
To: Ard Biesheuvel, James Morris, Serge E . Hallyn, David Howells,
Josh Boyer, Nayna Jain, Mimi Zohar
Cc: linux-efi, linux-security-module, linux-kernel, Lee, Chun-Yi
In-Reply-To: <20190502040441.30372-1-jlee@suse.com>
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));
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;
}
+ 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);
}
@@ -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
^ permalink raw reply related
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Bartosz Szczepanek @ 2019-05-02 6:45 UTC (permalink / raw)
To: Matthew Garrett
Cc: linux-integrity, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
Roberto Sassu, linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <CACdnJuvYAfFboej4e5jQ=iwhb-5Pi7BgSKEWGqJ0q=uarCoOfQ@mail.gmail.com>
Second patch tries to unmap "mapping" which is not declared. I'm on
top of jjs/master and your TPM_MEMREMAP patches are already there, so
the first patch applied cleanly. Using it, kernel still panicked on
boot:
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x420f5162]
[ 0.000000] Linux version 5.1.0-rc2+ (root@localhost.localdomain)
(gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC)) #78 SMP Wed May 1
01:05:38 EDT 2019
[ 0.000000] earlycon: pl11 at MMIO 0x0000000402020000 (options '115200n8')
[ 0.000000] printk: bootconsole [pl11] enabled
[ 0.000000] efi: Getting EFI parameters from FDT:
[ 0.000000] efi: EFI v2.60 by Cavium Inc.
TX2-FW-Release-7.2-build_08-0-g14f8c5bf8a Apr 15 2019 18:51:41
[ 0.000000] efi: TPMFinalLog=0xed5f0000 SMBIOS=0xfad90000 SMBIOS
3.0=0xed530000 ACPI 2.0=0xeda90000 ESRT=0xfafdb218
MEMATTR=0xf8489018 TPMEventLog=0xedaa9018 MEMRESERVE=0xedaa8018
[ 0.000000] Unhandled fault at 0xffff7dfffe77a018
[ 0.000000] Mem abort info:
[ 0.000000] ESR = 0x96000003
[ 0.000000] Exception class = DABT (current EL), IL = 32 bits
[ 0.000000] SET = 0, FnV = 0
[ 0.000000] EA = 0, S1PTW = 0
[ 0.000000] Data abort info:
[ 0.000000] ISV = 0, ISS = 0x00000003
[ 0.000000] CM = 0, WnR = 0
[ 0.000000] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
[ 0.000000] [ffff7dfffe77a018] pgd=0000000081b12003
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] kernel BUG at arch/arm64/mm/fault.c:189!
[ 0.000000] Internal error: Oops - BUG: 0 [#1] SMP
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.1.0-rc2+ #78
[ 0.000000] pstate: 60400089 (nZCv daIf +PAN -UAO)
[ 0.000000] pc : show_pte+0x1d0/0x1f0
[ 0.000000] lr : show_pte+0x88/0x1f0
[ 0.000000] sp : ffff000011533b30
[ 0.000000] x29: ffff000011533b30 x28: ffff0000115473c0
[ 0.000000] x27: ffff000011542500 x26: ffff7dfffe73a010
[ 0.000000] x25: 0000000000000018 x24: 0000000000000025
[ 0.000000] x23: 00000000000000fb x22: ffff0000117fc000
[ 0.000000] x21: ffff000010f32000 x20: ffffffffffffffff
[ 0.000000] x19: ffff7dfffe77a018 x18: ffffffffffffffff
[ 0.000000] x17: 0000000000000000 x16: 0000000000000000
[ 0.000000] x15: ffff00001153d708 x14: ffff00001172f420
[ 0.000000] x13: ffff00001172f069 x12: ffff000011568000
[ 0.000000] x11: ffff000011533800 x10: ffff000011533800
[ 0.000000] x9 : ffff00001153ef58 x8 : 303030303030303d
[ 0.000000] x7 : 646770205d383130 x6 : ffff00001172e7ff
[ 0.000000] x5 : 0000000000000000 x4 : 0000000000000000
[ 0.000000] x3 : 0000000000000000 x2 : 0000000000000000
[ 0.000000] x1 : 0000000081b12000 x0 : 0000000000000ff8
[ 0.000000] Process swapper (pid: 0, stack limit = 0x(____ptrval____))
[ 0.000000] Call trace:
[ 0.000000] show_pte+0x1d0/0x1f0
[ 0.000000] do_mem_abort+0xa8/0xb0
[ 0.000000] el1_da+0x20/0xc4
[ 0.000000] efi_tpm_eventlog_init+0xe8/0x268
[ 0.000000] efi_config_parse_tables+0x180/0x29c
[ 0.000000] uefi_init+0x1d0/0x22c
[ 0.000000] efi_init+0x90/0x180
[ 0.000000] setup_arch+0x1f4/0x5fc
[ 0.000000] start_kernel+0x90/0x51c
[ 0.000000] Code: 910d6000 94030b20 17ffffe6 d503201f (d4210000)
[ 0.000000] random: get_random_bytes called from
print_oops_end_marker+0x54/0x70 with crng_init=0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill
the idle task! ]---
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Ard Biesheuvel @ 2019-05-02 7:14 UTC (permalink / raw)
To: Matthew Garrett, Jarkko Sakkinen, Ingo Molnar
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: <CACdnJutpBPAX6TOGgs3Ng2v_cC5hAf-3pHThESvjQ9vbvQeVkA@mail.gmail.com>
(+ 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?
^ permalink raw reply
* Re: [PATCH v20 16/28] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2019-05-02 8:27 UTC (permalink / raw)
To: Jethro Beekman
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
dave.hansen@intel.com, sean.j.christopherson@intel.com,
nhorman@redhat.com, npmccallum@redhat.com, serge.ayoun@intel.com,
shay.katz-zamir@intel.com, haitao.huang@intel.com,
andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
kai.svahn@intel.com, bp@alien8.de, josh@joshtriplett.org,
luto@kernel.org, kai.huang@intel.com, rientjes@google.com,
James Morris, Serge E . Hallyn,
linux-security-module@vger.kernel.org
In-Reply-To: <4aade310-6400-d448-6d24-12f4ae7b21f2@fortanix.com>
On Wed, Apr 24, 2019 at 01:34:03AM +0000, Jethro Beekman wrote:
> On 2019-04-17 03:39, Jarkko Sakkinen wrote:
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index 7bf627ac4958..3b80acde8671 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -16,6 +16,8 @@
> > _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> > #define SGX_IOC_ENCLAVE_INIT \
> > _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> > + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
> Need to update Documentation/ioctl/ioctl-number.txt as well
Tha patch contains ioctl update. Can you be more specific?
/Jarkko
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Jarkko Sakkinen @ 2019-05-02 8:32 UTC (permalink / raw)
To: Bartosz Szczepanek
Cc: Matthew Garrett, linux-integrity, peterhuewe, jgg, roberto.sassu,
linux-efi, linux-security-module, linux-kernel, tweek,
Matthew Garrett
In-Reply-To: <CAJzaN5pUJoOCz5-ZDSnTb6dbVPuy0QwmFD0CeofAGK+bRQx0og@mail.gmail.com>
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?
/Jarkko
^ permalink raw reply
* Re: [PATCH 1/2 v2] efi: add a function to convert the status value to string
From: Ard Biesheuvel @ 2019-05-02 8:53 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: James Morris, Serge E . Hallyn, David Howells, Josh Boyer,
Nayna Jain, Mimi Zohar, linux-efi, linux-security-module,
Linux Kernel Mailing List, Lee, Chun-Yi, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck
In-Reply-To: <20190502040441.30372-1-jlee@suse.com>
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)
> - 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.
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> include/linux/efi.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 54357a258b35..6f3f89a32eef 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -42,6 +42,34 @@
> #define EFI_ABORTED (21 | (1UL << (BITS_PER_LONG-1)))
> #define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
>
> +#define EFI_STATUS_STR(_status) \
> +case EFI_##_status: \
> + return "EFI_" __stringify(_status);
> +
> +static __attribute__((unused)) char *
> +efi_status_to_str(unsigned long status)
> +{
> + switch (status) {
> + EFI_STATUS_STR(SUCCESS)
> + EFI_STATUS_STR(LOAD_ERROR)
> + EFI_STATUS_STR(INVALID_PARAMETER)
> + EFI_STATUS_STR(UNSUPPORTED)
> + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> + EFI_STATUS_STR(NOT_READY)
> + EFI_STATUS_STR(DEVICE_ERROR)
> + EFI_STATUS_STR(WRITE_PROTECTED)
> + EFI_STATUS_STR(OUT_OF_RESOURCES)
> + EFI_STATUS_STR(NOT_FOUND)
> + EFI_STATUS_STR(ABORTED)
> + EFI_STATUS_STR(SECURITY_VIOLATION)
> + default:
> + pr_warn("Unknown efi status: 0x%lx", status);
> + }
> +
> + return "Unknown efi status";
> +}
> +
> typedef unsigned long efi_status_t;
> typedef u8 efi_bool_t;
> typedef u16 efi_char16_t; /* UNICODE character */
> --
> 2.16.4
>
^ permalink raw reply
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: Ard Biesheuvel @ 2019-05-02 9:04 UTC (permalink / raw)
To: Lee, Chun-Yi, Mimi Zohar, David Howells
Cc: James Morris, Serge E . Hallyn, Josh Boyer, Nayna Jain, linux-efi,
linux-security-module, Linux Kernel Mailing List, Lee, Chun-Yi
In-Reply-To: <20190502040441.30372-2-jlee@suse.com>
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).
> 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;
> }
> + 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.
> @@ -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
- secure boot might be enabled but we may be booting without shim.
- secure boot might be disabled.
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.
^ permalink raw reply
* Re: [PATCH 1/2] apparmor: Use a memory pool instead per-CPU caches
From: Sebastian Andrzej Siewior @ 2019-05-02 10:51 UTC (permalink / raw)
To: John Johansen; +Cc: linux-security-module, James Morris, Serge E. Hallyn, tglx
In-Reply-To: <02d7772b-5d06-1c32-b089-454547fbe08b@canonical.com>
On 2019-05-01 14:29:17 [-0700], John Johansen wrote:
> On 4/30/19 7:47 AM, Sebastian Andrzej Siewior wrote:
> > On 2019-04-28 16:56:59 [-0700], John Johansen wrote:
> >> So digging into why the history of the per cpu buffers in apparmor.
> >> We used to do buffer allocations via kmalloc and there were a few reasons
> >> for the switch
> >>
> >> * speed/lockless: speaks for it self, mediation is already slow enough
> >
> > it is shared among all CPUs but it is a small/quick operation to
> > add/return a buffer.
> >
> I wouldn't exactly call taking a lock speedy. Getting an available buffer
> or returning it is indeed quick. The allocation fall back not so much.
Based on testing it happens only in the beginning. We could also start
with 2,3,4 pre allocated buffers or so.
My testing was most likely limited and I did not exceed two.
> >> * some buffer allocations had to be done with GFP_ATOMIC, making them
> >> more likely to fail. Since we fail closed that means failure would
> >> block access. This actually became a serious problem in a couple
> >> places. Switching to per cpu buffers and blocking pre-empt was
> >> the solution.
> >
> > GFP_KERNEL is allowed to use IO/SWAP and ATOMIC has emergency pools. The
> > new approach won't return a NULL pointer, simply spin to either allocate
> > new memory or get one which was just returned.
> >
>
> yeah, I am not really a fan of a potential infinite loop trying to allocate
> memory. It may be worth retrying once or twice but potentially infinitely
> spinning on failed allocation really isn't acceptable.
It shouldn't spin infinitely because even if kmalloc() does not return
any memory, one of the other CPUs should return their buffer at some
point. However, if you don't like it I could add two retries and return
NULL + fixup callers. On the other hand if the other CPUs BUG() with the
buffers then yes, we may spin.
So limited retries it is?
> >> * in heavy use cases we would see a lot of buffers being allocated
> >> and freed. Which resulted in locking slow downs and also buffer
> >> allocation failures. So having the buffers preallocated allowed us
> >> to bound this potential problem.
> >>
> >> This was all 6 years ago. Going to a mem pool certainly could help,
> >> reduce the memory foot print, and would definitely help with
> >> preempt/real time kernels.
> >>
> >> A big concern with this patchset is reverting back to GFP_KERNEL
> >> for everything. We definitely were getting failures due to allocations
> >> in atomic context. There have been lots of changes in the kernel over
> >> the last six years so it possible these cases don't exist anymore. I
> >> went through and built some kernels with this patchset and have run
> >> through some testing without tripping that problem but I don't think
> >> it has seen enough testing yet.
> >
> > Do you want apply #1 now and #2 later? I audited the ATOMIC->KERNEL
> > changes manually and I didn't see any atomic context. It looked like the
> > only reason for ATOMIC was the preempt_disable() due to the memory pool.
> >
>
> Indeed most if not all (I'd have to dig to be sure) the changes made in #2
> were original done because of the move to the per cpu buffers and blocking
> pre-emption.
>
> The problem was with the allocation of the buffer needing to be GFP_ATOMIC
> some times.
yup, that is what I saw, too.
Sebastian
^ permalink raw reply
* Re: [RFC PATCH 2/7] x86/sci: add core implementation for system call isolation
From: Robert O'Callahan @ 2019-05-02 11:35 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: <20190427104615.GA55518@gmail.com>
On Sat, Apr 27, 2019 at 10:46 PM Ingo Molnar <mingo@kernel.org> wrote:
> - A C language runtime that is a subset of current C syntax and
> semantics used in the kernel, and which doesn't allow access outside
> of existing objects and thus creates a strictly enforced separation
> between memory used for data, and memory used for code and control
> flow.
>
> - This would involve, at minimum:
>
> - tracking every type and object and its inherent length and valid
> access patterns, and never losing track of its type.
>
> - being a lot more organized about initialization, i.e. no
> uninitialized variables/fields.
>
> - being a lot more strict about type conversions and pointers in
> general.
>
> - ... and a metric ton of other details.
Several research groups have tried to do this, and it is very
difficult to do. In particular this was almost exactly the goal of
C-Cured [1]. Much more recently, there's Microsoft's CheckedC [2] [3],
which is less ambitious. Check the references of the latter for lots
of relevant work. If anyone really pursues this they should talk
directly to researchers who've worked on this, e.g. George Necula; you
need to know what *didn't* work well, which is hard to glean from
papers. (Academic publishing is broken that way.)
One problem with adopting "safe C" or Rust in the kernel is that most
of your security mitigations (e.g. KASLR, CFI, other randomizations)
probably need to remain in place as long as there is a significant
amount of C in the kernel, which means the benefits from eliminating
them will be realized very far in the future, if ever, which makes the
whole exercise harder to justify.
Having said that, I think there's a good case to be made for writing
kernel code in Rust, e.g. sketchy drivers. The classes of bugs
prevented in Rust are significantly broader than your usual safe-C
dialect (e.g. data races).
[1] https://web.eecs.umich.edu/~weimerw/p/p477-necula.pdf
[2] https://www.microsoft.com/en-us/research/uploads/prod/2019/05/checkedc-post2019.pdf
[3] https://github.com/Microsoft/checkedc
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 1/2] apparmor: Use a memory pool instead per-CPU caches
From: Tetsuo Handa @ 2019-05-02 13:17 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, John Johansen
Cc: linux-security-module, James Morris, Serge E. Hallyn, tglx
In-Reply-To: <20190502105158.2hluemukrdz5hbus@linutronix.de>
On 2019/05/02 19:51, Sebastian Andrzej Siewior wrote:
>>>> * some buffer allocations had to be done with GFP_ATOMIC, making them
>>>> more likely to fail. Since we fail closed that means failure would
>>>> block access. This actually became a serious problem in a couple
>>>> places. Switching to per cpu buffers and blocking pre-empt was
>>>> the solution.
>>>
>>> GFP_KERNEL is allowed to use IO/SWAP and ATOMIC has emergency pools. The
>>> new approach won't return a NULL pointer, simply spin to either allocate
>>> new memory or get one which was just returned.
>>>
>>
>> yeah, I am not really a fan of a potential infinite loop trying to allocate
>> memory. It may be worth retrying once or twice but potentially infinitely
>> spinning on failed allocation really isn't acceptable.
>
> It shouldn't spin infinitely because even if kmalloc() does not return
> any memory, one of the other CPUs should return their buffer at some
> point. However, if you don't like it I could add two retries and return
> NULL + fixup callers. On the other hand if the other CPUs BUG() with the
> buffers then yes, we may spin.
> So limited retries it is?
There is 'The "too small to fail" memory-allocation rule'
( https://lwn.net/Articles/627419/ ) where GFP_KERNEL allocation for
size <= 32768 bytes never fails unless current thread was killed by
the OOM killer. This means that kmalloc() in
+char *aa_get_buffer(void)
+{
+ union aa_buffer *aa_buf;
+
+try_again:
+ 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);
+ if (WARN_ON_ONCE(!aa_buf))
+ goto try_again;
+ return &aa_buf->buffer[0];
+}
can't return NULL unless current thread was killed by the OOM killer
if aa_g_path_max <= 32768. On the other hand, if aa_g_path_max > 32768,
this allocation can easily fail, and retrying forever is very bad.
If current thread was killed by the OOM killer, current thread should be
able to bail out without retrying. If allocation can never succeed (e.g.
aa_g_path_max == 1073741824 was specified), we must bail out.
By the way, did you really test your patch?
> @@ -1399,6 +1404,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 = min_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
I think that this will guarantee that aa_g_path_max <= sizeof(struct list_head)
which is too small to succeed. :-(
> pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
>
> return error;
^ permalink raw reply
* Re: [PATCH 1/2] apparmor: Use a memory pool instead per-CPU caches
From: Sebastian Andrzej Siewior @ 2019-05-02 13:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <7b41609f-2592-93c1-55f7-6026ff6dba26@I-love.SAKURA.ne.jp>
On 2019-05-02 22:17:35 [+0900], Tetsuo Handa wrote:
> There is 'The "too small to fail" memory-allocation rule'
> ( https://lwn.net/Articles/627419/ ) where GFP_KERNEL allocation for
> size <= 32768 bytes never fails unless current thread was killed by
> the OOM killer. This means that kmalloc() in
>
> +char *aa_get_buffer(void)
> +{
> + union aa_buffer *aa_buf;
> +
> +try_again:
> + 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);
> + if (WARN_ON_ONCE(!aa_buf))
> + goto try_again;
> + return &aa_buf->buffer[0];
> +}
>
> can't return NULL unless current thread was killed by the OOM killer
> if aa_g_path_max <= 32768. On the other hand, if aa_g_path_max > 32768,
> this allocation can easily fail, and retrying forever is very bad.
as I pointed out in the other email, it shouldn't retry forever because
we should have something in the pool which will be returned.
> If current thread was killed by the OOM killer, current thread should be
> able to bail out without retrying. If allocation can never succeed (e.g.
> aa_g_path_max == 1073741824 was specified), we must bail out.
okay. That is obviously too much and we would loop, indeed.
> By the way, did you really test your patch?
I booted Debian on a 112 core which has apparmor enabled and debian
ships a few profiles. And then I used the box for a while.
> > @@ -1399,6 +1404,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 = min_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
>
> I think that this will guarantee that aa_g_path_max <= sizeof(struct list_head)
> which is too small to succeed. :-(
Ach right, this should have been max instead of min. Btw: are there any
sane upper/lower limits while at it?
> > pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
> >
> > return error;
Sebastian
^ permalink raw reply
* Re: [PATCH 1/2] apparmor: Use a memory pool instead per-CPU caches
From: Tetsuo Handa @ 2019-05-02 14:10 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: John Johansen, linux-security-module, James Morris,
Serge E. Hallyn, tglx
In-Reply-To: <20190502134730.d3ya46ave6a7bvom@linutronix.de>
On 2019/05/02 22:47, Sebastian Andrzej Siewior wrote:
> On 2019-05-02 22:17:35 [+0900], Tetsuo Handa wrote:
>> There is 'The "too small to fail" memory-allocation rule'
>> ( https://lwn.net/Articles/627419/ ) where GFP_KERNEL allocation for
>> size <= 32768 bytes never fails unless current thread was killed by
>> the OOM killer. This means that kmalloc() in
>>
>> +char *aa_get_buffer(void)
>> +{
>> + union aa_buffer *aa_buf;
>> +
>> +try_again:
>> + 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);
>> + if (WARN_ON_ONCE(!aa_buf))
>> + goto try_again;
>> + return &aa_buf->buffer[0];
>> +}
>>
>> can't return NULL unless current thread was killed by the OOM killer
>> if aa_g_path_max <= 32768. On the other hand, if aa_g_path_max > 32768,
>> this allocation can easily fail, and retrying forever is very bad.
>
> as I pointed out in the other email, it shouldn't retry forever because
> we should have something in the pool which will be returned.
>
The point of 'The "too small to fail" memory-allocation rule' is that kmalloc(GFP_KERNEL)
can't return NULL after current thread once reached kmalloc(GFP_KERNEL). That is, current
thread loops forever inside __alloc_pages_nodemask() unless killed by the OOM killer.
If you want to use aa_get_buffer() as if a memory pool, you need to specify __GFP_NORETRY
(or __GFP_RETRY_MAYFAIL).
>>> @@ -1399,6 +1404,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 = min_t(uint32_t, aa_g_path_max, sizeof(union aa_buffer));
>>
>> I think that this will guarantee that aa_g_path_max <= sizeof(struct list_head)
>> which is too small to succeed. :-(
>
> Ach right, this should have been max instead of min. Btw: are there any
> sane upper/lower limits while at it?
Although PATH_MAX is a limit which can be passed as a string argument to syscalls,
there is no limit for a pathname calculated by d_path() etc. The pathname can grow
until memory allocation for dentry cache fails after OOM-killer killed almost all
userspace processes.
But for pathname based access control, returning an error to userspace due to memory
allocation failure for d_path() etc. might result in unexpected termination of that
process. Therefore, you don't want to give up easily.
^ permalink raw reply
* Re: [RFC PATCH 2/7] x86/sci: add core implementation for system call isolation
From: Ingo Molnar @ 2019-05-02 15:20 UTC (permalink / raw)
To: Robert O'Callahan
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: <CAOp6jLa1Rs2xrhJ2wpWoFbJGHyB99OX9doQZc+dNqOSUMgURsw@mail.gmail.com>
* Robert O'Callahan <robert@ocallahan.org> wrote:
> On Sat, Apr 27, 2019 at 10:46 PM Ingo Molnar <mingo@kernel.org> wrote:
> > - A C language runtime that is a subset of current C syntax and
> > semantics used in the kernel, and which doesn't allow access outside
> > of existing objects and thus creates a strictly enforced separation
> > between memory used for data, and memory used for code and control
> > flow.
> >
> > - This would involve, at minimum:
> >
> > - tracking every type and object and its inherent length and valid
> > access patterns, and never losing track of its type.
> >
> > - being a lot more organized about initialization, i.e. no
> > uninitialized variables/fields.
> >
> > - being a lot more strict about type conversions and pointers in
> > general.
> >
> > - ... and a metric ton of other details.
>
> Several research groups have tried to do this, and it is very
> difficult to do. In particular this was almost exactly the goal of
> C-Cured [1]. Much more recently, there's Microsoft's CheckedC [2] [3],
> which is less ambitious. Check the references of the latter for lots
> of relevant work. If anyone really pursues this they should talk
> directly to researchers who've worked on this, e.g. George Necula; you
> need to know what *didn't* work well, which is hard to glean from
> papers. (Academic publishing is broken that way.)
>
> One problem with adopting "safe C" or Rust in the kernel is that most
> of your security mitigations (e.g. KASLR, CFI, other randomizations)
> probably need to remain in place as long as there is a significant
> amount of C in the kernel, which means the benefits from eliminating
> them will be realized very far in the future, if ever, which makes the
> whole exercise harder to justify.
>
> Having said that, I think there's a good case to be made for writing
> kernel code in Rust, e.g. sketchy drivers. The classes of bugs
> prevented in Rust are significantly broader than your usual safe-C
> dialect (e.g. data races).
>
> [1] https://web.eecs.umich.edu/~weimerw/p/p477-necula.pdf
> [2] https://www.microsoft.com/en-us/research/uploads/prod/2019/05/checkedc-post2019.pdf
> [3] https://github.com/Microsoft/checkedc
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?
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH] kexec_buffer measure
From: Mimi Zohar @ 2019-05-02 15:48 UTC (permalink / raw)
To: prakhar srivastava
Cc: linux-integrity, linux-kernel, linux-security-module, Paul Moore,
Casey Schaufler, John Johansen
In-Reply-To: <1555978681.4914.305.camel@linux.ibm.com>
[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?
Mimi
^ permalink raw reply
* Re: [PATCH] kexec_buffer measure
From: Casey Schaufler @ 2019-05-02 16:26 UTC (permalink / raw)
To: Mimi Zohar, prakhar srivastava
Cc: linux-integrity, linux-kernel, linux-security-module, Paul Moore,
John Johansen, casey
In-Reply-To: <1556812101.4134.28.camel@linux.ibm.com>
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 have an interest in it for any work I have in progress.
>
> Mimi
>
^ permalink raw reply
* Re: [PATCH] kexec_buffer measure
From: Casey Schaufler @ 2019-05-02 16:28 UTC (permalink / raw)
To: Mimi Zohar, prakhar srivastava
Cc: linux-integrity, linux-kernel, linux-security-module, Paul Moore,
John Johansen, casey
In-Reply-To: <1556812101.4134.28.camel@linux.ibm.com>
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.
> Mimi
>
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett @ 2019-05-02 18:03 UTC (permalink / raw)
To: Jarkko Sakkinen
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: <20190502083240.GJ14532@linux.intel.com>
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.
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett @ 2019-05-02 18:04 UTC (permalink / raw)
To: Ard Biesheuvel
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: <CAKv+Gu9PF4u=-7QL4e36Q3S5kC4+5Z=yLYHLT9jE+eNY7YUV7A@mail.gmail.com>
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?
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett @ 2019-05-02 18:07 UTC (permalink / raw)
To: Bartosz Szczepanek
Cc: linux-integrity, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
Roberto Sassu, linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <CAJzaN5ofshg4KseGhOL2LSLDQNoAHC6Ve25gpgWU69bEfBq1fw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 212 bytes --]
Sorry, how about this one? I was confused by why I wasn't hitting
this, but on closer examination it turns out that my system populates
the final event log with 0 events which means we never hit this
codepath :(
[-- Attachment #2: fix_log.diff --]
[-- Type: text/x-patch, Size: 4136 bytes --]
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 2ccaa6661aaf..db0fdaa9c666 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index dccc97e6135c..190a33968a91 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- early_memunmap(marker_start, mapping_size);
+ early_memunmap(event, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = early_memremap((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = early_memremap((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -257,8 +256,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
+ event_field->event_size;
size = marker - marker_start;
- if ((event->event_type == 0) && (event_field->event_size == 0))
- size = 0;
out:
if (do_mapping)
early_memunmap(mapping, mapping_size);
^ permalink raw reply related
* Re: [PATCH 1/2] apparmor: Use a memory pool instead per-CPU caches
From: John Johansen @ 2019-05-02 19:33 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-security-module, James Morris, Serge E. Hallyn, tglx
In-Reply-To: <20190502105158.2hluemukrdz5hbus@linutronix.de>
On 5/2/19 3:51 AM, Sebastian Andrzej Siewior wrote:
> On 2019-05-01 14:29:17 [-0700], John Johansen wrote:
>> On 4/30/19 7:47 AM, Sebastian Andrzej Siewior wrote:
>>> On 2019-04-28 16:56:59 [-0700], John Johansen wrote:
>>>> So digging into why the history of the per cpu buffers in apparmor.
>>>> We used to do buffer allocations via kmalloc and there were a few reasons
>>>> for the switch
>>>>
>>>> * speed/lockless: speaks for it self, mediation is already slow enough
>>>
>>> it is shared among all CPUs but it is a small/quick operation to
>>> add/return a buffer.
>>>
>> I wouldn't exactly call taking a lock speedy. Getting an available buffer
>> or returning it is indeed quick. The allocation fall back not so much.
>
> Based on testing it happens only in the beginning. We could also start
> with 2,3,4 pre allocated buffers or so.
> My testing was most likely limited and I did not exceed two.
>
yeah lets have a few preallocated
>>>> * some buffer allocations had to be done with GFP_ATOMIC, making them
>>>> more likely to fail. Since we fail closed that means failure would
>>>> block access. This actually became a serious problem in a couple
>>>> places. Switching to per cpu buffers and blocking pre-empt was
>>>> the solution.
>>>
>>> GFP_KERNEL is allowed to use IO/SWAP and ATOMIC has emergency pools. The
>>> new approach won't return a NULL pointer, simply spin to either allocate
>>> new memory or get one which was just returned.
>>>
>>
>> yeah, I am not really a fan of a potential infinite loop trying to allocate
>> memory. It may be worth retrying once or twice but potentially infinitely
>> spinning on failed allocation really isn't acceptable.
>
> It shouldn't spin infinitely because even if kmalloc() does not return
> any memory, one of the other CPUs should return their buffer at some
> point. However, if you don't like it I could add two retries and return
> NULL + fixup callers. On the other hand if the other CPUs BUG() with the
> buffers then yes, we may spin.
> So limited retries it is?
>
yes please
^ 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