Re: [PATCH 08/30] kexec_file: Restrict at runtime if the kernel is locked down
On Thu, Feb 22, 2018 at 02:21:53PM +, David Howells wrote: > commit ed0424c531d7dd25adebdec0ee6a78a5784f207a > Author: David Howells > Date: Thu Feb 22 14:01:49 2018 + > > kexec_file: Restrict at runtime if the kernel is locked down > > When KEXEC_VERIFY_SIG is not enabled, kernel should not load images > through s/KEXEC_VERIFY_SIG/KEXEC_SIG/ Again, my mistake :/ Other than that, looks OK. Much cleaner than my version. Thanks! Reviewed-by: Jiri Bohac -- Jiri Bohac SUSE Labs, Prague, Czechia -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/30] kexec_file: Restrict at runtime if the kernel is locked down
On Thu, Feb 22, 2018 at 02:20:43PM +, David Howells wrote: > commit 87a39b258eca2e15884ee90c3fcd5758d6057b17 > Author: David Howells > Date: Thu Feb 22 13:42:04 2018 + > > kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE > > This is a preparatory patch for kexec_file_load() lockdown. A locked down > kernel needs to prevent unsigned kernel images to be loaded with s/to be loaded/from being loaded/ (my own mistake :-)) Otherwise looks good. Thanks for improving my idea. Reviewed-by: Jiri Bohac -- Jiri Bohac SUSE Labs, Prague, Czechia -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/30] Enforce module signatures if the kernel is locked down
On Thu, Feb 22, 2018 at 01:07:57PM +, David Howells wrote: > I'm considering folding the attached changes into this patch. > > It adjusts the errors generated: > > (1) If there's no signature (ENODATA) or we can't check it (ENOPKG, ENOKEY), > then: > > (a) If signatures are enforced then EKEYREJECTED is returned. > > (b) If IMA will have validated the image, return 0 (okay). > > (c) If there's no signature or we can't check it, but the kernel is >locked down then EPERM is returned (this is then consistent with >other lockdown cases). > > (2) If the signature is unparseable (EBADMSG, EINVAL), the signature fails > the check (EKEYREJECTED) or a system error occurs (eg. ENOMEM), we return > the error we got. > > Note that the X.509 code doesn't check for key expiry as the RTC might not be > valid or might not have been transferred to the kernel's clock yet. Looks good. Reviewed-by: Jiri Bohac -- Jiri Bohac SUSE Labs, Prague, Czechia -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] efivarfs: Limit the rate for non-root to read files
On 22 February 2018 at 18:07, Linus Torvalds wrote: > On Thu, Feb 22, 2018 at 9:54 AM, Luck, Tony wrote: >> With the new "while/nap" change there would still be one message >> per second, but the number of callbacks suppressed should be 1 >> (unless the user has many threads doing reads). >> >> Maybe it is good to know that an application is doing something >> stupid and we should drop that line from the patch and let the >> warnings flow? > > I think the "one message per second" is fine. > > Looks good. Do I get this through the EFI tree, or should I just take > it directly? > Please take it directly if everybody is happy with it. Acked-by: Ard Biesheuvel -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] efivarfs: Limit the rate for non-root to read files
On Thu, Feb 22, 2018 at 9:54 AM, Luck, Tony wrote: > With the new "while/nap" change there would still be one message > per second, but the number of callbacks suppressed should be 1 > (unless the user has many threads doing reads). > > Maybe it is good to know that an application is doing something > stupid and we should drop that line from the patch and let the > warnings flow? I think the "one message per second" is fine. Looks good. Do I get this through the EFI tree, or should I just take it directly? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] efivarfs: Limit the rate for non-root to read files
On Thu, Feb 22, 2018 at 09:39:10AM -0800, Linus Torvalds wrote: > I'm certainly ok with this. I'm assuming this has been tested I read some files using "dd bs=1" as root and non-root. Root still goes fast, non-root is limited. Both see the same data. I can ^C the non-root version and the dd quits as expected: $ dd if=DefSetup-e8a99903-302c-4851-a6be-ab2731873b2f of=/dev/null bs=1 ^C301+0 records in 300+0 records out 300 bytes copied, 3.10487 s, 0.1 kB/s > and gives nice warnings too? They seemed very spammy before so I turned them off with this: + ratelimit_set_flags(&new->ratelimit, RATELIMIT_MSG_ON_RELEASE); They looked like this: [ 176.607182] efivarfs_file_read: 42 callbacks suppressed [ 177.611064] efivarfs_file_read: 42 callbacks suppressed [ 178.614931] efivarfs_file_read: 41 callbacks suppressed [ 179.622986] efivarfs_file_read: 42 callbacks suppressed [ 180.630920] efivarfs_file_read: 42 callbacks suppressed [ 181.634839] efivarfs_file_read: 42 callbacks suppressed [ 182.646729] efivarfs_file_read: 42 callbacks suppressed [ 183.658679] efivarfs_file_read: 42 callbacks suppressed [ 184.678664] efivarfs_file_read: 43 callbacks suppressed [ 185.698571] efivarfs_file_read: 43 callbacks suppressed [ 186.703129] efivarfs_file_read: 42 callbacks suppressed [ 187.718510] efivarfs_file_read: 43 callbacks suppressed With the new "while/nap" change there would still be one message per second, but the number of callbacks suppressed should be 1 (unless the user has many threads doing reads). Maybe it is good to know that an application is doing something stupid and we should drop that line from the patch and let the warnings flow? -Tony -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] efivarfs: Limit the rate for non-root to read files
On Thu, Feb 22, 2018 at 9:15 AM, Luck, Tony wrote: > > In efivarfs if the limit is exceeded when reading, we take an > interruptible nap for 50ms and check the rate limit again. Ok, turning that 'if' into a 'while' makes the sleeping work even in the presence of lots of threads, and that all looks very simple. I'm certainly ok with this. I'm assuming this has been tested and gives nice warnings too? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] efivarfs: Limit the rate for non-root to read files
Each read from a file in efivarfs results in two calls to EFI
(one to get the file size, another to get the actual data).
On X86 these EFI calls result in broadcast system management
interrupts (SMI) which affect performance of the whole system.
A malicious user can loop performing reads from efivarfs bringing
the system to its knees.
Linus suggested per-user rate limit to solve this.
So we add a ratelimit structure to "user_struct" and initialize
it for the root user for no limit. When allocating user_struct for
other users we set the limit to 100 per second. This could be used
for other places that want to limit the rate of some detrimental
user action.
In efivarfs if the limit is exceeded when reading, we take an
interruptible nap for 50ms and check the rate limit again.
Signed-off-by: Tony Luck
---
Does this look better? User processes above the rate limit will
sleep and check the limit again.
fs/efivarfs/file.c | 6 ++
include/linux/sched/user.h | 4
kernel/user.c | 3 +++
3 files changed, 13 insertions(+)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 5f22e74bbade..8e568428c88b 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -8,6 +8,7 @@
*/
#include
+#include
#include
#include
#include
@@ -74,6 +75,11 @@ static ssize_t efivarfs_file_read(struct file *file, char
__user *userbuf,
ssize_t size = 0;
int err;
+ while (!__ratelimit(&file->f_cred->user->ratelimit)) {
+ if (!msleep_interruptible(50))
+ return -EINTR;
+ }
+
err = efivar_entry_size(var, &datasize);
/*
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index 0dcf4e480ef7..96fe289c4c6e 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -4,6 +4,7 @@
#include
#include
+#include
struct key;
@@ -41,6 +42,9 @@ struct user_struct {
defined(CONFIG_NET)
atomic_long_t locked_vm;
#endif
+
+ /* Miscellaneous per-user rate limit */
+ struct ratelimit_state ratelimit;
};
extern int uids_sysfs_init(void);
diff --git a/kernel/user.c b/kernel/user.c
index 9a20acce460d..36288d840675 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -101,6 +101,7 @@ struct user_struct root_user = {
.sigpending = ATOMIC_INIT(0),
.locked_shm = 0,
.uid= GLOBAL_ROOT_UID,
+ .ratelimit = RATELIMIT_STATE_INIT(root_user.ratelimit, 0, 0),
};
/*
@@ -191,6 +192,8 @@ struct user_struct *alloc_uid(kuid_t uid)
new->uid = uid;
atomic_set(&new->__count, 1);
+ ratelimit_state_init(&new->ratelimit, HZ, 100);
+ ratelimit_set_flags(&new->ratelimit, RATELIMIT_MSG_ON_RELEASE);
/*
* Before adding this, check whether we raced
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efivarfs: Limit the rate for non-root to read files
"Luck, Tony" writes: >> - add a per-user mutex, and do the usleep inside of it, so that >> anybody who tries to do a thousand threads will just be serialized by >> the mutex. >> >> Note that the mutex needs to be per-user, because otherwise it will be >> a DoS for the other users. > > I can try that tomorrow (adding the per-user mutex to struct user_struct > right next to the ratelimit I added. Another possibility is to cache the files in the page cache. That will reduce re-reads of the same data to the maximum extent. If efi has a chance of changing variables behind our back we might want something that would have a timeout on how long the data is cached, and we probably want to make the caching policy write-trough not write-back. I just suggest this as it seems like a much more tried and true solution. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/30] kexec_file: Restrict at runtime if the kernel is locked down
David Howells wrote:
> I'm intending on inserting the attached patch before this one.
And replacing this patch with the attached.
David
---
commit ed0424c531d7dd25adebdec0ee6a78a5784f207a
Author: David Howells
Date: Thu Feb 22 14:01:49 2018 +
kexec_file: Restrict at runtime if the kernel is locked down
When KEXEC_VERIFY_SIG is not enabled, kernel should not load images through
kexec_file systemcall if the kernel is locked down unless IMA can be used
to validate the image.
[Modified by David Howells to fit with modifications to the previous patch
and to return -EPERM if the kernel is locked down for consistency with
other lockdowns]
Signed-off-by: Jiri Bohac
Signed-off-by: David Howells
Cc: Matthew Garrett
cc: Chun-Yi Lee
cc: ke...@lists.infradead.org
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index d5931e392050..c47c4de604cd 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -167,6 +167,14 @@ kimage_file_prepare_segments(struct kimage *image, int
kernel_fd, int initrd_fd,
}
ret = 0;
+ if (is_ima_appraise_enabled())
+ break;
+
+ if (kernel_is_locked_down(reason)) {
+ ret = -EPERM;
+ goto out;
+ }
+
break;
/* All other errors are fatal, including nomem, unparseable
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/30] kexec_file: Restrict at runtime if the kernel is locked down
I'm intending on inserting the attached patch before this one.
David
---
commit 87a39b258eca2e15884ee90c3fcd5758d6057b17
Author: David Howells
Date: Thu Feb 22 13:42:04 2018 +
kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE
This is a preparatory patch for kexec_file_load() lockdown. A locked down
kernel needs to prevent unsigned kernel images to be loaded with
kexec_file_load(). Currently, the only way to force the signature
verification is compiling with KEXEC_VERIFY_SIG. This prevents loading
usigned images even when the kernel is not locked down at runtime.
This patch splits KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE.
Analogous to the MODULE_SIG and MODULE_SIG_FORCE for modules, KEXEC_SIG
turns on the signature verification but allows unsigned images to be
loaded. KEXEC_SIG_FORCE disallows images without a valid signature.
[Modified by David Howells such that:
(1) verify_pefile_signature() differentiates between no-signature and
sig-didn't-match in its returned errors.
(2) kexec fails with EKEYREJECTED and logs an appropriate message if
signature checking is enforced and an signature is not found, uses
unsupported crypto or has no matching key.
(3) kexec fails with EKEYREJECTED if there is a signature for which we
have a key, but signature doesn't match - even if in non-forcing mode.
(4) kexec fails with EBADMSG or some other error if there is a signature
which cannot be parsed - even if in non-forcing mode.
(5) kexec fails with ELIBBAD if the PE file cannot be parsed to extract
the signature - even if in non-forcing mode.
]
Signed-off-by: Jiri Bohac
Signed-off-by: David Howells
cc: Matthew Garrett
cc: Chun-Yi Lee
cc: ke...@lists.infradead.org
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1236b187824..cb6e67b7442d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2019,20 +2019,30 @@ config KEXEC_FILE
for kernel and initramfs as opposed to list of segments as
accepted by previous system call.
-config KEXEC_VERIFY_SIG
+config KEXEC_SIG
bool "Verify kernel signature during kexec_file_load() syscall"
depends on KEXEC_FILE
---help---
- This option makes kernel signature verification mandatory for
- the kexec_file_load() syscall.
- In addition to that option, you need to enable signature
+ This option makes the kexec_file_load() syscall check for a valid
+ signature of the kernel image. The image can still be loaded without
+ a valid signature unless you also enable KEXEC_SIG_FORCE, though if
+ there's a signature that we can check, then it must be valid.
+
+ In addition to this option, you need to enable signature
verification for the corresponding kernel image type being
loaded in order for this to work.
+config KEXEC_SIG_FORCE
+ bool "Require a valid signature in kexec_file_load() syscall"
+ depends on KEXEC_SIG
+ ---help---
+ This option makes kernel signature verification mandatory for
+ the kexec_file_load() syscall.
+
config KEXEC_BZIMAGE_VERIFY_SIG
bool "Enable bzImage signature verification support"
- depends on KEXEC_VERIFY_SIG
+ depends on KEXEC_SIG
depends on SIGNED_PE_FILE_VERIFICATION
select SYSTEM_TRUSTED_KEYRING
---help---
diff --git a/arch/x86/kernel/machine_kexec_64.c
b/arch/x86/kernel/machine_kexec_64.c
index 1f790cf9d38f..3fbe35b923ef 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -406,7 +406,7 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
return image->fops->cleanup(image->image_loader_data);
}
-#ifdef CONFIG_KEXEC_VERIFY_SIG
+#ifdef CONFIG_KEXEC_SIG
int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel,
unsigned long kernel_len)
{
diff --git a/crypto/asymmetric_keys/verify_pefile.c
b/crypto/asymmetric_keys/verify_pefile.c
index d178650fd524..4473cea1e877 100644
--- a/crypto/asymmetric_keys/verify_pefile.c
+++ b/crypto/asymmetric_keys/verify_pefile.c
@@ -100,7 +100,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned
int pelen,
if (!ddir->certs.virtual_address || !ddir->certs.size) {
pr_debug("Unsigned PE binary\n");
- return -EKEYREJECTED;
+ return -ENODATA;
}
chkaddr(ctx->header_size, ddir->certs.virtual_address,
@@ -408,6 +408,8 @@ static int pefile_digest_pe(const void *pebuf, unsigned int
pelen,
* (*) 0 if at least one signature chain intersects with the keys in the trust
* keyring, or:
*
+ * (*) -ENODATA if there is no signature present.
+ *
* (*) -ENOPKG if a suitable crypto module couldn't be
Re: [PATCH 04/30] Enforce module signatures if the kernel is locked down
I'm considering folding the attached changes into this patch.
It adjusts the errors generated:
(1) If there's no signature (ENODATA) or we can't check it (ENOPKG, ENOKEY),
then:
(a) If signatures are enforced then EKEYREJECTED is returned.
(b) If IMA will have validated the image, return 0 (okay).
(c) If there's no signature or we can't check it, but the kernel is
locked down then EPERM is returned (this is then consistent with
other lockdown cases).
(2) If the signature is unparseable (EBADMSG, EINVAL), the signature fails
the check (EKEYREJECTED) or a system error occurs (eg. ENOMEM), we return
the error we got.
Note that the X.509 code doesn't check for key expiry as the RTC might not be
valid or might not have been transferred to the kernel's clock yet.
David
---
diff --git a/kernel/module.c b/kernel/module.c
index 1eb06a0ccbfb..62419cf48ef6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2769,8 +2769,9 @@ static inline void kmemleak_load_module(const struct
module *mod,
static int module_sig_check(struct load_info *info, int flags,
bool can_do_ima_check)
{
- int err = -ENOKEY;
+ int err = -ENODATA;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+ const char *reason;
const void *mod = info->hdr;
/*
@@ -2785,18 +2786,42 @@ static int module_sig_check(struct load_info *info, int
flags,
err = mod_verify_sig(mod, &info->len);
}
- if (!err) {
+ switch (err) {
+ case 0:
info->sig_ok = true;
return 0;
- }
- /* Not having a signature is only an error if we're strict. */
- if (err == -ENOKEY && !sig_enforce &&
- (!can_do_ima_check || !is_ima_appraise_enabled()) &&
- !kernel_is_locked_down("Loading of unsigned modules"))
- err = 0;
+ /* We don't permit modules to be loaded into trusted kernels
+* without a valid signature on them, but if we're not
+* enforcing, certain errors are non-fatal.
+*/
+ case -ENODATA:
+ reason = "Loading of unsigned module";
+ goto decide;
+ case -ENOPKG:
+ reason = "Loading of module with unsupported crypto";
+ goto decide;
+ case -ENOKEY:
+ reason = "Loading of module with unavailable key";
+ decide:
+ if (sig_enforce) {
+ pr_notice("%s is rejected\n", reason);
+ return -EKEYREJECTED;
+ }
- return err;
+ if (can_do_ima_check && is_ima_appraise_enabled())
+ return 0;
+ if (kernel_is_locked_down(reason))
+ return -EPERM;
+ return 0;
+
+ /* All other errors are fatal, including nomem, unparseable
+* signatures and signature check failures - even if signatures
+* aren't required.
+*/
+ default:
+ return err;
+ }
}
#else /* !CONFIG_MODULE_SIG */
static int module_sig_check(struct load_info *info, int flags,
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
