* [PATCH v2 07/11] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
The args_p member is a simple string that is allocated by
ima_rule_init(). Shallow copy it like other non-LSM references in
ima_rule_entry structs.
There are no longer any necessary error path cleanups to do in
ima_lsm_copy_rule().
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- Adjusted context to account for ima_lsm_copy_rule() directly calling
ima_lsm_free_rule() and the lack of explicit reference ownership
transfers
- Added comment to ima_lsm_copy_rule() to document the args_p
reference ownership transfer
security/integrity/ima/ima_policy.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f9b1bdb897da..ef69c54266c6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
- nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
- GFP_KERNEL);
- if (!nentry->lsm[i].args_p)
- goto out_err;
+ nentry->lsm[i].args_p = entry->lsm[i].args_p;
+ /*
+ * Remove the reference from entry so that the associated
+ * memory will not be freed during a later call to
+ * ima_lsm_free_rule(entry).
+ */
+ entry->lsm[i].args_p = NULL;
security_filter_rule_init(nentry->lsm[i].type,
Audit_equal,
@@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
(char *)entry->lsm[i].args_p);
}
return nentry;
-
-out_err:
- ima_lsm_free_rule(nentry);
- kfree(nentry);
- return NULL;
}
static int ima_lsm_update_rule(struct ima_rule_entry *entry)
--
2.25.1
^ permalink raw reply related
* [PATCH v2 10/11] ima: Use the common function to detect LSM conditionals in a rule
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Make broader use of ima_rule_contains_lsm_cond() to check if a given
rule contains an LSM conditional. This is a code cleanup and has no
user-facing change.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
* v2
- No change
security/integrity/ima/ima_policy.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 43d49ad958fb..5eb14b567a31 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -360,17 +360,10 @@ static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
static void ima_lsm_update_rules(void)
{
struct ima_rule_entry *entry, *e;
- int i, result, needs_update;
+ int result;
list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
- needs_update = 0;
- for (i = 0; i < MAX_LSM_RULES; i++) {
- if (entry->lsm[i].args_p) {
- needs_update = 1;
- break;
- }
- }
- if (!needs_update)
+ if (!ima_rule_contains_lsm_cond(entry))
continue;
result = ima_lsm_update_rule(entry);
--
2.25.1
^ permalink raw reply related
* [PATCH v2 06/11] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
The KEY_CHECK function only supports the uid, pcr, and keyrings
conditionals. Make this clear at policy load so that IMA policy authors
don't assume that other conditionals are supported.
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- No change
security/integrity/ima/ima_policy.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 676d5557af1a..f9b1bdb897da 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1018,6 +1018,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
+ if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+ IMA_KEYRINGS))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
break;
default:
return false;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 04/11] ima: Fail rule parsing when buffer hook functions have an invalid action
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Buffer based hook functions, such as KEXEC_CMDLINE and KEY_CHECK, can
only measure. The process_buffer_measurement() function quietly ignores
all actions except measure so make this behavior clear at the time of
policy load.
The parsing of the keyrings conditional had a check to ensure that it
was only specified with measure actions but the check should be on the
hook function and not the keyrings conditional since
"appraise func=KEY_CHECK" is not a valid rule.
Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- No change
security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e458cd47c099..166124d67774 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -973,6 +973,39 @@ static void check_template_modsig(const struct ima_template_desc *template)
#undef MSG
}
+static bool ima_validate_rule(struct ima_rule_entry *entry)
+{
+ if (entry->action == UNKNOWN)
+ return false;
+
+ if (entry->flags & IMA_FUNC) {
+ switch (entry->func) {
+ case NONE:
+ case FILE_CHECK:
+ case MMAP_CHECK:
+ case BPRM_CHECK:
+ case CREDS_CHECK:
+ case POST_SETATTR:
+ case MODULE_CHECK:
+ case FIRMWARE_CHECK:
+ case KEXEC_KERNEL_CHECK:
+ case KEXEC_INITRAMFS_CHECK:
+ case POLICY_CHECK:
+ break;
+ case KEXEC_CMDLINE:
+ case KEY_CHECK:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ break;
+ default:
+ return false;
+ }
+ }
+
+ return true;
+}
+
static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
@@ -1150,7 +1183,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
keyrings_len = strlen(args[0].from) + 1;
if ((entry->keyrings) ||
- (entry->action != MEASURE) ||
(entry->func != KEY_CHECK) ||
(keyrings_len < 2)) {
result = -EINVAL;
@@ -1356,7 +1388,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
}
}
- if (!result && (entry->action == UNKNOWN))
+ if (!result && !ima_validate_rule(entry))
result = -EINVAL;
else if (entry->action == APPRAISE)
temp_ima_appraise |= ima_appraise_flag(entry->func);
--
2.25.1
^ permalink raw reply related
* [PATCH v2 05/11] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
The KEXEC_CMDLINE hook function only supports the pcr conditional. Make
this clear at policy load so that IMA policy authors don't assume that
other conditionals are supported.
Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned
true on any loaded KEXEC_CMDLINE rule without any consideration for
other conditionals present in the rule. Make it clear that pcr is the
only supported KEXEC_CMDLINE conditional by returning an error during
policy load.
An example of why this is a problem can be explained with the following
rule:
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
An IMA policy author would have assumed that rule is valid because the
parser accepted it but the result was that measurements for all
KEXEC_CMDLINE operations would be disabled.
Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
* v2
- Added Mimi's Reviewed-by
security/integrity/ima/ima_policy.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 166124d67774..676d5557af1a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -343,6 +343,17 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
return 0;
}
+static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
+{
+ int i;
+
+ for (i = 0; i < MAX_LSM_RULES; i++)
+ if (entry->lsm[i].args_p)
+ return true;
+
+ return false;
+}
+
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
@@ -993,6 +1004,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
case POLICY_CHECK:
break;
case KEXEC_CMDLINE:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ if (entry->flags & ~(IMA_FUNC | IMA_PCR))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
+ break;
case KEY_CHECK:
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 03/11] ima: Free the entire rule if it fails to parse
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
members, such as .fsname and .keyrings, when an error is encountered
during rule parsing.
Set the args_p pointer to NULL after freeing it in the error path of
ima_lsm_rule_init() so that it isn't freed twice.
This fixes a memory leak seen when loading an rule that contains an
additional piece of allocated memory, such as an fsname, followed by an
invalid conditional:
# echo "measure fsname=tmpfs bad=cond" > /sys/kernel/security/ima/policy
-bash: echo: write error: Invalid argument
# echo scan > /sys/kernel/debug/kmemleak
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff98e7e4ece6c0 (size 8):
comm "bash", pid 672, jiffies 4294791843 (age 21.855s)
hex dump (first 8 bytes):
74 6d 70 66 73 00 6b a5 tmpfs.k.
backtrace:
[<00000000abab7413>] kstrdup+0x2e/0x60
[<00000000f11ede32>] ima_parse_add_rule+0x7d4/0x1020
[<00000000f883dd7a>] ima_write_policy+0xab/0x1d0
[<00000000b17cf753>] vfs_write+0xde/0x1d0
[<00000000b8ddfdea>] ksys_write+0x68/0xe0
[<00000000b8e21e87>] do_syscall_64+0x56/0xa0
[<0000000089ea7b98>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- No change
security/integrity/ima/ima_policy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index bf00b966e87f..e458cd47c099 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -913,6 +913,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
if (ima_rules == &ima_default_rules) {
kfree(entry->lsm[lsm_rule].args_p);
+ entry->lsm[lsm_rule].args_p = NULL;
result = -EINVAL;
} else
result = 0;
@@ -1404,7 +1405,7 @@ ssize_t ima_parse_add_rule(char *rule)
result = ima_parse_rule(p, entry);
if (result) {
- kfree(entry);
+ ima_free_rule(entry);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "invalid-policy", result,
audit_info);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 05/14] umh: Separate the user mode driver and the user mode helper support
From: Tetsuo Handa @ 2020-06-27 1:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87d05lbwt4.fsf@x220.int.ebiederm.org>
On 2020/06/27 1:45, Eric W. Biederman wrote:
>> After this cleanup, I expect adding some protections/isolation which kernel threads
>> have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
>> SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
>> SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's
>>
>> It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
>> That works and very convenient when it comes to debugging. Compare that to debugging
>> a kernel module!
>>
>> but I think doing it is essential for keeping usermode blob processes as secure/robust
>> as kernel threads.
>
> Do you have an application for a user mode driver?
No, I'm not a user of this interface.
>
> I think concerns like that are best addressed in the context of a
> specific driver/usecase. Just to make certain we are solving the right
> problems.
>
> My sense is that an advantage of user mode drivers can safely be buggier
> than kernel drivers and the freedom to kill them when the drivers go
> wrong (knowing the drivers will restart) is important.
Right. Segmentation fault in user mode drivers does not cause a kernel oops
is an advantage of user mode drivers. But the freedom to disturb/kill user mode
drivers due to interference like ptrace()/signals from user mode processes,
SIGKILL from OOM-killer/SysRq-i etc. is a big disadvantage of user mode drivers.
I expect that user mode drivers should be killable only when the manager
interface detected that user mode drivers need to be stopped (or restarted).
One of advantages kernel mode drivers have is that their memory is not swapped
out/in. I don't know whether mlockall(MCL_FUTURE) should be automatically applied
to user mode drivers.
>
> Does this series by using the normal path through exec solve your
> concerns with LSMs being able to identify these processes (both
> individually and as class)?.
I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
at do_open_execat() from do_execveat_common().
I guess "no" for inode based LSMs, for they want a chance to associate
security labels at blob_to_mnt().
^ permalink raw reply
* Re: [PATCH 05/14] umh: Separate the user mode driver and the user mode helper support
From: Eric W. Biederman @ 2020-06-27 4:21 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <32604829-35f7-5263-aff1-27808500c1d1@i-love.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/06/27 1:45, Eric W. Biederman wrote:
>> Does this series by using the normal path through exec solve your
>> concerns with LSMs being able to identify these processes (both
>> individually and as class)?.
>
> I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
> AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
> at do_open_execat() from do_execveat_common().
Is that a problem with the current do_execveat_common in general?
That does not sound like a problem in the user mode driver case as
there are no symlinks involved.
Eric
^ permalink raw reply
* Re: [PATCH 05/14] umh: Separate the user mode driver and the user mode helper support
From: Tetsuo Handa @ 2020-06-27 4:36 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87k0zt87fd.fsf@x220.int.ebiederm.org>
On 2020/06/27 13:21, Eric W. Biederman wrote:
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
>> On 2020/06/27 1:45, Eric W. Biederman wrote:
>>> Does this series by using the normal path through exec solve your
>>> concerns with LSMs being able to identify these processes (both
>>> individually and as class)?.
>>
>> I guess "yes" for pathname based LSMs. Though, TOMOYO wants to obtain both
>> AT_SYMLINK_NOFOLLOW "struct path" and !AT_SYMLINK_NOFOLLOW "struct path"
>> at do_open_execat() from do_execveat_common().
>
> Is that a problem with the current do_execveat_common in general?
In general. Since LSM does not receive parameters needed for obtaining
AT_SYMLINK_NOFOLLOW "struct path" (and it is racy even if parameters were
passed to LSM), I want to obtain both paths in one place.
>
> That does not sound like a problem in the user mode driver case as
> there are no symlinks involved.
Right.
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Tetsuo Handa @ 2020-06-27 11:38 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87pn9mgfc2.fsf_-_@x220.int.ebiederm.org>
On 2020/06/26 21:51, Eric W. Biederman wrote:
> Please let me know if you see any bugs. Once the code review is
> finished I plan to take this through my tree.
This series needs some sanity checks.
diff --git a/kernel/umd.c b/kernel/umd.c
index de2f542191e5..f3e0227a3012 100644
--- a/kernel/umd.c
+++ b/kernel/umd.c
@@ -47,15 +47,18 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
/**
* umd_load_blob - Remember a blob of bytes for fork_usermode_driver
- * @info: information about usermode driver
- * @data: a blob of bytes that can be executed as a file
- * @len: The lentgh of the blob
+ * @info: information about usermode driver (shouldn't be NULL)
+ * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
+ * @len: The lentgh of the blob (shouldn't be 0)
*
*/
int umd_load_blob(struct umd_info *info, const void *data, size_t len)
{
struct vfsmount *mnt;
+ if (!info || !info->driver_name || !data || !len)
+ return -EINVAL;
+
if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
return -EBUSY;
@@ -158,6 +161,9 @@ int fork_usermode_driver(struct umd_info *info)
char **argv = NULL;
int err;
+ if (!info || !info->driver_name)
+ return -EINVAL;
+
if (WARN_ON_ONCE(info->tgid))
return -EBUSY;
But loading
----- test.c -----
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/umd.h>
static int __init test_init(void)
{
const char blob[464] = {
"\x7f\x45\x4c\x46\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x02\x00\x3e\x00\x01\x00\x00\x00\x80\x00\x40\x00\x00\x00\x00\x00"
"\x40\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x40\x00\x38\x00\x01\x00\x40\x00\x04\x00\x03\x00"
"\x01\x00\x00\x00\x05\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x40\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00\x00\x00"
"\xb4\x00\x00\x00\x00\x00\x00\x00\xb4\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x20\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\xb8\x01\x00\x00\x00\xbf\x01\x00\x00\x00\x48\xbe\xa8\x00\x40\x00"
"\x00\x00\x00\x00\xba\x0c\x00\x00\x00\x0f\x05\xb8\xe7\x00\x00\x00"
"\xbf\x00\x00\x00\x00\x0f\x05\x00\x48\x65\x6c\x6c\x6f\x20\x77\x6f"
"\x72\x6c\x64\x0a\x00\x2e\x73\x68\x73\x74\x72\x74\x61\x62\x00\x2e"
"\x74\x65\x78\x74\x00\x2e\x72\x6f\x64\x61\x74\x61\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x0b\x00\x00\x00\x01\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00\x00"
"\x80\x00\x40\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00"
"\x27\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x10\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x11\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00"
"\xa8\x00\x40\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00"
"\x0c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x01\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\xb4\x00\x00\x00\x00\x00\x00\x00"
"\x19\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
};
struct umd_info *info = kzalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;
info->driver_name = kstrdup("my test driver", GFP_KERNEL);
printk("umd_load_blob()=%d\n", umd_load_blob(info, blob, 464));
//printk("fork_usermode_driver()=%d\n", fork_usermode_driver(info));
return -EINVAL;
}
module_init(test_init);
MODULE_LICENSE("GPL");
----- test.c -----
causes
BUG_ON(!(task->flags & PF_KTHREAD));
in __fput_sync(). Do we want to forbid umd_load_blob() from process context (e.g.
upon module initialization time) ?
Also, since umd_load_blob() uses info->driver_name as filename, info->driver_name has to
satisfy strchr(info->driver_name, '/') == NULL && strlen(info->driver_name) <= NAME_MAX
in order to avoid -ENOENT failure. On the other hand, since fork_usermode_driver() uses
info->driver_name as argv[], info->driver_name has to use ' ' within this constraint.
This might be inconvenient...
^ permalink raw reply related
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-06-27 12:59 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <40720db5-92f0-4b5b-3d8a-beb78464a57f@i-love.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/06/26 21:51, Eric W. Biederman wrote:
>> Please let me know if you see any bugs. Once the code review is
>> finished I plan to take this through my tree.
>
[sniped example code]
> causes
>
> BUG_ON(!(task->flags & PF_KTHREAD));
>
> in __fput_sync(). Do we want to forbid umd_load_blob() from process context (e.g.
> upon module initialization time) ?
Interesting. I had not realized that fput_sync would not work from
module context.
Forcing the fput to finish is absolutely necessary. Otherwise the file
will still be open for write and deny_write_access in execve will fail.
Can you try replacing the __fput_sync with:
fput(file);
flush_delayed_fput();
task_work_run();
Given that there is a big requirement for the code to run before init
I don't necessarily think it is a problem __fput_sync is a problem.
But it also seems silly to forbid modules if we can easily fix
the code.
Eric
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Tetsuo Handa @ 2020-06-27 13:57 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87366g8y1e.fsf@x220.int.ebiederm.org>
On 2020/06/27 21:59, Eric W. Biederman wrote:
> Can you try replacing the __fput_sync with:
> fput(file);
> flush_delayed_fput();
> task_work_run();
With below change, TOMOYO can obtain pathname like "tmpfs:/my\040test\040driver".
Please avoid WARN_ON() if printk() is sufficient (for friendliness to panic_on_warn=1 environments).
For argv[], I guess that fork_usermode_driver() should receive argv[] as argument rather than
trying to split info->driver_name, for somebody might want to pass meaningful argv[] (and
TOMOYO wants to use meaningful argv[] as a hint for identifying the intent).
diff --git a/kernel/umd.c b/kernel/umd.c
index de2f542191e5..ae6e85283f13 100644
--- a/kernel/umd.c
+++ b/kernel/umd.c
@@ -7,6 +7,7 @@
#include <linux/mount.h>
#include <linux/fs_struct.h>
#include <linux/umd.h>
+#include <linux/task_work.h>
static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *name)
{
@@ -25,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
if (IS_ERR(mnt))
return mnt;
- file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
+ file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
if (IS_ERR(file)) {
mntput(mnt);
return ERR_CAST(file);
@@ -41,23 +42,33 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
return ERR_PTR(err);
}
- __fput_sync(file);
+ if (current->flags & PF_KTHREAD) {
+ __fput_sync(file);
+ } else {
+ fput(file);
+ flush_delayed_fput();
+ task_work_run();
+ }
return mnt;
}
/**
* umd_load_blob - Remember a blob of bytes for fork_usermode_driver
- * @info: information about usermode driver
- * @data: a blob of bytes that can be executed as a file
- * @len: The lentgh of the blob
+ * @info: information about usermode driver (shouldn't be NULL)
+ * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
+ * @len: The lentgh of the blob (shouldn't be 0)
*
*/
int umd_load_blob(struct umd_info *info, const void *data, size_t len)
{
struct vfsmount *mnt;
- if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
+ if (!info || !info->driver_name || !data || !len)
+ return -EINVAL;
+ if (info->wd.dentry || info->wd.mnt) {
+ pr_info("%s already loaded.\n", info->driver_name);
return -EBUSY;
+ }
mnt = blob_to_mnt(data, len, info->driver_name);
if (IS_ERR(mnt))
@@ -71,14 +82,14 @@ EXPORT_SYMBOL_GPL(umd_load_blob);
/**
* umd_unload_blob - Disassociate @info from a previously loaded blob
- * @info: information about usermode driver
+ * @info: information about usermode driver (shouldn't be NULL)
*
*/
int umd_unload_blob(struct umd_info *info)
{
- if (WARN_ON_ONCE(!info->wd.mnt ||
- !info->wd.dentry ||
- info->wd.mnt->mnt_root != info->wd.dentry))
+ if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
+ return -EINVAL;
+ if (WARN_ON_ONCE(info->wd.mnt->mnt_root != info->wd.dentry))
return -EINVAL;
kern_unmount(info->wd.mnt);
@@ -158,8 +169,14 @@ int fork_usermode_driver(struct umd_info *info)
char **argv = NULL;
int err;
- if (WARN_ON_ONCE(info->tgid))
+ if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
+ return -EINVAL;
+ if (WARN_ON_ONCE(info->wd.mnt->mnt_root != info->wd.dentry))
+ return -EINVAL;
+ if (info->tgid) {
+ pr_info("%s already running.\n", info->driver_name);
return -EBUSY;
+ }
err = -ENOMEM;
argv = argv_split(GFP_KERNEL, info->driver_name, NULL);
^ permalink raw reply related
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-27 17:43 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, linux-kernel, linux-security-module,
Jethro Beekman, Haitao Huang, Chunyang Hui, Jordan Hand,
Nathaniel McCallum, Seth Moore, Sean Christopherson,
Suresh Siddha, akpm, andriy.shevchenko, asapek, cedric.xing,
chenalexchen, conradparker, cyhanish, dave.hansen, haitao.huang,
josh, kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman,
puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200617220844.57423-12-jarkko.sakkinen@linux.intel.com>
On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
> + void *token)
> +{
> + u64 mrsigner[4];
> + int ret;
> + int i;
> + int j;
> +
> + /* Check that the required attributes have been authorized. */
> + if (encl->secs_attributes & ~encl->allowed_attributes)
> + return -EACCES;
> +
> + ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&encl->lock);
> +
> + if (atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) {
> + ret = -EFAULT;
> + goto err_out;
> + }
That test should be the first thing this function or its caller does.
> + for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> + for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
Ew, what's that double-loop for?
It tries to init an enclave a bunch of times. Why does it need to init
more than once?
> + ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> + mrsigner);
> + if (ret == SGX_UNMASKED_EVENT)
> + continue;
> + else
> + break;
> + }
> +
> + if (ret != SGX_UNMASKED_EVENT)
> + break;
> +
> + msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> +
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + goto err_out;
> + }
> + }
> +
> + if (ret & ENCLS_FAULT_FLAG) {
> + if (encls_failed(ret))
> + ENCLS_WARN(ret, "EINIT");
> +
> + sgx_encl_destroy(encl);
> + ret = -EFAULT;
> + } else if (ret) {
> + pr_debug("EINIT returned %d\n", ret);
> + ret = -EPERM;
> + } else {
> + atomic_or(SGX_ENCL_INITIALIZED, &encl->flags);
> + }
> +
> +err_out:
> + mutex_unlock(&encl->lock);
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> + *
> + * @filep: open file to /dev/sgx
@encl: pointer to an enclave instance (via ioctl() file pointer)
> + * @arg: userspace pointer to a struct sgx_enclave_init instance
> + *
> + * Flush any outstanding enqueued EADD operations and perform EINIT. The
> + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> + *
> + * Return:
> + * 0 on success,
> + * SGX error code on EINIT failure,
> + * -errno otherwise
> + */
> +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> +{
> + struct sgx_sigstruct *sigstruct;
> + struct sgx_enclave_init einit;
> + struct page *initp_page;
> + void *token;
> + int ret;
> +
> + if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
Might just as well check the other flags: doing EINIT on an already
initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
similarly on a SGX_ENCL_DEAD enclave.
And you could do similar sanity checks in the other ioctl functions.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2 06/11] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
From: Lakshmi Ramasubramanian @ 2020-06-27 23:39 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <20200626223900.253615-7-tyhicks@linux.microsoft.com>
On 6/26/20 3:38 PM, Tyler Hicks wrote:
> The KEY_CHECK function only supports the uid, pcr, and keyrings
> conditionals. Make this clear at policy load so that IMA policy authors
> don't assume that other conditionals are supported.
>
> Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>
> * v2
> - No change
>
> security/integrity/ima/ima_policy.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 676d5557af1a..f9b1bdb897da 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1018,6 +1018,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> if (entry->action & ~(MEASURE | DONT_MEASURE))
> return false;
>
> + if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> + IMA_KEYRINGS))
> + return false;
> +
> + if (ima_rule_contains_lsm_cond(entry))
> + return false;
> +
> break;
> default:
> return false;
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
^ permalink raw reply
* Re: [PATCH v2 05/11] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond
From: Lakshmi Ramasubramanian @ 2020-06-27 23:40 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <20200626223900.253615-6-tyhicks@linux.microsoft.com>
On 6/26/20 3:38 PM, Tyler Hicks wrote:
> The KEXEC_CMDLINE hook function only supports the pcr conditional. Make
> this clear at policy load so that IMA policy authors don't assume that
> other conditionals are supported.
>
> Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned
> true on any loaded KEXEC_CMDLINE rule without any consideration for
> other conditionals present in the rule. Make it clear that pcr is the
> only supported KEXEC_CMDLINE conditional by returning an error during
> policy load.
>
> An example of why this is a problem can be explained with the following
> rule:
>
> dont_measure func=KEXEC_CMDLINE obj_type=foo_t
>
> An IMA policy author would have assumed that rule is valid because the
> parser accepted it but the result was that measurements for all
> KEXEC_CMDLINE operations would be disabled.
>
> Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>
> * v2
> - Added Mimi's Reviewed-by
>
> security/integrity/ima/ima_policy.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 166124d67774..676d5557af1a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -343,6 +343,17 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> return 0;
> }
>
> +static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_LSM_RULES; i++)
> + if (entry->lsm[i].args_p)
> + return true;
> +
> + return false;
> +}
> +
> /*
> * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
> * to the old, stale LSM policy. Update the IMA LSM based rules to reflect
> @@ -993,6 +1004,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> case POLICY_CHECK:
> break;
> case KEXEC_CMDLINE:
> + if (entry->action & ~(MEASURE | DONT_MEASURE))
> + return false;
> +
> + if (entry->flags & ~(IMA_FUNC | IMA_PCR))
> + return false;
> +
> + if (ima_rule_contains_lsm_cond(entry))
> + return false;
> +
> + break;
> case KEY_CHECK:
> if (entry->action & ~(MEASURE | DONT_MEASURE))
> return false;
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
^ permalink raw reply
* Re: [PATCH v2 09/11] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Lakshmi Ramasubramanian @ 2020-06-27 23:49 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <20200626223900.253615-10-tyhicks@linux.microsoft.com>
On 6/26/20 3:38 PM, Tyler Hicks wrote:
> Use ima_validate_rule() to ensure that the combination of a hook
> function and the keyrings conditional is valid and that the keyrings
> conditional is not specified without an explicit KEY_CHECK func
> conditional. This is a code cleanup and has no user-facing change.
In addition to checking for func=KEY_CHECK and the keyrings conditional,
the patch also validates the flags for other IMA hooks (such as
KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in
the patch description.
-lakshmi
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>
> * v2
> - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> present in the rule entry flags for non-buffer hook functions.
>
> security/integrity/ima/ima_policy.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8cdca2399d59..43d49ad958fb 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> case KEXEC_KERNEL_CHECK:
> case KEXEC_INITRAMFS_CHECK:
> case POLICY_CHECK:
> + if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> + IMA_UID | IMA_FOWNER | IMA_FSUUID |
> + IMA_INMASK | IMA_EUID | IMA_PCR |
> + IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> + IMA_PERMIT_DIRECTIO |
> + IMA_MODSIG_ALLOWED |
> + IMA_CHECK_BLACKLIST))
> + return false;
> +
> break;
> case KEXEC_CMDLINE:
> if (entry->action & ~(MEASURE | DONT_MEASURE))
> @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> default:
> return false;
> }
> - }
> + } else if (entry->flags & IMA_KEYRINGS)
> + return false;
>
> return true;
> }
> @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> keyrings_len = strlen(args[0].from) + 1;
>
> if ((entry->keyrings) ||
> - (entry->func != KEY_CHECK) ||
> (keyrings_len < 2)) {
> result = -EINVAL;
> break;
>
^ permalink raw reply
* Re: [PATCH v2 11/11] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Lakshmi Ramasubramanian @ 2020-06-28 0:03 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Eric Biederman, kexec
In-Reply-To: <20200626223900.253615-12-tyhicks@linux.microsoft.com>
On 6/26/20 3:39 PM, Tyler Hicks wrote:
> Take the properties of the kexec kernel's inode and the current task
> ownership into consideration when matching a KEXEC_CMDLINE operation to
> the rules in the IMA policy. This allows for some uniformity when
> writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
> and KEXEC_CMDLINE operations.
>
> Prior to this patch, it was not possible to write a set of rules like
> this:
>
> dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> measure func=KEXEC_KERNEL_CHECK
> measure func=KEXEC_INITRAMFS_CHECK
> measure func=KEXEC_CMDLINE
>
> The inode information associated with the kernel being loaded by a
> kexec_kernel_load(2) syscall can now be included in the decision to
> measure or not
>
> Additonally, the uid, euid, and subj_* conditionals can also now be
> used in KEXEC_CMDLINE rules. There was no technical reason as to why
> those conditionals weren't being considered previously other than
> ima_match_rules() didn't have a valid inode to use so it immediately
> bailed out for KEXEC_CMDLINE operations rather than going through the
> full list of conditional comparisons.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org
> ---
>
> * v2
> - Moved the inode parameter of process_buffer_measurement() to be the
> first parameter so that it more closely matches process_masurement()
>
> include/linux/ima.h | 4 ++--
> kernel/kexec_file.c | 2 +-
> security/integrity/ima/ima.h | 2 +-
> security/integrity/ima/ima_api.c | 2 +-
> security/integrity/ima/ima_appraise.c | 2 +-
> security/integrity/ima/ima_asymmetric_keys.c | 2 +-
> security/integrity/ima/ima_main.c | 23 +++++++++++++++-----
> security/integrity/ima/ima_policy.c | 17 +++++----------
> security/integrity/ima/ima_queue_keys.c | 2 +-
> 9 files changed, 31 insertions(+), 25 deletions(-)
>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
^ permalink raw reply
* Re: possible deadlock in process_measurement (2)
From: syzbot @ 2020-06-28 3:31 UTC (permalink / raw)
To: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <000000000000880dcc0598bcfac9@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 1590a2e1 Merge tag 'acpi-5.8-rc3' of git://git.kernel.org/..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=164b959b100000
kernel config: https://syzkaller.appspot.com/x/.config?x=20c907630cbdbe5
dashboard link: https://syzkaller.appspot.com/bug?extid=18a1619cceea30ed45af
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=117f43c5100000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17cbb239100000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+18a1619cceea30ed45af@syzkaller.appspotmail.com
======================================================
WARNING: possible circular locking dependency detected
5.8.0-rc2-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor165/6816 is trying to acquire lock:
ffff888092f48080 (&iint->mutex){+.+.}-{3:3}, at: process_measurement+0x66d/0x18e0 security/integrity/ima/ima_main.c:247
but task is already holding lock:
ffff888214040450 (sb_writers#4){.+.+}-{0:0}, at: sb_start_write include/linux/fs.h:1664 [inline]
ffff888214040450 (sb_writers#4){.+.+}-{0:0}, at: mnt_want_write+0x45/0x90 fs/namespace.c:354
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (sb_writers#4){.+.+}-{0:0}:
lock_acquire+0x160/0x720 kernel/locking/lockdep.c:4959
percpu_down_read include/linux/percpu-rwsem.h:51 [inline]
__sb_start_write+0x14b/0x410 fs/super.c:1672
sb_start_write include/linux/fs.h:1664 [inline]
mnt_want_write+0x45/0x90 fs/namespace.c:354
ovl_maybe_copy_up+0x117/0x180 fs/overlayfs/copy_up.c:961
ovl_open+0xa2/0x200 fs/overlayfs/file.c:145
do_dentry_open+0x813/0x1070 fs/open.c:828
vfs_open fs/open.c:942 [inline]
dentry_open+0xc6/0x120 fs/open.c:958
ima_calc_file_hash+0xfa/0x1f30 security/integrity/ima/ima_crypto.c:557
ima_collect_measurement+0x1fd/0x490 security/integrity/ima/ima_api.c:250
process_measurement+0xddf/0x18e0 security/integrity/ima/ima_main.c:324
ima_file_check+0x9c/0xe0 security/integrity/ima/ima_main.c:492
do_open fs/namei.c:3245 [inline]
path_openat+0x27d6/0x37f0 fs/namei.c:3360
do_filp_open+0x191/0x3a0 fs/namei.c:3387
do_sys_openat2+0x463/0x770 fs/open.c:1179
do_sys_open fs/open.c:1195 [inline]
ksys_open include/linux/syscalls.h:1388 [inline]
__do_sys_open fs/open.c:1201 [inline]
__se_sys_open fs/open.c:1199 [inline]
__x64_sys_open+0x1af/0x1e0 fs/open.c:1199
do_syscall_64+0x73/0xe0 arch/x86/entry/common.c:359
entry_SYSCALL_64_after_hwframe+0x44/0xa9
-> #0 (&iint->mutex){+.+.}-{3:3}:
check_prev_add kernel/locking/lockdep.c:2496 [inline]
check_prevs_add kernel/locking/lockdep.c:2601 [inline]
validate_chain+0x1b0c/0x8920 kernel/locking/lockdep.c:3218
__lock_acquire+0x116c/0x2c30 kernel/locking/lockdep.c:4380
lock_acquire+0x160/0x720 kernel/locking/lockdep.c:4959
__mutex_lock_common+0x189/0x2fc0 kernel/locking/mutex.c:956
__mutex_lock kernel/locking/mutex.c:1103 [inline]
mutex_lock_nested+0x1a/0x20 kernel/locking/mutex.c:1118
process_measurement+0x66d/0x18e0 security/integrity/ima/ima_main.c:247
ima_file_check+0x9c/0xe0 security/integrity/ima/ima_main.c:492
do_open fs/namei.c:3245 [inline]
path_openat+0x27d6/0x37f0 fs/namei.c:3360
do_filp_open+0x191/0x3a0 fs/namei.c:3387
do_sys_openat2+0x463/0x770 fs/open.c:1179
do_sys_open fs/open.c:1195 [inline]
__do_sys_openat fs/open.c:1209 [inline]
__se_sys_openat fs/open.c:1204 [inline]
__x64_sys_openat+0x1c8/0x1f0 fs/open.c:1204
do_syscall_64+0x73/0xe0 arch/x86/entry/common.c:359
entry_SYSCALL_64_after_hwframe+0x44/0xa9
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(sb_writers#4);
lock(&iint->mutex);
lock(sb_writers#4);
lock(&iint->mutex);
*** DEADLOCK ***
1 lock held by syz-executor165/6816:
#0: ffff888214040450 (sb_writers#4){.+.+}-{0:0}, at: sb_start_write include/linux/fs.h:1664 [inline]
#0: ffff888214040450 (sb_writers#4){.+.+}-{0:0}, at: mnt_want_write+0x45/0x90 fs/namespace.c:354
stack backtrace:
CPU: 1 PID: 6816 Comm: syz-executor165 Not tainted 5.8.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1f0/0x31e lib/dump_stack.c:118
print_circular_bug+0xc72/0xea0 kernel/locking/lockdep.c:1703
check_noncircular+0x1fb/0x3a0 kernel/locking/lockdep.c:1827
check_prev_add kernel/locking/lockdep.c:2496 [inline]
check_prevs_add kernel/locking/lockdep.c:2601 [inline]
validate_chain+0x1b0c/0x8920 kernel/locking/lockdep.c:3218
__lock_acquire+0x116c/0x2c30 kernel/locking/lockdep.c:4380
lock_acquire+0x160/0x720 kernel/locking/lockdep.c:4959
__mutex_lock_common+0x189/0x2fc0 kernel/locking/mutex.c:956
__mutex_lock kernel/locking/mutex.c:1103 [inline]
mutex_lock_nested+0x1a/0x20 kernel/locking/mutex.c:1118
process_measurement+0x66d/0x18e0 security/integrity/ima/ima_main.c:247
ima_file_check+0x9c/0xe0 security/integrity/ima/ima_main.c:492
do_open fs/namei.c:3245 [inline]
path_openat+0x27d6/0x37f0 fs/namei.c:3360
do_filp_open+0x191/0x3a0 fs/namei.c:3387
do_sys_openat2+0x463/0x770 fs/open.c:1179
do_sys_open fs/open.c:1195 [inline]
__do_sys_openat fs/open.c:1209 [inline]
__se_sys_openat fs/open.c:1204 [inline]
__x64_sys_openat+0x1c8/0x1f0 fs/open.c:1204
do_syscall_64+0x73/0xe0 arch/x86/entry/common.c:359
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x446289
Code: Bad RIP value.
RSP: 002b:00007fc5eb6ccdb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00000000006dbc48 RCX: 0000000000446289
RDX: 000000000000275a RSI: 00000000200001c0 RDI: 00000000ffffff9c
RBP: 00000000006dbc40 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Alexei Starovoitov @ 2020-06-28 19:44 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Eric W. Biederman, Linus Torvalds, David Miller,
Greg Kroah-Hartman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <aa737d87-cf38-55d6-32f1-2d989a5412ea@i-love.sakura.ne.jp>
On Sat, Jun 27, 2020 at 10:57:10PM +0900, Tetsuo Handa wrote:
> On 2020/06/27 21:59, Eric W. Biederman wrote:
> > Can you try replacing the __fput_sync with:
> > fput(file);
> > flush_delayed_fput();
> > task_work_run();
>
> With below change, TOMOYO can obtain pathname like "tmpfs:/my\040test\040driver".
>
> Please avoid WARN_ON() if printk() is sufficient (for friendliness to panic_on_warn=1 environments).
> For argv[], I guess that fork_usermode_driver() should receive argv[] as argument rather than
> trying to split info->driver_name, for somebody might want to pass meaningful argv[] (and
> TOMOYO wants to use meaningful argv[] as a hint for identifying the intent).
>
> diff --git a/kernel/umd.c b/kernel/umd.c
> index de2f542191e5..ae6e85283f13 100644
> --- a/kernel/umd.c
> +++ b/kernel/umd.c
> @@ -7,6 +7,7 @@
> #include <linux/mount.h>
> #include <linux/fs_struct.h>
> #include <linux/umd.h>
> +#include <linux/task_work.h>
>
> static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *name)
> {
> @@ -25,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
> if (IS_ERR(mnt))
> return mnt;
>
> - file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
> + file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
> if (IS_ERR(file)) {
> mntput(mnt);
> return ERR_CAST(file);
> @@ -41,23 +42,33 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
> return ERR_PTR(err);
> }
>
> - __fput_sync(file);
> + if (current->flags & PF_KTHREAD) {
> + __fput_sync(file);
> + } else {
> + fput(file);
> + flush_delayed_fput();
> + task_work_run();
> + }
Thanks. This makes sense to me.
> return mnt;
> }
>
> /**
> * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
> - * @info: information about usermode driver
> - * @data: a blob of bytes that can be executed as a file
> - * @len: The lentgh of the blob
> + * @info: information about usermode driver (shouldn't be NULL)
> + * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
> + * @len: The lentgh of the blob (shouldn't be 0)
> *
> */
> int umd_load_blob(struct umd_info *info, const void *data, size_t len)
> {
> struct vfsmount *mnt;
>
> - if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
> + if (!info || !info->driver_name || !data || !len)
> + return -EINVAL;
> + if (info->wd.dentry || info->wd.mnt) {
> + pr_info("%s already loaded.\n", info->driver_name);
> return -EBUSY;
> + }
But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.
I would like to generalize elf_header_check() a bit and call it
before doing blob_to_mnt() to make sure that all blobs are elf files only.
Supporting '#!/bin/bash' or other things as blobs seems wrong to me.
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Tetsuo Handa @ 2020-06-29 2:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric W. Biederman, Linus Torvalds, David Miller,
Greg Kroah-Hartman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <20200628194440.puzh7nhdnk6i4rqj@ast-mbp.dhcp.thefacebook.com>
On 2020/06/29 4:44, Alexei Starovoitov wrote:
> But all the defensive programming kinda goes against general kernel style.
> I wouldn't do it. Especially pr_info() ?!
> Though I don't feel strongly about it.
Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.
>
> I would like to generalize elf_header_check() a bit and call it
> before doing blob_to_mnt() to make sure that all blobs are elf files only.
> Supporting '#!/bin/bash' or other things as blobs seems wrong to me.
Why? There is no point with forbidding "#!", for users can use a wrapper
ELF binary which contains instructions including glibc's execv()/system()
functions even if "#!" cannot be used.
What is more important is what protection/isolation properties processes started
via fork_usermode_driver() should hold, for ELF binary can contain arbitrary
instructions, these processes run as daemons (reading request from stdin and
writing response to stdout) but hidden from "/usr/bin/pstree -p 1" (because
they are forked from kthreadd kernel thread).
^ permalink raw reply
* Re: [PATCH] ima: Rename internal audit rule functions
From: Casey Schaufler @ 2020-06-29 16:24 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, linux-kernel, linux-integrity,
linux-security-module, Casey Schaufler
In-Reply-To: <20200629153037.337349-1-tyhicks@linux.microsoft.com>
On 6/29/2020 8:30 AM, Tyler Hicks wrote:
> Rename IMA's internal audit rule functions from security_filter_rule_*()
> to ima_audit_rule_*(). This avoids polluting the security_* namespace,
> which is typically reserved for general security subsystem
> infrastructure, and better aligns the IMA function names with the names
> of the LSM hooks.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Suggested-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>
> Developed on top of next-integrity-testing, commit cd1d8603df60 ("IMA:
> Add audit log for failure conditions"), plus this patch series:
>
> [PATCH v2 00/11] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
> https://lore.kernel.org/linux-integrity/20200626223900.253615-1-tyhicks@linux.microsoft.com/T/#t
>
> This patch has dependencies on the above patch series.
>
> Tested with and without CONFIG_IMA_LSM_RULES enabled by attempting to
> load IMA policy with rules containing the subj_role=foo conditional.
> Build logs are clean in both configurations. The IMA policy was first
> loaded without and then with a valid AppArmor profile named "foo". The
> behavior is the same before and after this patch is applied:
>
> | CONFIG_IMA_LSM_RULES=n | CONFIG_IMA_LSM_RULES=y
> -----------------------------------------------------------------------
> Without Profile | IMA policy load fails | IMA policy load fails
> With Profile | IMA policy load fails | IMA policy load succeeds
>
> security/integrity/ima/ima.h | 16 +++++++--------
> security/integrity/ima/ima_policy.c | 30 +++++++++++++----------------
> 2 files changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index ff2bf57ff0c7..5d62ee8319f4 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -419,24 +419,24 @@ static inline void ima_free_modsig(struct modsig *modsig)
> /* LSM based policy rules require audit */
> #ifdef CONFIG_IMA_LSM_RULES
>
> -#define security_filter_rule_init security_audit_rule_init
> -#define security_filter_rule_free security_audit_rule_free
> -#define security_filter_rule_match security_audit_rule_match
> +#define ima_audit_rule_init security_audit_rule_init
> +#define ima_audit_rule_free security_audit_rule_free
> +#define ima_audit_rule_match security_audit_rule_match
>
> #else
>
> -static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> - void **lsmrule)
> +static inline int ima_audit_rule_init(u32 field, u32 op, char *rulestr,
> + void **lsmrule)
> {
> return -EINVAL;
> }
>
> -static inline void security_filter_rule_free(void *lsmrule)
> +static inline void ima_audit_rule_free(void *lsmrule)
> {
> }
>
> -static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> - void *lsmrule)
> +static inline int ima_audit_rule_match(u32 secid, u32 field, u32 op,
> + void *lsmrule)
> {
> return -EINVAL;
> }
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 294323b36d06..60894656a4b7 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> int i;
>
> for (i = 0; i < MAX_LSM_RULES; i++) {
> - security_filter_rule_free(entry->lsm[i].rule);
> + ima_audit_rule_free(entry->lsm[i].rule);
> kfree(entry->lsm[i].args_p);
> }
> }
> @@ -308,10 +308,9 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> */
> entry->lsm[i].args_p = NULL;
>
> - security_filter_rule_init(nentry->lsm[i].type,
> - Audit_equal,
> - nentry->lsm[i].args_p,
> - &nentry->lsm[i].rule);
> + ima_audit_rule_init(nentry->lsm[i].type, Audit_equal,
> + nentry->lsm[i].args_p,
> + &nentry->lsm[i].rule);
> if (!nentry->lsm[i].rule)
> pr_warn("rule for LSM \'%s\' is undefined\n",
> entry->lsm[i].args_p);
> @@ -495,18 +494,16 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_OBJ_ROLE:
> case LSM_OBJ_TYPE:
> security_inode_getsecid(inode, &osid);
> - rc = security_filter_rule_match(osid,
> - rule->lsm[i].type,
> - Audit_equal,
> - rule->lsm[i].rule);
> + rc = ima_audit_rule_match(osid, rule->lsm[i].type,
> + Audit_equal,
> + rule->lsm[i].rule);
> break;
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> - rc = security_filter_rule_match(secid,
> - rule->lsm[i].type,
> - Audit_equal,
> - rule->lsm[i].rule);
> + rc = ima_audit_rule_match(secid, rule->lsm[i].type,
> + Audit_equal,
> + rule->lsm[i].rule);
> default:
> break;
> }
> @@ -901,10 +898,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
> return -ENOMEM;
>
> entry->lsm[lsm_rule].type = audit_type;
> - result = security_filter_rule_init(entry->lsm[lsm_rule].type,
> - Audit_equal,
> - entry->lsm[lsm_rule].args_p,
> - &entry->lsm[lsm_rule].rule);
> + result = ima_audit_rule_init(entry->lsm[lsm_rule].type, Audit_equal,
> + entry->lsm[lsm_rule].args_p,
> + &entry->lsm[lsm_rule].rule);
> if (!entry->lsm[lsm_rule].rule) {
> pr_warn("rule for LSM \'%s\' is undefined\n",
> entry->lsm[lsm_rule].args_p);
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-06-29 15:27 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jarkko Sakkinen, x86, linux-sgx, linux-kernel,
linux-security-module, Jethro Beekman, Haitao Huang, Chunyang Hui,
Jordan Hand, Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200627174335.GC15585@zn.tnic>
On Sat, Jun 27, 2020 at 07:43:35PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
> > + void *token)
> > +{
> > + u64 mrsigner[4];
> > + int ret;
> > + int i;
> > + int j;
> > +
> > + /* Check that the required attributes have been authorized. */
> > + if (encl->secs_attributes & ~encl->allowed_attributes)
> > + return -EACCES;
> > +
> > + ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&encl->lock);
> > +
> > + if (atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) {
> > + ret = -EFAULT;
> > + goto err_out;
> > + }
>
> That test should be the first thing this function or its caller does.
Hmm, I was going to say that SGX_ENCL_INITIALIZED can't be checked until
encl->lock is held, but that's not true for this path as mutual exclusion
is provided by the SGX_ENCL_IOCTL flag. So yeah, this can be checked at
the same time as SGX_ENCL_CREATED in sgx_ioc_enclave_init().
> > + for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> > + for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
>
> Ew, what's that double-loop for?
>
> It tries to init an enclave a bunch of times. Why does it need to init
> more than once?
ENCLS[EINIT] is interruptible because it has such a high latency, e.g. 50k+
cycles on success. If an IRQ/NMI/SMI becomes pending, EINIT may fail with
SGX_UNMASKED_EVENT so that the event can be serviced.
The idea behind the double loop is to try EINIT in a tight loop, then back
off and sleep for a while before retrying that tight inner loop.
> > + ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> > + mrsigner);
> > + if (ret == SGX_UNMASKED_EVENT)
> > + continue;
> > + else
> > + break;
> > + }
> > +
> > + if (ret != SGX_UNMASKED_EVENT)
> > + break;
> > +
> > + msleep_interruptible(SGX_EINIT_SLEEP_TIME);
> > +
> > + if (signal_pending(current)) {
> > + ret = -ERESTARTSYS;
> > + goto err_out;
> > + }
> > + }
> > +
> > + if (ret & ENCLS_FAULT_FLAG) {
> > + if (encls_failed(ret))
> > + ENCLS_WARN(ret, "EINIT");
> > +
> > + sgx_encl_destroy(encl);
> > + ret = -EFAULT;
> > + } else if (ret) {
> > + pr_debug("EINIT returned %d\n", ret);
> > + ret = -EPERM;
> > + } else {
> > + atomic_or(SGX_ENCL_INITIALIZED, &encl->flags);
> > + }
> > +
> > +err_out:
> > + mutex_unlock(&encl->lock);
> > + return ret;
> > +}
> > +
> > +/**
> > + * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
> > + *
> > + * @filep: open file to /dev/sgx
>
> @encl: pointer to an enclave instance (via ioctl() file pointer)
>
> > + * @arg: userspace pointer to a struct sgx_enclave_init instance
> > + *
> > + * Flush any outstanding enqueued EADD operations and perform EINIT. The
> > + * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
> > + * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
> > + *
> > + * Return:
> > + * 0 on success,
> > + * SGX error code on EINIT failure,
> > + * -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> > +{
> > + struct sgx_sigstruct *sigstruct;
> > + struct sgx_enclave_init einit;
> > + struct page *initp_page;
> > + void *token;
> > + int ret;
> > +
> > + if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
>
> Might just as well check the other flags: doing EINIT on an already
> initialized enclave - SGX_ENCL_INITIALIZED - is perhaps a nono or
> similarly on a SGX_ENCL_DEAD enclave.
>
> And you could do similar sanity checks in the other ioctl functions.
Ya, as above, SGX_ENCL_INITIALIZED can be checked here.
SGX_ENCL_DEAD is actually already checked in in the top level sgx_ioctl(),
i.e. the check in sgx_encl_add_page() can technically be flat out dropped.
I say "technically" because I'm a bit torn over SGX_ENCL_DEAD; encl->lock
must be held to SGX_ENCL_DEAD (the page fault and reclaim flows rely on
this), but as it stands today only ioctl() paths (guarded by SGX_ENCL_IOCTL)
and sgx_release() (makes the ioctls() unreachable) set SGX_ENCL_DEAD.
So it's safe to check SGX_ENCL_DEAD from ioctl() context without holding
encl->lock, at least in the current code base, but it feels weird/sketchy.
In the end I don't think I have a strong opinion. Removing the technically
unnecessary DEAD check in sgx_encl_add_page() is the simplest change, so it
may make sense to do that and nothing more for initial upstreaming. Long
term, I fully expect we'll add paths that set SGX_ENCL_DEAD outside of
ioctl() context, e.g. to handle EPC OOM, but it wouldn't be a bad thing to
have a standalone commit in a future series to add DEAD checks (under
encl->lock) in the ADD and INIT flows.
^ permalink raw reply
* Re: [PATCH v2 09/11] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Tyler Hicks @ 2020-06-29 14:16 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <0e7012e7-e1df-466d-9d51-a691f779570a@linux.microsoft.com>
On 2020-06-27 16:49:46, Lakshmi Ramasubramanian wrote:
> On 6/26/20 3:38 PM, Tyler Hicks wrote:
>
> > Use ima_validate_rule() to ensure that the combination of a hook
> > function and the keyrings conditional is valid and that the keyrings
> > conditional is not specified without an explicit KEY_CHECK func
> > conditional. This is a code cleanup and has no user-facing change.
>
> In addition to checking for func=KEY_CHECK and the keyrings conditional, the
> patch also validates the flags for other IMA hooks (such as
> KEXEC_KERNEL_CHECK, POLICY_CHECK, etc.) Would be good to mention that in the
> patch description.
It actually doesn't do any additional validation of other IMA hooks at
this time. That check on entry->flags is an allowlist of every possible
conditional flag except IMA_KEYRINGS. The ima_parse_rule() function is
already validating all of these conditional flags.
Tyler
>
> -lakshmi
>
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >
> > * v2
> > - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
> > IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
> > present in the rule entry flags for non-buffer hook functions.
> >
> > security/integrity/ima/ima_policy.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 8cdca2399d59..43d49ad958fb 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > case KEXEC_KERNEL_CHECK:
> > case KEXEC_INITRAMFS_CHECK:
> > case POLICY_CHECK:
> > + if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
> > + IMA_UID | IMA_FOWNER | IMA_FSUUID |
> > + IMA_INMASK | IMA_EUID | IMA_PCR |
> > + IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> > + IMA_PERMIT_DIRECTIO |
> > + IMA_MODSIG_ALLOWED |
> > + IMA_CHECK_BLACKLIST))
> > + return false;
> > +
> > break;
> > case KEXEC_CMDLINE:
> > if (entry->action & ~(MEASURE | DONT_MEASURE))
> > @@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > default:
> > return false;
> > }
> > - }
> > + } else if (entry->flags & IMA_KEYRINGS)
> > + return false;
> > return true;
> > }
> > @@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > keyrings_len = strlen(args[0].from) + 1;
> > if ((entry->keyrings) ||
> > - (entry->func != KEY_CHECK) ||
> > (keyrings_len < 2)) {
> > result = -EINVAL;
> > break;
> >
^ permalink raw reply
* [PATCH] ima: Rename internal audit rule functions
From: Tyler Hicks @ 2020-06-29 15:30 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, linux-kernel, linux-integrity,
linux-security-module, Casey Schaufler
Rename IMA's internal audit rule functions from security_filter_rule_*()
to ima_audit_rule_*(). This avoids polluting the security_* namespace,
which is typically reserved for general security subsystem
infrastructure, and better aligns the IMA function names with the names
of the LSM hooks.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Suggested-by: Casey Schaufler <casey@schaufler-ca.com>
---
Developed on top of next-integrity-testing, commit cd1d8603df60 ("IMA:
Add audit log for failure conditions"), plus this patch series:
[PATCH v2 00/11] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
https://lore.kernel.org/linux-integrity/20200626223900.253615-1-tyhicks@linux.microsoft.com/T/#t
This patch has dependencies on the above patch series.
Tested with and without CONFIG_IMA_LSM_RULES enabled by attempting to
load IMA policy with rules containing the subj_role=foo conditional.
Build logs are clean in both configurations. The IMA policy was first
loaded without and then with a valid AppArmor profile named "foo". The
behavior is the same before and after this patch is applied:
| CONFIG_IMA_LSM_RULES=n | CONFIG_IMA_LSM_RULES=y
-----------------------------------------------------------------------
Without Profile | IMA policy load fails | IMA policy load fails
With Profile | IMA policy load fails | IMA policy load succeeds
security/integrity/ima/ima.h | 16 +++++++--------
security/integrity/ima/ima_policy.c | 30 +++++++++++++----------------
2 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ff2bf57ff0c7..5d62ee8319f4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -419,24 +419,24 @@ static inline void ima_free_modsig(struct modsig *modsig)
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
-#define security_filter_rule_init security_audit_rule_init
-#define security_filter_rule_free security_audit_rule_free
-#define security_filter_rule_match security_audit_rule_match
+#define ima_audit_rule_init security_audit_rule_init
+#define ima_audit_rule_free security_audit_rule_free
+#define ima_audit_rule_match security_audit_rule_match
#else
-static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
- void **lsmrule)
+static inline int ima_audit_rule_init(u32 field, u32 op, char *rulestr,
+ void **lsmrule)
{
return -EINVAL;
}
-static inline void security_filter_rule_free(void *lsmrule)
+static inline void ima_audit_rule_free(void *lsmrule)
{
}
-static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
- void *lsmrule)
+static inline int ima_audit_rule_match(u32 secid, u32 field, u32 op,
+ void *lsmrule)
{
return -EINVAL;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 294323b36d06..60894656a4b7 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
int i;
for (i = 0; i < MAX_LSM_RULES; i++) {
- security_filter_rule_free(entry->lsm[i].rule);
+ ima_audit_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
}
@@ -308,10 +308,9 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
*/
entry->lsm[i].args_p = NULL;
- security_filter_rule_init(nentry->lsm[i].type,
- Audit_equal,
- nentry->lsm[i].args_p,
- &nentry->lsm[i].rule);
+ ima_audit_rule_init(nentry->lsm[i].type, Audit_equal,
+ nentry->lsm[i].args_p,
+ &nentry->lsm[i].rule);
if (!nentry->lsm[i].rule)
pr_warn("rule for LSM \'%s\' is undefined\n",
entry->lsm[i].args_p);
@@ -495,18 +494,16 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
case LSM_OBJ_ROLE:
case LSM_OBJ_TYPE:
security_inode_getsecid(inode, &osid);
- rc = security_filter_rule_match(osid,
- rule->lsm[i].type,
- Audit_equal,
- rule->lsm[i].rule);
+ rc = ima_audit_rule_match(osid, rule->lsm[i].type,
+ Audit_equal,
+ rule->lsm[i].rule);
break;
case LSM_SUBJ_USER:
case LSM_SUBJ_ROLE:
case LSM_SUBJ_TYPE:
- rc = security_filter_rule_match(secid,
- rule->lsm[i].type,
- Audit_equal,
- rule->lsm[i].rule);
+ rc = ima_audit_rule_match(secid, rule->lsm[i].type,
+ Audit_equal,
+ rule->lsm[i].rule);
default:
break;
}
@@ -901,10 +898,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
return -ENOMEM;
entry->lsm[lsm_rule].type = audit_type;
- result = security_filter_rule_init(entry->lsm[lsm_rule].type,
- Audit_equal,
- entry->lsm[lsm_rule].args_p,
- &entry->lsm[lsm_rule].rule);
+ result = ima_audit_rule_init(entry->lsm[lsm_rule].type, Audit_equal,
+ entry->lsm[lsm_rule].args_p,
+ &entry->lsm[lsm_rule].rule);
if (!entry->lsm[lsm_rule].rule) {
pr_warn("rule for LSM \'%s\' is undefined\n",
entry->lsm[lsm_rule].args_p);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-29 15:37 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, x86, linux-sgx, linux-kernel,
linux-security-module, Jethro Beekman, Haitao Huang, Chunyang Hui,
Jordan Hand, Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200629152718.GA12312@linux.intel.com>
On Mon, Jun 29, 2020 at 08:27:19AM -0700, Sean Christopherson wrote:
> Hmm, I was going to say that SGX_ENCL_INITIALIZED can't be checked until
> encl->lock is held, but that's not true for this path as mutual exclusion
> is provided by the SGX_ENCL_IOCTL flag. So yeah, this can be checked at
> the same time as SGX_ENCL_CREATED in sgx_ioc_enclave_init().
Right, so my point is to have state checks for flags which make sense in
all ioctl entry points, in order to catch a misuse early. But we're on
the same page.
> ENCLS[EINIT] is interruptible because it has such a high latency, e.g. 50k+
> cycles on success. If an IRQ/NMI/SMI becomes pending, EINIT may fail with
> SGX_UNMASKED_EVENT so that the event can be serviced.
>
> The idea behind the double loop is to try EINIT in a tight loop, then back
> off and sleep for a while before retrying that tight inner loop.
That gist of that kinda wants to be in a comment over that double-loop for
future on-lookers.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ 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