* Re: sleep in selinux_audit_rule_init
From: Janne Karhunen @ 2019-05-30 10:39 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module
In-Reply-To: <1432f617-424e-044c-4f78-47f1100262ae@tycho.nsa.gov>
On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > Ok. The question is then how should IMA handle missing domains/types.
> > Just dropping IMA policy rules doesn't sound safe, nor does skipping
> > rules in case the domains/types are restored.
>
> You can just do what audit_dupe_lsm_field() does. It effectively
> disables the rule upon the invalidation (which makes sense, since it can
> no longer match anything since nothing can have that domain/type) but
> retains the string value so it can later re-activate the rule if the
> domain/type becomes valid again later.
Finally got a moment to look into this. It looks to me there is
already a notifier? Could something like this work?
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 5749ec92516f..449502f5c3dc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -52,6 +52,10 @@ int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
static struct workqueue_struct *ima_update_wq;
+static struct notifier_block ima_lsm_policy_notifier = {
+ .notifier_call = ima_lsm_policy_change,
+};
+
static int __init hash_setup(char *str)
{
struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -691,6 +695,10 @@ static int __init init_ima(void)
error = ima_init();
}
+ error = register_lsm_notifier(&ima_lsm_policy_notifier);
+ if (error)
+ pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
if (!error)
ima_update_policy_flag();
else
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index e0cc323f948f..c3983d24279a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -252,8 +252,8 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
/*
* 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
- * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if
- * they don't.
+ * the reloaded LSM policy. Keep currently invalid fields around in case
+ * they become valid after a policy reload.
*/
static void ima_lsm_update_rules(void)
{
@@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
Audit_equal,
entry->lsm[i].args_p,
&entry->lsm[i].rule);
- BUG_ON(!entry->lsm[i].rule);
+ if (result == -EINVAL)
+ pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+ entry->lsm[i].type);
}
}
}
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data)
+{
+ if (event != LSM_POLICY_CHANGE)
+ return NOTIFY_DONE;
+
+ ima_lsm_update_rules();
+ return NOTIFY_DONE;
+}
+
/**
* ima_match_rules - determine whether an inode matches the measure rule.
* @rule: a pointer to a rule
@@ -327,11 +339,10 @@ static bool ima_match_rules(struct
ima_rule_entry *rule, struct inode *inode,
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid;
- int retried = 0;
if (!rule->lsm[i].rule)
continue;
-retry:
+
switch (i) {
case LSM_OBJ_USER:
case LSM_OBJ_ROLE:
@@ -352,11 +363,6 @@ static bool ima_match_rules(struct ima_rule_entry
*rule, struct inode *inode,
default:
break;
}
- if ((rc < 0) && (!retried)) {
- retried = 1;
- ima_lsm_update_rules();
- goto retry;
- }
if (!rc)
return false;
}
--
Janne
^ permalink raw reply related
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Jan Kara @ 2019-05-30 11:00 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, David Howells, Al Viro, Ian Kent, linux-fsdevel,
linux-api, linux-block, keyrings, LSM List, linux-kernel
In-Reply-To: <CAOQ4uxjLzURf8c1UH_xCJKkuD2es8i-=P-ZNM=t3aFcZLMwXEg@mail.gmail.com>
On Wed 29-05-19 18:53:21, Amir Goldstein wrote:
> > > David,
> > >
> > > I am interested to know how you envision filesystem notifications would
> > > look with this interface.
> > >
> > > fanotify can certainly benefit from providing a ring buffer interface to read
> > > events.
> > >
> > > From what I have seen, a common practice of users is to monitor mounts
> > > (somehow) and place FAN_MARK_MOUNT fanotify watches dynamically.
> > > It'd be good if those users can use a single watch mechanism/API for
> > > watching the mount namespace and filesystem events within mounts.
> > >
> > > A similar usability concern is with sb_notify and FAN_MARK_FILESYSTEM.
> > > It provides users with two complete different mechanisms to watch error
> > > and filesystem events. That is generally not a good thing to have.
> > >
> > > I am not asking that you implement fs_notify() before merging sb_notify()
> > > and I understand that you have a use case for sb_notify().
> > > I am asking that you show me the path towards a unified API (how a
> > > typical program would look like), so that we know before merging your
> > > new API that it could be extended to accommodate fsnotify events
> > > where the final result will look wholesome to users.
> >
> > Are you sure we want to combine notification about file changes etc. with
> > administrator-type notifications about the filesystem? To me these two
> > sound like rather different (although sometimes related) things.
> >
>
> Well I am sure that ring buffer for fanotify events would be useful, so
> seeing that David is proposing a generic notification mechanism, I wanted
> to know how that mechanism could best share infrastructure with fsnotify.
>
> But apart from that I foresee the questions from users about why the
> mount notification API and filesystem events API do not have better
> integration.
>
> The way I see it, the notification queue can serve several classes
> of notifications and fsnotify could be one of those classes
> (at least FAN_CLASS_NOTIF fits nicely to the model).
I agree that for some type of fsnotify uses a ring buffer would make sense.
But for others - such as permission events or unlimited queues - you cannot
really use the ring buffer and I don't like the idea of having different
ways of passing fsnotify events to userspace based on notification group
type...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v2 1/3] evm: check hash algorithm passed to init_desc()
From: Mimi Zohar @ 2019-05-30 11:53 UTC (permalink / raw)
To: Roberto Sassu, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20190529133035.28724-2-roberto.sassu@huawei.com>
On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> This patch prevents memory access beyond the evm_tfm array by checking the
> validity of the index (hash algorithm) passed to init_desc(). The hash
> algorithm can be arbitrarily set if the security.ima xattr type is not
> EVM_XATTR_HMAC.
>
> Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
Thanks, queued.
> ---
> security/integrity/evm/evm_crypto.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index e11564eb645b..82a38e801ee4 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
> tfm = &hmac_tfm;
> algo = evm_hmac;
> } else {
> + if (hash_algo >= HASH_ALGO__LAST)
> + return ERR_PTR(-EINVAL);
> +
> tfm = &evm_tfm[hash_algo];
> algo = hash_algo_name[hash_algo];
> }
^ permalink raw reply
* Re: [PATCH v2 3/3] ima: show rules with IMA_INMASK correctly
From: Mimi Zohar @ 2019-05-30 11:53 UTC (permalink / raw)
To: Roberto Sassu, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20190529133035.28724-4-roberto.sassu@huawei.com>
On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> Show the '^' character when a policy rule has flag IMA_INMASK.
>
> Fixes: 80eae209d63ac ("IMA: allow reading back the current IMA policy")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
Thanks, queued.
> ---
> security/integrity/ima/ima_policy.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..ae4034f041c4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1146,10 +1146,10 @@ enum {
> };
>
> static const char *const mask_tokens[] = {
> - "MAY_EXEC",
> - "MAY_WRITE",
> - "MAY_READ",
> - "MAY_APPEND"
> + "^MAY_EXEC",
> + "^MAY_WRITE",
> + "^MAY_READ",
> + "^MAY_APPEND"
> };
>
> #define __ima_hook_stringify(str) (#str),
> @@ -1209,6 +1209,7 @@ int ima_policy_show(struct seq_file *m, void *v)
> struct ima_rule_entry *entry = v;
> int i;
> char tbuf[64] = {0,};
> + int offset = 0;
>
> rcu_read_lock();
>
> @@ -1232,15 +1233,17 @@ int ima_policy_show(struct seq_file *m, void *v)
> if (entry->flags & IMA_FUNC)
> policy_func_show(m, entry->func);
>
> - if (entry->flags & IMA_MASK) {
> + if ((entry->flags & IMA_MASK) || (entry->flags & IMA_INMASK)) {
> + if (entry->flags & IMA_MASK)
> + offset = 1;
> if (entry->mask & MAY_EXEC)
> - seq_printf(m, pt(Opt_mask), mt(mask_exec));
> + seq_printf(m, pt(Opt_mask), mt(mask_exec) + offset);
> if (entry->mask & MAY_WRITE)
> - seq_printf(m, pt(Opt_mask), mt(mask_write));
> + seq_printf(m, pt(Opt_mask), mt(mask_write) + offset);
> if (entry->mask & MAY_READ)
> - seq_printf(m, pt(Opt_mask), mt(mask_read));
> + seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
> if (entry->mask & MAY_APPEND)
> - seq_printf(m, pt(Opt_mask), mt(mask_append));
> + seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
> seq_puts(m, " ");
> }
>
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Mimi Zohar @ 2019-05-30 12:00 UTC (permalink / raw)
To: Roberto Sassu, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20190529133035.28724-3-roberto.sassu@huawei.com>
On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> Currently, ima_appraise_measurement() ignores the EVM status when
> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
> security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
> ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
> status. The problem is that the EVM status is overwritten with the
> > appraisal statu
Roberto, your framing of this problem is harsh and misleading. IMA
and EVM are intentionally independent of each other and can be
configured independently of each other. The intersection of the two
is the call to evm_verifyxattr(). INTEGRITY_UNKNOWN is returned for a
number of reasons - when EVM is not configured, the EVM hmac key has
not yet been loaded, the protected security attribute is unknown, or
the file is not in policy.
This patch does not differentiate between any of the above cases,
requiring mutable files to always be protected by EVM, when specified
as an "ima_appraise=" option on the boot command line.
IMA could be extended to require EVM on a per IMA policy rule basis.
Instead of framing allowing IMA file hashes without EVM as a bug that
has existed from the very beginning, now that IMA/EVM have matured and
is being used, you could frame it as extending IMA or hardening.
>
> This patch mitigates the issue by selecting signature verification as the
> only method allowed for appraisal when EVM is not initialized. Since the
> new behavior might break user space, it must be turned on by adding the
> '-evm' suffix to the value of the ima_appraise= kernel option.
>
> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
> Documentation/admin-guide/kernel-parameters.txt | 3 ++-
> security/integrity/ima/ima_appraise.c | 8 ++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f6664b2e2..d84a2e612b93 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1585,7 +1585,8 @@
> Set number of hash buckets for inode cache.
>
> ima_appraise= [IMA] appraise integrity measurements
> - Format: { "off" | "enforce" | "fix" | "log" }
> + Format: { "off" | "enforce" | "fix" | "log" |
> + "enforce-evm" | "log-evm" }
Is it necessary to define both "enforce-evm" and "log-evm"? Perhaps
defining "require-evm" is sufficient.
Mimi
> default: "enforce"
>
> ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead.
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 5fb7127bbe68..afef06e10fb9 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -18,6 +18,7 @@
>
> #include "ima.h"
>
> +static bool ima_appraise_req_evm __ro_after_init;
> static int __init default_appraise_setup(char *str)
> {
> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> @@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
> else if (strncmp(str, "fix", 3) == 0)
> ima_appraise = IMA_APPRAISE_FIX;
> #endif
> + if (strcmp(str, "enforce-evm") == 0 ||
> + strcmp(str, "log-evm") == 0)
> + ima_appraise_req_evm = true;
> return 1;
> }
>
> @@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
> switch (status) {
> case INTEGRITY_PASS:
> case INTEGRITY_PASS_IMMUTABLE:
> + break;
> case INTEGRITY_UNKNOWN:
> + if (ima_appraise_req_evm &&
> + xattr_value->type != EVM_IMA_XATTR_DIGSIG)
> + goto out;
> break;
> case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
> case INTEGRITY_NOLABEL: /* No security.evm xattr. */
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Stephen Smalley @ 2019-05-30 12:07 UTC (permalink / raw)
To: Janne Karhunen
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
Dan Jurgens
In-Reply-To: <CAE=NcranYrvV5Xnu8656kyDVUUzCs=Tdy+fkeo5jwVhtF8=81Q@mail.gmail.com>
On 5/30/19 6:39 AM, Janne Karhunen wrote:
> On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>>> Ok. The question is then how should IMA handle missing domains/types.
>>> Just dropping IMA policy rules doesn't sound safe, nor does skipping
>>> rules in case the domains/types are restored.
>>
>> You can just do what audit_dupe_lsm_field() does. It effectively
>> disables the rule upon the invalidation (which makes sense, since it can
>> no longer match anything since nothing can have that domain/type) but
>> retains the string value so it can later re-activate the rule if the
>> domain/type becomes valid again later.
>
> Finally got a moment to look into this. It looks to me there is
> already a notifier? Could something like this work?
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..2203451862d4 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
> int ima_init_template(void);
> void ima_init_template_list(void);
> int __init ima_init_digests(void);
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> + void *lsm_data);
>
> /*
> * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 5749ec92516f..449502f5c3dc 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -52,6 +52,10 @@ int ima_hash_algo = HASH_ALGO_SHA1;
> static int hash_setup_done;
> static struct workqueue_struct *ima_update_wq;
>
> +static struct notifier_block ima_lsm_policy_notifier = {
> + .notifier_call = ima_lsm_policy_change,
> +};
> +
> static int __init hash_setup(char *str)
> {
> struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -691,6 +695,10 @@ static int __init init_ima(void)
> error = ima_init();
> }
>
> + error = register_lsm_notifier(&ima_lsm_policy_notifier);
> + if (error)
> + pr_warn("Couldn't register LSM notifier, error %d\n", error);
> +
> if (!error)
> ima_update_policy_flag();
> else
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..c3983d24279a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -252,8 +252,8 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
> /*
> * 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
> - * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if
> - * they don't.
> + * the reloaded LSM policy. Keep currently invalid fields around in case
> + * they become valid after a policy reload.
> */
> static void ima_lsm_update_rules(void)
> {
> @@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
> Audit_equal,
> entry->lsm[i].args_p,
> &entry->lsm[i].rule);
> - BUG_ON(!entry->lsm[i].rule);
> + if (result == -EINVAL)
> + pr_warn("ima: rule for LSM \'%d\' is invalid\n",
> + entry->lsm[i].type);
I could be wrong, but I think there is still a problem here in that you
are modifying entry->lsm[i].rule in-place, but it is protected under RCU
and therefore needs to be duplicated and then modified? Also you are
leaking the old rule? Both of those issues also exist prior to your
patch but you aren't fixing them here. And lastly, it looks like lsm
notifiers are atomic notifiers (not clear to me why) so you can't block
in the callback, thereby requiring scheduling the work as is done in
infiniband. I'm not sure though why we can't make the lsm notifiers
blocking notifiers. The only callers of call_lsm_notifier() are
sel_write_enforce() and selinux_lsm_notifier_avc_callback(), called from
avc_ss_reset(), called from sel_write_enforce(), security_load_policy()
and security_set_bools(), all outside of locks and in process context
AFAICS.
> }
> }
> }
>
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> + void *lsm_data)
> +{
> + if (event != LSM_POLICY_CHANGE)
> + return NOTIFY_DONE;
> +
> + ima_lsm_update_rules();
> + return NOTIFY_DONE;
> +}
> +
> /**
> * ima_match_rules - determine whether an inode matches the measure rule.
> * @rule: a pointer to a rule
> @@ -327,11 +339,10 @@ static bool ima_match_rules(struct
> ima_rule_entry *rule, struct inode *inode,
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> u32 osid;
> - int retried = 0;
>
> if (!rule->lsm[i].rule)
> continue;
> -retry:
> +
> switch (i) {
> case LSM_OBJ_USER:
> case LSM_OBJ_ROLE:
> @@ -352,11 +363,6 @@ static bool ima_match_rules(struct ima_rule_entry
> *rule, struct inode *inode,
> default:
> break;
> }
> - if ((rc < 0) && (!retried)) {
> - retried = 1;
> - ima_lsm_update_rules();
> - goto retry;
> - }
> if (!rc)
> return false;
> }
>
>
>
> --
> Janne
>
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Paul Moore @ 2019-05-30 12:29 UTC (permalink / raw)
To: Stephen Smalley
Cc: Janne Karhunen, Mimi Zohar, linux-integrity,
linux-security-module, Dan Jurgens
In-Reply-To: <a39cedd1-a932-3140-bf11-ef4e3a88e254@tycho.nsa.gov>
On Thu, May 30, 2019 at 8:07 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> ... And lastly, it looks like lsm
> notifiers are atomic notifiers (not clear to me why) so you can't block
> in the callback, thereby requiring scheduling the work as is done in
> infiniband. I'm not sure though why we can't make the lsm notifiers
> blocking notifiers. The only callers of call_lsm_notifier() are
> sel_write_enforce() and selinux_lsm_notifier_avc_callback(), called from
> avc_ss_reset(), called from sel_write_enforce(), security_load_policy()
> and security_set_bools(), all outside of locks and in process context
> AFAICS.
Off the top of my head I don't recall why the atomic notifiers were
chosen over the blocking notifiers; it may simply be an artifact of an
interim patch that was changed. Regardless, I have no problem if we
switch to using blocking notifiers. However, if we are changing it
now it might be a good idea to also add a "block"/"blocking" somewhere
in the lsm_notifier functions' name to make the change obvious and to
help make it easier if we ever need to add atomic notifier support in
the future.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Janne Karhunen @ 2019-05-30 13:27 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
Dan Jurgens
In-Reply-To: <a39cedd1-a932-3140-bf11-ef4e3a88e254@tycho.nsa.gov>
On Thu, May 30, 2019 at 3:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > @@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
> > Audit_equal,
> > entry->lsm[i].args_p,
> > &entry->lsm[i].rule);
> > - BUG_ON(!entry->lsm[i].rule);
> > + if (result == -EINVAL)
> > + pr_warn("ima: rule for LSM \'%d\' is invalid\n",
> > + entry->lsm[i].type);
>
> I could be wrong, but I think there is still a problem here in that you
> are modifying entry->lsm[i].rule in-place, but it is protected under RCU
> and therefore needs to be duplicated and then modified? Also you are
> leaking the old rule?
Right. Bit too fast tapping the keyboard without thinking, will fix
and post in the proper form. But I guess the original point was to
verify if that 'notifier_block' is indeed the right way to get the
update notification?
> Both of those issues also exist prior to your
> patch but you aren't fixing them here. And lastly, it looks like lsm
> notifiers are atomic notifiers (not clear to me why) so you can't block
> in the callback, thereby requiring scheduling the work as is done in
> infiniband.
Great catch, thank you. That's an easy fix if no-one objects pushing
these through the system-wq for example.
--
Janne
^ permalink raw reply
* Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings
From: David Howells @ 2019-05-30 13:31 UTC (permalink / raw)
To: Eric Biggers; +Cc: dhowells, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20190529232500.GA131466@gmail.com>
Eric Biggers <ebiggers@kernel.org> wrote:
> This shows up after a few seconds of syzkaller fuzzing with a description of
> KEYCTL_MOVE added:
Yeah... I'm fixing that now. I've also created a bunch of tests, manpages,
etc. for keyutils which I'll push when I've fixed my patches.
David
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Stephen Smalley @ 2019-05-30 14:17 UTC (permalink / raw)
To: Janne Karhunen
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module,
Dan Jurgens
In-Reply-To: <CAE=NcrYV_x6dKbVvO+EDZNKMuEqVhyMKrdEjuUJXqfcYVeiXBA@mail.gmail.com>
On 5/30/19 9:27 AM, Janne Karhunen wrote:
> On Thu, May 30, 2019 at 3:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>>> @@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
>>> Audit_equal,
>>> entry->lsm[i].args_p,
>>> &entry->lsm[i].rule);
>>> - BUG_ON(!entry->lsm[i].rule);
>>> + if (result == -EINVAL)
>>> + pr_warn("ima: rule for LSM \'%d\' is invalid\n",
>>> + entry->lsm[i].type);
>>
>> I could be wrong, but I think there is still a problem here in that you
>> are modifying entry->lsm[i].rule in-place, but it is protected under RCU
>> and therefore needs to be duplicated and then modified? Also you are
>> leaking the old rule?
>
> Right. Bit too fast tapping the keyboard without thinking, will fix
> and post in the proper form. But I guess the original point was to
> verify if that 'notifier_block' is indeed the right way to get the
> update notification?
Yes.
>
>> Both of those issues also exist prior to your
>> patch but you aren't fixing them here. And lastly, it looks like lsm
>> notifiers are atomic notifiers (not clear to me why) so you can't block
>> in the callback, thereby requiring scheduling the work as is done in
>> infiniband.
>
> Great catch, thank you. That's an easy fix if no-one objects pushing
> these through the system-wq for example.
I think you can switch the lsm notifier over to using blocking notifiers
instead; there seems to be no valid reason for making it atomic.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-30 14:22 UTC (permalink / raw)
To: Xing, Cedric, Christopherson, Sean J, William Roberts
Cc: Andy Lutomirski, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654EB487@ORSMSX116.amr.corp.intel.com>
On 5/30/19 2:12 AM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-owner@vger.kernel.org] On Behalf
>> Of Stephen Smalley
>> Sent: Wednesday, May 29, 2019 7:08 AM
>>
>> On 5/28/19 4:24 PM, Sean Christopherson wrote:
>>> On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
>>>>> From: Andy Lutomirski [mailto:luto@kernel.org]
>>>>> Sent: Saturday, May 25, 2019 5:58 PM
>>>>>
>>>>> On Sat, May 25, 2019 at 3:40 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>>>>>>
>>>>>> If we think of EADD as a way of mmap()'ing an enclave file into memory,
>>>>>> would this
>>>>> security_enclave_load() be the same as
>>>>> security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
>>>>> the target is now EPC instead of regular pages?
>>>>>
>>>>> Hmm, that's clever. Although it seems plausible that an LSM would want to
>>>>> allow RX or RWX of a given file page but only in the context of an approved
>>>>> enclave, so I think it should still be its own hook.
>>>>
>>>> What do you mean by "in the context of an approved enclave"? EPC pages are
>>>> *inaccessible* to any software until after EINIT. So it would never be a
>>>> security concern to EADD a page with wrong permissions as long as the enclave
>>>> would be denied eventually by LSM at EINIT.
>>>>
>>>> But I acknowledge the difference between loading a page into regular memory
>>>> vs. into EPC. So it's beneficial to have a separate hook, which if not
>>>> hooked, would pass through to security_mmap_file() by default?
>>>
>>> Mapping the enclave will still go through security_mmap_file(), the extra
>>> security_enclave_load() hook allows the mmap() to use PROT_NONE.
>>>
>>>>> If it's going to be in an arbitrary file, then I think the signature needs to be more
>> like:
>>>>>
>>>>> int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
>> sigstruct_offset,
>>>>> const sgx_sigstruct *sigstruct);
>>>>>
>>>>> So that the LSM still has the opportunity to base its decision on the contents of the
>>>>> SIGSTRUCT. Actually, we need that change regardless.
>>>>
>>>> Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
>>>> a pointer, because the VMA could be looked up using the pointer and the
>>>> offset would then be (pointer - vma->vm_start)?
>>>
>>> VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
>>> said, why does the LSM need the VMA? E.g. why not this?
>>>
>>> int security_enclave_init(struct file *file, struct sgx_sigstruct *sigstruct);
>>>
>>>>>> Loosely speaking, an enclave (including initial contents of all of its pages and
>> their
>>>>> permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision
>> resistant
>>>>> property of SHA-2). So only one is needed for a decision, and either one would lead to
>> the
>>>>> same decision. So I don't see anything making any sense here.
>>>>>>
>>>>>> Theoretically speaking, if LSM can make a decision at EINIT by means of
>>>>> security_enclave_load(), then security_enclave_load() is never needed.
>>>>>>
>>>>>> In practice, I support keeping both because security_enclave_load() can only approve
>> an
>>>>> enumerable set while security_enclave_load() can approve a non-enumerable set of
>> enclaves.
>>>>> Moreover, in order to determine the validity of a MRENCLAVE (as in development of a
>> policy
>>>>> or in creation of a white/black list), system admins will need the audit log produced
>> by
>>>>> security_enclave_load().
>>>>>
>>>>> I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
>>>>> up. Also, security_enclave_load() provides no protection against loading a
>>>>> mishmash of two different enclave files. I see security_enclave_init() as
>>>>> "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
>>>>> grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
>>>>> "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
>>>>> check enclave files for some label."
>>>>
>>>> Sorry for the confusion. I was saying the same thing except that the decision
>>>> of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
>>>> prototype of security_enclave_load(), I think we are on the same page. I made
>>>> the above comment to object to the idea of "require that the sigstruct be
>>>> supplied before any EADD operations so that the maxperm decisions can depend
>>>> on the sigstruct".
>>>
>>> Except that having the sigstruct allows using the sigstruct as the proxy
>>> for the enclave. I think the last big disconnect is that Andy and I want
>>> to tie everything to an enclave-specific file, i.e. sigstruct, while you
>>> are proposing labeling /dev/sgx/enclave. If someone wants to cram several
>>> sigstructs into a single file, so be it, but using /dev/sgx/enclave means
>>> users can't do per-enclave permissions, period.
>>>
>>> What is your objection to working on the sigstruct?
>>>
>>>>>>>> Passing both would allow tying EXECMOD to /dev/sgx/enclave as
>>>>>>>> Cedric wanted (without having to play games and pass
>>>>>>>> /dev/sgx/enclave to security_enclave_load()), but I don't think
>>>>>>>> there's anything fundamentally broken with using .sigstruct for
>>>>>>>> EXECMOD. It requires more verbose labeling, but that's not a bad thing.
>>>>>>>
>>>>>>> The benefit of putting it on .sigstruct is that it can be per-enclave.
>>>>>>>
>>>>>>> As I understand it from Fedora packaging, the way this works on
>>>>>>> distros is generally that a package will include some files and
>>>>>>> their associated labels, and, if the package needs EXECMOD, then the
>>>>>>> files are labeled with EXECMOD and the author of the relevant code might get a dirty
>>>>> look.
>>>>>>>
>>>>>>> This could translate to the author of an exclave that needs RWX
>>>>>>> regions getting a dirty look without leaking this permission into other enclaves.
>>>>>>>
>>>>>>> (In my opinion, the dirty looks are actually the best security
>>>>>>> benefit of the entire concept of LSMs making RWX difficult. A
>>>>>>> sufficiently creative attacker can almost always bypass W^X
>>>>>>> restrictions once they’ve pwned you, but W^X makes it harder to pwn
>>>>>>> you in the first place, and SELinux makes it really obvious when
>>>>>>> packaging a program that doesn’t respect W^X. The upshot is that a
>>>>>>> lot of programs got fixed.)
>>>>>>
>>>>>> I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX,
>> i.e.
>>>>> FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
>>>>>
>>>>> Hmm. If we want to make this distinction, we need something a big richer
>>>>> than my proposed callbacks. A check of the actual mprotect() / mmap()
>>>>> permissions would also be needed. Specifically, allowing MAXPERM=RWX
>>>>> wouldn't imply that PROT_WRITE | PROT_EXEC is allowed.
>>>
>>> Actually, I think we do have everything we need from an LSM perspective.
>>> LSMs just need to understand that sgx_enclave_load() with a NULL vma
>>> implies a transition from RW. For example, SELinux would interpret
>>> sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
>>>
>>> As Cedric mentioned earlier, the host process doesn't necessarily know
>>> which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL, RX)
>>> already has to be invoked at runtime, and when that happens, the kernel
>>> can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
>>>
>>> For simplicity in the kernel and clarity in userspace, it makes sense to
>>> require an explicit ioctl() to add the to-be-EAUG'd range. That just
>>> leaves us wanting an ioctl() to set the post-EACCEPT{COPY} permissions.
>>>
>>> E.g.:
>>>
>>> ioctl(<prefix>_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
>>>
>>> mprotect(addr, size, RW);
>>> ...
>>>
>>> EACCEPTCOPY -> EAUG /* page fault handler */
>>>
>>> ioctl(<prefix>_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
>>>
>>> mprotect(addr, size, RX);
>>>
>>> ...
>>>
>>> And making ACTIVATE_REGION a single-shot per page eliminates the need for
>>> the MAXPERMS concept (see below).
>>>
>>>> If we keep only one MAXPERM, wouldn't this be the current behavior of
>>>> mmap()/mprotect()?
>>>>
>>>> To be a bit more clear, system admin sets MAXPERM upper bound in the form of
>>>> FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a
>>>> process/enclave, if what it requires falls below what's allowed on
>>>> /dev/sgx/enclave, then everything will just work. Otherwise, it fails in the
>>>> form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM here
>>>> applies to "runtime" permissions, while "initial" permissions are taken care
>>>> of by security_enclave_{load|init}. "initial" permissions could be more
>>>> permissive than "runtime" permissions, e.g., RX is still required for initial
>>>> code pages even though system admins could disable dynamically loaded code
>>>> pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial"
>>>> mapping would still have to be done by the driver (to bypass LSM), either via
>>>> a new ioctl or as part of IOC_EINIT.
>>>
>>> Aha!
>>>
>>> Starting with Cedric's assertion that initial permissions can be taken
>>> directly from SECINFO:
>>>
>>> - Initial permissions for *EADD* pages are explicitly handled via
>>> sgx_enclave_load() with the exact SECINFO permissions.
>>>
>>> - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
>>> requires the target EPC page to be RW, and EACCEPT with RO is useless.
>>>
>>> - Runtime permissions break down as follows:
>>> R - N/A, subset of RW (EAUG)
>>> W - N/A, subset of RW (EAUG) and x86 paging can't do W
>>> X - N/A, subset of RX (x86 paging can't do XO)
>>> RW - Handled by EAUG LSM hook (uses RW unconditionally)
>>> WX - N/A, subset of RWX (x86 paging can't do WX)
>>> RX - Handled by ACTIVATE_REGION
>>> RWX - Handled by ACTIVATE_REGION
>>>
>>> In other words, if we define the SGX -> LSM calls as follows (minus the
>>> file pointer and other params for brevity):
>>>
>>> - <prefix>_ACTIVATE_REGION(vma, perms) -> sgx_enclave_load(NULL, perms)
>>>
>>> - <prefix>_ADD_REGION(vma) -> sgx_enclave_load(vma, SECINFO.perms)
>>>
>>> - <prefix>_ADD_REGION(NULL) -> sgx_enclave_load(NULL, RW)
>>>
>>> then SGX and LSMs have all the information and hooks needed. The catch
>>> is that the LSM semantics of sgx_enclave_load(..., RW) would need to be
>>> different than normal shared memory, e.g. FILE__WRITE should *not* be
>>> required, but that's ok since it's an SGX specific hook. And if for some
>>> reason an LSM wanted to gate access to EAUG *without* FILE__EXECMOD, it'd
>>> have the necessary information to do so.
>>
>> Assuming that sgx_enclave_load() is a LSM hook (probably named
>> security_enclave_load() instead), then:
>>
>> a) Does the sigstruct file get passed to this hook in every case, even
>> when vma is NULL? I think the answer is yes, just want to confirm.
>
> I'm confused.
I'm finding it difficult to follow as well, so my questions are just an
attempt to understand the latest model.
> In the case of EADD (non-NULL vma), are we passing both vma and sigstruct file? If so, which file dictates allowed permissions, vma->vm_file or sigstruct, or both???
My impression was that they were going to pass both, but the sigstruct
file is the target of permission checks. The vma if non-NULL would be
used to determine whether PROT_EXEC is being added or was already
present and whether EXECMOD needs to be checked (i.e. copy-on-write has
occurred and PROT_EXEC is being added).
> In the case of EAUG (NULL vma), all other parameters are constant for any given enclave. Then why do we call this same hook for every region added, assuming the hook will return the same value everytime anyway?
Yes, I was wondering about that as well.
> And it looks like ACTIVATE_REGION is needed only because the proposed security_enclave_load() would base its decision on the sigstruct file. An alternative is to base that decision on /dev/sgx/enclave. Of course the former has finer granularity but is that really necessary? From security perspective, only the weakest link matters. FILE__EXECMOD on a regular shared object could allow exploits of all bugs throughout the host process because code within that shared object is modifiable by not only itself but also any code within that same process. In contrast, FILE__EXECMOD on /dev/sgx/enclave only allows enclaves to modify themselves. They cannot modify each other, neither can "untrusted" code outside of enclaves modify any of them. So it doesn't look like a weaker link to me. Moreover, requiring FILE__EXECMOD on sigstruct means it could be used as a target buffer for code injection attacks. IMHO that *lowers* the security of the whole process.
This is partly why I suggested separate ENCLAVE__EXECMOD and other
checks below, so we can distinguish between FILE__EXECMOD versus
ENCLAVE__EXECMOD on the sigstruct file. If using /dev/sgx/enclave as
the target, then we don't need a separate permission per se but we lose
the per-sigstruct granularity.
>
>>
>> b) Should we use a different hook for ACTIVATE_REGION than for
>> ADD_REGION or is the distinction between them irrelevant/unnecessary
>> from an access control point of view? At present LSM/SELinux won't be
>> able to distinguish ACTIVATE_REGION(vma, RW) from ADD_REGION(NULL) above
>> since they will both invoke the same hook with the same arguments IIUC.
>> Does it matter? It's ok if the answer is no, just want to confirm.
>>
>> c) Is there still also a separate security_enclave_init() hook that will
>> be called, and if so, how does it differ and when is it called relative
>> to security_enclave_load()?
>
> I think security_enclave_init() will always be useful, as it offers a way for LSM to implement whitelisting/blacklisting. Of course an LSM module like SELinux can look into the backing inode too. I think the hook should have a signature like:
>
> int security_enclave_init(struct sgx_sigstruct __user *sigstruct);
>
> An LSM that cares about the backing file could look into vm_file of the VMA covering the buffer, while an LSM that cares the sigstruct itself (e.g. signing key) could just look into the buffer.
I'm not a fan of passing __user pointers to LSM hooks. And it certainly
shouldn't be looking at the buffer since it could change between the
time of check and time of use.
>
>>
>> d) What checks were you envisioning each of these calls making?
>>
>> With the separate security_enclave_*() hooks, we could define and use
>> new ENCLAVE__* permissions, e.g. ENCLAVE__LOAD, ENCLAVE__INIT,
>> ENCLAVE__EXECUTE, ENCLAVE__EXECMEM, ENCLAVE__EXECMOD, if we want to
>> distinguish these operations from regular file mmap/mprotect operations.
>
> I'm not sure if these ENCLAVE__* flags are an overkill, unless we want to enforce an enclave file cannot be loaded as a regular shared object or vice versa.
ENCLAVE__LOAD and/or ENCLAVE__INIT would be to support whitelisting of
what enclaves can be loaded/initialized by the process. That's separate
from the W^X discussion. Those permissions would be between the process
and either the sigstruct file or the enclave file (the consensus seemed
to be the sigstruct file as the stronger/more complete binding of the
enclave). We probably only need one of those two permission checks not
both.
ENCLAVE__EXECUTE, ENCLAVE__EXECMEM, ENCLAVE__EXECMOD would allow
distinctions between host process mmap/mprotect PROT_EXEC operations
(which would continue to apply FILE__EXECUTE, PROCESS__EXECMEM, and
FILE__EXECMOD checks if appropriate) and the driver's setting of initial
and runtime permissions (which would apply ENCLAVE__EXECUTE,
ENCLAVE__EXECMEM, and ENCLAVE__EXECMOD checks if appropriate). That's
particularly helpful if we are using the sigstruct or enclave file as
the target of all checks instead of /dev/sgx/enclave, so that we don't
have to allow FILE__EXECUTE or FILE__EXECMOD to the sigstruct file by
the host process.
>
>>
>>>
>>> The userspace changes are fairly minimal:
>>>
>>> - For SGX1, use PROT_NONE for the initial mmap() and refactor ADD_PAGE
>>> to ADD_REGION.
>>>
>>> - For SGX2, do an explicit ADD_REGION on the ranges to be EAUG'd, and an
>>> ACTIVATE_REGION to make a region RX or R (no extra ioctl() required to
>>> keep RW permissions).
>>>
>>> Because ACTIVATE_REGION can only be done once per page, to do *abitrary*
>>> mprotect() transitions, userspace would need to set the added/activated
>>> permissions to be a superset of the transitions, e.g. RW -> RX would
>>> require RWX, but that's a non-issue.
>>>
>>> - For SGX1 it's a nop since it's impossible to change the EPCM
>>> permissions, i.e. the page would need to be RWX regardless.
>>>
>>> - For SGX2, userspace can suck it up and request RWX to do completely
>>> arbitrary transitions (working as intended), or the kernel can support
>>> trimming (removing) pages from an enclave, which would allow userspace
>>> to do "arbitrary" transitions by first removing the page.
>>>
>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-30 14:31 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Christopherson, Sean J, William Roberts,
Andy Lutomirski, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <678a37af-797d-7bd5-a406-32548a270e3d@tycho.nsa.gov>
Hi all-
After an offline discussion with Sean yesterday, here are some updates
to the user API parts of my proposal.
Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
described it because, for SGX2, the enclave loader won't know at load
time whether a given EAUG-ed page will ever be executed. So here's an
update.
First, here are the requrements as I see them, where EXECUTE, EXECMOD,
and EXECMEM could be substituted with other rules at the LSM's
discretion:
- You can create a WX or RWX mapping if and only if you have EXECMEM.
- To create an X mapping of an enclave page that has ever been W, you
need EXECMOD.
- To create an X mapping of an enclave page that came from EADD, you
need EXECUTE on the source file. Optionally, we could also permit
this if you have EXECMOD.
And I have two design proposals. One is static and one is dynamic.
To implement either one, we will probably need a new .may_mprotect vm
operation, and that operation can call an LSM hook. Or we can give
LSMs a way to detect that a given vm_area_struct is an enclave. As I
see it, this is an implementation detail that is certainly solveable.
Static proposal:
EADD takes an execute_intent flag. It calls a new hook:
int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
This hook will fail if execute_intent==true and the caller has neither
EXECUTE, EXECMOD, nor EXECMEM.
EAUG sets execute_intent = false.
EINIT takes a sigstruct pointer. SGX can (when initially upstreamed
or later on once there's demand) call a new hook:
security_enclave_init(struct sigstruct *sigstruct, struct
vm_area_struct *source);
mmap() and mprotect() will require EXECMEM to create WX or RWX
mappings. They will require EXECMOD to create RX or X mappings of an
execute_intent==false page. They require no permissions in the other
cases.
Dynamic proposal:
EADD does not take any special flags. It does something like this internally:
bool execute_intent = true;
int security_enclave_load(struct vm_area_struct *source, bool
*execute_intent);
The implementation of security_enclave_load() may set *execute_intent to false.
The driver records execute_intent after the LSM is done.
mmap() and mprotect() will require EXECMEM to create WX or RWX
mappings. They will require EXECMOD to create RX or X mappings of an
execute_intent==false page. They require no permissions in the other
cases.
A benefit of the static proposal is that audit failures due to a lack
of EXECUTE permission are easy to implement and to understand in the
lods. With the dynamic model, we can only really audit the lack of
EXECMOD or EXECMEM. A benefit of the dynamic model is that we hide
what is arguably a decently large wart from the API.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-30 15:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, Christopherson, Sean J, William Roberts,
Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrWXB9fNNDH7gZxPTx05F78Og6K=ZtAr2aA++BDwY09Wbg@mail.gmail.com>
On 5/30/19 10:31 AM, Andy Lutomirski wrote:
> Hi all-
>
> After an offline discussion with Sean yesterday, here are some updates
> to the user API parts of my proposal.
>
> Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
> described it because, for SGX2, the enclave loader won't know at load
> time whether a given EAUG-ed page will ever be executed. So here's an
> update.
>
> First, here are the requrements as I see them, where EXECUTE, EXECMOD,
> and EXECMEM could be substituted with other rules at the LSM's
> discretion:
>
> - You can create a WX or RWX mapping if and only if you have EXECMEM.
>
> - To create an X mapping of an enclave page that has ever been W, you
> need EXECMOD.
EXECMOD to what file? The enclave file from which the page's content
originated, the sigstruct file, or /dev/sgx/enclave?
> - To create an X mapping of an enclave page that came from EADD, you
> need EXECUTE on the source file. Optionally, we could also permit
> this if you have EXECMOD.
What is the "source file" i.e. the target of the check? Enclave file,
sigstruct file, or /dev/sgx/enclave?
>
> And I have two design proposals. One is static and one is dynamic.
> To implement either one, we will probably need a new .may_mprotect vm
> operation, and that operation can call an LSM hook. Or we can give
> LSMs a way to detect that a given vm_area_struct is an enclave. As I
> see it, this is an implementation detail that is certainly solveable.
>
>
> Static proposal:
>
>
> EADD takes an execute_intent flag. It calls a new hook:
>
> int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
>
> This hook will fail if execute_intent==true and the caller has neither
> EXECUTE, EXECMOD, nor EXECMEM.
EADD execute_intent flag is originally provided by whom (userspace or
driver) on what basis? Which file is referenced by source->vm_file? Why
trigger all three checks up front versus only checking if needed? Won't
this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will
need to be dontaudit'd? What if there is a mismatch between
execute_intent and the initial permissions?
>
> EAUG sets execute_intent = false.
>
> EINIT takes a sigstruct pointer. SGX can (when initially upstreamed
> or later on once there's demand) call a new hook:
>
> security_enclave_init(struct sigstruct *sigstruct, struct
> vm_area_struct *source);
Is struct sigstruct the same as struct sgx_sigstruct in the current
patches (i.e. just the sigstruct data, no file)? What file is
referenced by source->vm_file (the sigstruct or the enclave or
/dev/sgx/enclave)? Is this hook only for enforcing a whitelist on what
enclaves can be loaded? What is the target of the check?
> mmap() and mprotect() will require EXECMEM to create WX or RWX
> mappings. They will require EXECMOD to create RX or X mappings of an
> execute_intent==false page. They require no permissions in the other
> cases.
Does this occur for both setting initial permissions and runtime
permissions or just runtime? Both userspace- and driver-initiated
mmap/mprotect operations or just userspace-initiated ones? Does the
driver use interfaces that call the mmap/mprotect hooks or lower level
functions?
>
>
> Dynamic proposal:
>
>
> EADD does not take any special flags. It does something like this internally:
>
> bool execute_intent = true;
> int security_enclave_load(struct vm_area_struct *source, bool
> *execute_intent);
>
> The implementation of security_enclave_load() may set *execute_intent to false.
> The driver records execute_intent after the LSM is done.
On what basis does LSM decide whether to set *execute_intent? If the
process lacks all three permissions? What if there is a mismatch with
the initial permissions?
>
> mmap() and mprotect() will require EXECMEM to create WX or RWX
> mappings. They will require EXECMOD to create RX or X mappings of an
> execute_intent==false page. They require no permissions in the other
> cases.
>
>
>
> A benefit of the static proposal is that audit failures due to a lack
> of EXECUTE permission are easy to implement and to understand in the
> lods. With the dynamic model, we can only really audit the lack of
> EXECMOD or EXECMEM. A benefit of the dynamic model is that we hide
> what is arguably a decently large wart from the API.
>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-30 16:14 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Xing, Cedric, Christopherson, Sean J,
William Roberts, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <c1135352-0b5e-4694-b1a9-105876095877@tycho.nsa.gov>
On Thu, May 30, 2019 at 8:04 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 5/30/19 10:31 AM, Andy Lutomirski wrote:
> > Hi all-
> >
> > After an offline discussion with Sean yesterday, here are some updates
> > to the user API parts of my proposal.
> >
> > Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
> > described it because, for SGX2, the enclave loader won't know at load
> > time whether a given EAUG-ed page will ever be executed. So here's an
> > update.
> >
> > First, here are the requrements as I see them, where EXECUTE, EXECMOD,
> > and EXECMEM could be substituted with other rules at the LSM's
> > discretion:
> >
> > - You can create a WX or RWX mapping if and only if you have EXECMEM.
> >
> > - To create an X mapping of an enclave page that has ever been W, you
> > need EXECMOD.
>
> EXECMOD to what file? The enclave file from which the page's content
> originated, the sigstruct file, or /dev/sgx/enclave?
I leave that decision to you :) The user should need permission to do
an execmod thing on an enclave, however that wants to be encoded.
>
> > - To create an X mapping of an enclave page that came from EADD, you
> > need EXECUTE on the source file. Optionally, we could also permit
> > this if you have EXECMOD.
>
> What is the "source file" i.e. the target of the check? Enclave file,
> sigstruct file, or /dev/sgx/enclave?
Enclave file -- that is, the file backing the vma from which the data is loaded.
>
> >
> > And I have two design proposals. One is static and one is dynamic.
> > To implement either one, we will probably need a new .may_mprotect vm
> > operation, and that operation can call an LSM hook. Or we can give
> > LSMs a way to detect that a given vm_area_struct is an enclave. As I
> > see it, this is an implementation detail that is certainly solveable.
> >
> >
> > Static proposal:
> >
> >
> > EADD takes an execute_intent flag. It calls a new hook:
> >
> > int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
> >
> > This hook will fail if execute_intent==true and the caller has neither
> > EXECUTE, EXECMOD, nor EXECMEM.
>
> EADD execute_intent flag is originally provided by whom (userspace or
> driver) on what basis? Which file is referenced by source->vm_file? Why
> trigger all three checks up front versus only checking if needed? Won't
> this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will
> need to be dontaudit'd? What if there is a mismatch between
> execute_intent and the initial permissions?
It's provided by userspace based on whether it thinks the data in
question is enclave code. source->vm_file is the file from which the
code is being loaded. I'm assuming that the user code will only set
excute_intent ==true if it actually wants to execute the code, so, if
there's a denial, it will be fatal. The normal case will be that the
request will be granted on the basis of EXECUTE.
>
> >
> > EAUG sets execute_intent = false.
> >
> > EINIT takes a sigstruct pointer. SGX can (when initially upstreamed
> > or later on once there's demand) call a new hook:
> >
> > security_enclave_init(struct sigstruct *sigstruct, struct
> > vm_area_struct *source);
>
> Is struct sigstruct the same as struct sgx_sigstruct in the current
> patches (i.e. just the sigstruct data, no file)? What file is
> referenced by source->vm_file (the sigstruct or the enclave or
> /dev/sgx/enclave)? Is this hook only for enforcing a whitelist on what
> enclaves can be loaded? What is the target of the check?
sigstruct is just the data. source->vm_file is the file from which
the sigstruct came, which could be a .sigstruct file or could be the
main executable or a DSO that contains an embedded enclave. The
sigstruct data is there so that an LSM (not necessarily SELinux) could
check MRENCLAVE or MRSIGNER, and the source is there so that the
file's label can be checked.
>
> > mmap() and mprotect() will require EXECMEM to create WX or RWX
> > mappings. They will require EXECMOD to create RX or X mappings of an
> > execute_intent==false page. They require no permissions in the other
> > cases.
>
> Does this occur for both setting initial permissions and runtime
> permissions or just runtime? Both userspace- and driver-initiated
> mmap/mprotect operations or just userspace-initiated ones? Does the
> driver use interfaces that call the mmap/mprotect hooks or lower level
> functions?
These would occur for any mmap(), mprotect(), or ioctl() that changes
VMA permissions. Actually arranging for the hooks to be called is an
implementation detail that might require a new .mprotect vm_operation.
As an alternative, security_enclave_init() or similar could supply
may_execmod and may_execmem flags to the driver, and the driver could
do these checks on its own when mmap() and mprotect() happen without a
new LSM callback.
>
> >
> >
> > Dynamic proposal:
> >
> >
> > EADD does not take any special flags. It does something like this internally:
> >
> > bool execute_intent = true;
> > int security_enclave_load(struct vm_area_struct *source, bool
> > *execute_intent);
> >
> > The implementation of security_enclave_load() may set *execute_intent to false.
> > The driver records execute_intent after the LSM is done.
>
> On what basis does LSM decide whether to set *execute_intent? If the
> process lacks all three permissions? What if there is a mismatch with
> the initial permissions?
>
I think it would set *execute_intent=false if the process lacks
EXECUTE on source->vm_file. I'm not sure any more complexity is
required. If the enclave has EXECMOD, then it will still work on the
basis of the mmap/mprotect rules.
> >
> > mmap() and mprotect() will require EXECMEM to create WX or RWX
> > mappings. They will require EXECMOD to create RX or X mappings of an
> > execute_intent==false page. They require no permissions in the other
> > cases.
> >
> >
> >
> > A benefit of the static proposal is that audit failures due to a lack
> > of EXECUTE permission are easy to implement and to understand in the
> > lods. With the dynamic model, we can only really audit the lack of
> > EXECMOD or EXECMEM. A benefit of the dynamic model is that we hide
> > what is arguably a decently large wart from the API.
> >
>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-30 17:21 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Stephen Smalley, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654EB439@ORSMSX116.amr.corp.intel.com>
On Wed, May 29, 2019 at 10:38:06PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Tuesday, May 28, 2019 2:41 PM
> >
> > On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> > > On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > >
> > > > Actually, I think we do have everything we need from an LSM perspective.
> > > > LSMs just need to understand that sgx_enclave_load() with a NULL vma
> > > > implies a transition from RW. For example, SELinux would interpret
> > > > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
> > >
> > > You lost me here. What operation triggers this callback? And
> > > wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> > > or just some fresh executable zero bytes?
> >
> > An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
> > For all intents and purposes, the EAUG'd page must start RW. Maybe a better way to phrase
> > it is that at some point the page must be writable to have any value whatsover.
> > EACCEPTCOPY explicitly requires the page to be at least RW. EACCEPT technically doesn't
> > require RW, but a RO or RX zero page is useless. Userspace could still EACCEPT with RO or
> > RX, but SGX would assume a minimum of RW for the purposes of the LSM check.
>
> Why is an explicit ioctl() necessary after EACCEPTCOPY? Or why is mprotect() not sufficient?
Ignore this, I was trying to avoid having to add a vm_ops mprotect(),
which Andy pointed out was silly.
> > In theory, it's still your MAXPERM model, but with the unnecessary states removed and the
> > others enforced/handled by the natural SGX transitions instead of explictly in ioctls.
> > Underneath the hood the SGX driver would still need to track the MAXPERM.
>
> What are the "unnecessary states" removed?
Andy proposed taking full RWX in MAXPERMs, but really we only need "can
writes ever happen to this page", as that allows the SGX driver to avoid
having to track if a page has been mapped PROT_WRITE by any VMA in any
process.
> I'm not sure understand the proposal fully. The whole thing looks to me like
> the driver is undertaking things that should/would otherwise be done by
> mmap()/mprotect() syscalls. It also imposes unnecessary restrictions on user
> mode code, such as mmap(PROT_NONE), ACTIVATE_REGION can be called only once,
> etc. What'd happen if ACTIVATE_REGION is called with a range spanning
> multiple/partial VMAs? What'd happen if an enclave was unmapped than mapped
> again? I'd say the proposal is unintuitive at least.
>
> In theory, if the driver can keep track of MAXPERM for all pages within an
> enclave, then it could fail mmap() if the requested prot conflicts with any
> page's MAXPERM within that range. Otherwise, MAXPERM could be copied into
> VM_MAY* flags then mprotect() will just follow through. Wouldn't that be a
> much simpler and more intuitive approach?
Ignore all this, again I was trying to avoid hooking mprotect().
^ permalink raw reply
* [PATCH 01/10] keys: sparse: Fix key_fs[ug]id_changed() [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings
Cc: James Morris, dhowells, linux-security-module, linux-kernel,
ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
accesses of tsk->cred, which is marked __rcu.
Fix this by passing the new cred struct to these functions from
commit_creds() rather than the task pointer.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
include/linux/key.h | 8 ++++----
kernel/cred.c | 4 ++--
security/keys/process_keys.c | 22 ++++++++++------------
3 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..1f09aad1c98c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -402,8 +402,8 @@ extern struct ctl_table key_sysctls[];
* the userspace interface
*/
extern int install_thread_keyring_to_cred(struct cred *cred);
-extern void key_fsuid_changed(struct task_struct *tsk);
-extern void key_fsgid_changed(struct task_struct *tsk);
+extern void key_fsuid_changed(struct cred *new_cred);
+extern void key_fsgid_changed(struct cred *new_cred);
extern void key_init(void);
#else /* CONFIG_KEYS */
@@ -418,8 +418,8 @@ extern void key_init(void);
#define make_key_ref(k, p) NULL
#define key_ref_to_ptr(k) NULL
#define is_key_possessed(k) 0
-#define key_fsuid_changed(t) do { } while(0)
-#define key_fsgid_changed(t) do { } while(0)
+#define key_fsuid_changed(c) do { } while(0)
+#define key_fsgid_changed(c) do { } while(0)
#define key_init() do { } while(0)
#endif /* CONFIG_KEYS */
diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..3bd40de9e192 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -455,9 +455,9 @@ int commit_creds(struct cred *new)
/* alter the thread keyring */
if (!uid_eq(new->fsuid, old->fsuid))
- key_fsuid_changed(task);
+ key_fsuid_changed(new);
if (!gid_eq(new->fsgid, old->fsgid))
- key_fsgid_changed(task);
+ key_fsgid_changed(new);
/* do it
* RLIMIT_NPROC limits on user->processes have already been checked
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..ba5d3172cafe 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -293,28 +293,26 @@ static int install_session_keyring(struct key *keyring)
/*
* Handle the fsuid changing.
*/
-void key_fsuid_changed(struct task_struct *tsk)
+void key_fsuid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->uid = tsk->cred->fsuid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->uid = new_cred->fsuid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
/*
* Handle the fsgid changing.
*/
-void key_fsgid_changed(struct task_struct *tsk)
+void key_fsgid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->gid = tsk->cred->fsgid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->gid = new_cred->fsgid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
^ permalink raw reply related
* [PATCH 00/10] keys: Miscellany [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings
Cc: Mat Martineau, Eric Biggers, James Morris, dhowells,
linux-security-module, linux-kernel, ebiggers
Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window:
(1) Fix a bunch of warnings from sparse, including missing RCU bits and
kdoc-function argument mismatches
(2) Implement a keyctl to allow a key to be moved from one keyring to
another, with the option of prohibiting key replacement in the
destination keyring.
(3) Grant Link permission to possessors of request_key_auth tokens so that
upcall servicing daemons can more easily arrange things such that only
the necessary auth key is passed to the actual service program, and
not all the auth keys a daemon might possesss.
(4) Improvement in lookup_user_key().
(5) Implement a keyctl to allow keyrings subsystem capabilities to be
queried.
The patches can be found on the following branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-misc
The keyutils next branch has commits to make available, document and test
the move-key and capabilities code:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git/log/?h=next
Changes:
(*) Fixed lock ordering bug in KEYCTL_MOVE.
(*) Added improvement patch from Eric.
(*) Added capabilities patch.
David
---
David Howells (9):
keys: sparse: Fix key_fs[ug]id_changed()
keys: sparse: Fix incorrect RCU accesses
keys: sparse: Fix kdoc mismatches
keys: Change keyring_serialise_link_sem to a mutex
keys: Break bits out of key_unlink()
keys: Hoist locking out of __key_link_begin()
keys: Add a keyctl to move a key between keyrings
keys: Grant Link permission to possessers of request_key auth keys
keys: Add capability-checking keyctl function
Eric Biggers (1):
KEYS: reuse keyring_index_key::desc_len in lookup_user_key()
Documentation/security/keys/core.rst | 21 +++
include/linux/key.h | 13 +-
include/uapi/linux/keyctl.h | 17 ++
kernel/cred.c | 4
security/keys/compat.c | 6 +
security/keys/internal.h | 7 +
security/keys/key.c | 23 ++-
security/keys/keyctl.c | 92 +++++++++++
security/keys/keyring.c | 275 +++++++++++++++++++++++++++-------
security/keys/process_keys.c | 26 +--
security/keys/request_key.c | 7 +
security/keys/request_key_auth.c | 4
12 files changed, 413 insertions(+), 82 deletions(-)
^ permalink raw reply
* [PATCH 02/10] keys: sparse: Fix incorrect RCU accesses [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings
Cc: James Morris, dhowells, linux-security-module, linux-kernel,
ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Fix a pair of accesses that should be using RCU protection.
rcu_dereference_protected() is needed to access task_struct::real_parent.
current_cred() should be used to access current->cred.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/keyctl.c | 3 ++-
security/keys/request_key_auth.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..0f947bcbad46 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1524,7 +1524,8 @@ long keyctl_session_to_parent(void)
ret = -EPERM;
oldwork = NULL;
- parent = me->real_parent;
+ parent = rcu_dereference_protected(me->real_parent,
+ lockdep_is_held(&tasklist_lock));
/* the parent mustn't be init and mustn't be a kernel thread */
if (parent->pid <= 1 || !parent->mm)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..572c7a60473a 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -152,7 +152,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
struct key *dest_keyring)
{
struct request_key_auth *rka, *irka;
- const struct cred *cred = current->cred;
+ const struct cred *cred = current_cred();
struct key *authkey = NULL;
char desc[20];
int ret = -ENOMEM;
^ permalink raw reply related
* [PATCH 03/10] keys: sparse: Fix kdoc mismatches [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings
Cc: James Morris, Mat Martineau, dhowells, linux-security-module,
linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Fix some kdoc argument description mismatches reported by sparse and give
keyring_restrict() a description.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
security/keys/keyring.c | 10 +++++++---
security/keys/request_key.c | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..5b218b270598 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -520,7 +520,7 @@ EXPORT_SYMBOL(keyring_alloc);
* @keyring: The keyring being added to.
* @type: The type of key being added.
* @payload: The payload of the key intended to be added.
- * @data: Additional data for evaluating restriction.
+ * @restriction_key: Keys providing additional data for evaluating restriction.
*
* Reject the addition of any links to a keyring. It can be overridden by
* passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
@@ -976,9 +976,13 @@ static bool keyring_detect_restriction_cycle(const struct key *dest_keyring,
/**
* keyring_restrict - Look up and apply a restriction to a keyring
- *
- * @keyring: The keyring to be restricted
+ * @keyring_ref: The keyring to be restricted
+ * @type: The key type that will provide the restriction checker.
* @restriction: The restriction options to apply to the keyring
+ *
+ * Look up a keyring and apply a restriction to it. The restriction is managed
+ * by the specific key type, but can be configured by the options specified in
+ * the restriction string.
*/
int keyring_restrict(key_ref_t keyring_ref, const char *type,
const char *restriction)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..1f234b019437 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -24,7 +24,7 @@
/**
* complete_request_key - Complete the construction of a key.
- * @auth_key: The authorisation key.
+ * @authkey: The authorisation key.
* @error: The success or failute of the construction.
*
* Complete the attempt to construct a key. The key will be negated
^ permalink raw reply related
* [PATCH 04/10] keys: Change keyring_serialise_link_sem to a mutex [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Change keyring_serialise_link_sem to a mutex as it's only ever
write-locked.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5b218b270598..ca6694ba1773 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -100,7 +100,7 @@ EXPORT_SYMBOL(key_type_keyring);
* Semaphore to serialise link/link calls to prevent two link calls in parallel
* introducing a cycle.
*/
-static DECLARE_RWSEM(keyring_serialise_link_sem);
+static DEFINE_MUTEX(keyring_serialise_link_lock);
/*
* Publish the name of a keyring so that it can be found by name (if it has
@@ -1206,7 +1206,7 @@ int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
__acquires(&keyring->sem)
- __acquires(&keyring_serialise_link_sem)
+ __acquires(&keyring_serialise_link_lock)
{
struct assoc_array_edit *edit;
int ret;
@@ -1228,7 +1228,7 @@ int __key_link_begin(struct key *keyring,
/* serialise link/link calls to prevent parallel calls causing a cycle
* when linking two keyring in opposite orders */
if (index_key->type == &key_type_keyring)
- down_write(&keyring_serialise_link_sem);
+ mutex_lock(&keyring_serialise_link_lock);
/* Create an edit script that will insert/replace the key in the
* keyring tree.
@@ -1260,7 +1260,7 @@ int __key_link_begin(struct key *keyring,
assoc_array_cancel_edit(edit);
error_sem:
if (index_key->type == &key_type_keyring)
- up_write(&keyring_serialise_link_sem);
+ mutex_unlock(&keyring_serialise_link_lock);
error_krsem:
up_write(&keyring->sem);
kleave(" = %d", ret);
@@ -1307,13 +1307,13 @@ void __key_link_end(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit *edit)
__releases(&keyring->sem)
- __releases(&keyring_serialise_link_sem)
+ __releases(&keyring_serialise_link_lock)
{
BUG_ON(index_key->type == NULL);
kenter("%d,%s,", keyring->serial, index_key->type->name);
if (index_key->type == &key_type_keyring)
- up_write(&keyring_serialise_link_sem);
+ mutex_unlock(&keyring_serialise_link_lock);
if (edit) {
if (!edit->dead_leaf) {
^ permalink raw reply related
* [PATCH 05/10] keys: Break bits out of key_unlink() [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Break bits out of key_unlink() into helper functions so that they can be
used in implementing key_move().
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 86 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 65 insertions(+), 21 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index ca6694ba1773..2f91b9a699ef 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1382,6 +1382,63 @@ int key_link(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_link);
+/*
+ * Lock a keyring for unlink.
+ */
+static int __key_unlink_lock(struct key *keyring)
+ __acquires(&keyring->sem)
+{
+ if (keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ down_write(&keyring->sem);
+ return 0;
+}
+
+/*
+ * Begin the process of unlinking a key from a keyring.
+ */
+static int __key_unlink_begin(struct key *keyring, struct key *key,
+ struct assoc_array_edit **_edit)
+{
+ struct assoc_array_edit *edit;
+
+ edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
+ &key->index_key);
+ if (IS_ERR(edit))
+ return PTR_ERR(edit);
+
+ if (!edit)
+ return -ENOENT;
+
+ *_edit = edit;
+ return 0;
+}
+
+/*
+ * Apply an unlink change.
+ */
+static void __key_unlink(struct key *keyring, struct key *key,
+ struct assoc_array_edit **_edit)
+{
+ assoc_array_apply_edit(*_edit);
+ *_edit = NULL;
+ key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
+}
+
+/*
+ * Finish unlinking a key from to a keyring.
+ */
+static void __key_unlink_end(struct key *keyring,
+ struct key *key,
+ struct assoc_array_edit *edit)
+ __releases(&keyring->sem)
+{
+ if (edit)
+ assoc_array_cancel_edit(edit);
+ up_write(&keyring->sem);
+}
+
/**
* key_unlink - Unlink the first link to a key from a keyring.
* @keyring: The keyring to remove the link from.
@@ -1401,33 +1458,20 @@ EXPORT_SYMBOL(key_link);
*/
int key_unlink(struct key *keyring, struct key *key)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret;
key_check(keyring);
key_check(key);
- if (keyring->type != &key_type_keyring)
- return -ENOTDIR;
-
- down_write(&keyring->sem);
-
- edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
- &key->index_key);
- if (IS_ERR(edit)) {
- ret = PTR_ERR(edit);
- goto error;
- }
- ret = -ENOENT;
- if (edit == NULL)
- goto error;
-
- assoc_array_apply_edit(edit);
- key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
- ret = 0;
+ ret = __key_unlink_lock(keyring);
+ if (ret < 0)
+ return ret;
-error:
- up_write(&keyring->sem);
+ ret = __key_unlink_begin(keyring, key, &edit);
+ if (ret == 0)
+ __key_unlink(keyring, key, &edit);
+ __key_unlink_end(keyring, key, edit);
return ret;
}
EXPORT_SYMBOL(key_unlink);
^ permalink raw reply related
* [PATCH 06/10] keys: Hoist locking out of __key_link_begin() [ver #2]
From: David Howells @ 2019-05-30 17:25 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Hoist the locking of out of __key_link_begin() and into its callers. This
is necessary to allow the upcoming key_move() operation to correctly order
taking of the source keyring semaphore, the destination keyring semaphore
and the keyring serialisation lock.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/internal.h | 2 +
security/keys/key.c | 23 +++++++++++--
security/keys/keyring.c | 77 +++++++++++++++++++++++++------------------
security/keys/request_key.c | 5 +++
4 files changed, 71 insertions(+), 36 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..25cdd0cbdc06 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,6 +93,8 @@ extern wait_queue_head_t request_key_conswq;
extern struct key_type *key_type_lookup(const char *type);
extern void key_type_put(struct key_type *ktype);
+extern int __key_link_lock(struct key *keyring,
+ const struct keyring_index_key *index_key);
extern int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..591b88f9a51d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -515,10 +515,14 @@ int key_instantiate_and_link(struct key *key,
}
if (keyring) {
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_lock(keyring, &key->index_key);
if (ret < 0)
goto error;
+ ret = __key_link_begin(keyring, &key->index_key, &edit);
+ if (ret < 0)
+ goto error_link_end;
+
if (keyring->restrict_link && keyring->restrict_link->check) {
struct key_restriction *keyres = keyring->restrict_link;
@@ -570,7 +574,7 @@ int key_reject_and_link(struct key *key,
struct key *keyring,
struct key *authkey)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret, awaken, link_ret = 0;
key_check(key);
@@ -583,7 +587,12 @@ int key_reject_and_link(struct key *key,
if (keyring->restrict_link)
return -EPERM;
- link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ link_ret = __key_link_lock(keyring, &key->index_key);
+ if (link_ret == 0) {
+ link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ if (link_ret < 0)
+ __key_link_end(keyring, &key->index_key, edit);
+ }
}
mutex_lock(&key_construction_mutex);
@@ -860,12 +869,18 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
index_key.desc_len = strlen(index_key.description);
- ret = __key_link_begin(keyring, &index_key, &edit);
+ ret = __key_link_lock(keyring, &index_key);
if (ret < 0) {
key_ref = ERR_PTR(ret);
goto error_free_prep;
}
+ ret = __key_link_begin(keyring, &index_key, &edit);
+ if (ret < 0) {
+ key_ref = ERR_PTR(ret);
+ goto error_link_end;
+ }
+
if (restrict_link && restrict_link->check) {
ret = restrict_link->check(keyring, index_key.type,
&prep.payload, restrict_link->key);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2f91b9a699ef..8838c300ea7b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1199,14 +1199,34 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
return PTR_ERR(ctx.result) == -EAGAIN ? 0 : PTR_ERR(ctx.result);
}
+/*
+ * Lock keyring for link.
+ */
+int __key_link_lock(struct key *keyring,
+ const struct keyring_index_key *index_key)
+ __acquires(&keyring->sem)
+ __acquires(&keyring_serialise_link_lock)
+{
+ if (keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ down_write(&keyring->sem);
+
+ /* Serialise link/link calls to prevent parallel calls causing a cycle
+ * when linking two keyring in opposite orders.
+ */
+ if (index_key->type == &key_type_keyring)
+ mutex_lock(&keyring_serialise_link_lock);
+
+ return 0;
+}
+
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
- __acquires(&keyring->sem)
- __acquires(&keyring_serialise_link_lock)
{
struct assoc_array_edit *edit;
int ret;
@@ -1216,19 +1236,9 @@ int __key_link_begin(struct key *keyring,
BUG_ON(index_key->desc_len == 0);
- if (keyring->type != &key_type_keyring)
- return -ENOTDIR;
-
- down_write(&keyring->sem);
-
ret = -EKEYREVOKED;
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
- goto error_krsem;
-
- /* serialise link/link calls to prevent parallel calls causing a cycle
- * when linking two keyring in opposite orders */
- if (index_key->type == &key_type_keyring)
- mutex_lock(&keyring_serialise_link_lock);
+ goto error;
/* Create an edit script that will insert/replace the key in the
* keyring tree.
@@ -1239,7 +1249,7 @@ int __key_link_begin(struct key *keyring,
NULL);
if (IS_ERR(edit)) {
ret = PTR_ERR(edit);
- goto error_sem;
+ goto error;
}
/* If we're not replacing a link in-place then we're going to need some
@@ -1258,11 +1268,7 @@ int __key_link_begin(struct key *keyring,
error_cancel:
assoc_array_cancel_edit(edit);
-error_sem:
- if (index_key->type == &key_type_keyring)
- mutex_unlock(&keyring_serialise_link_lock);
-error_krsem:
- up_write(&keyring->sem);
+error:
kleave(" = %d", ret);
return ret;
}
@@ -1312,9 +1318,6 @@ void __key_link_end(struct key *keyring,
BUG_ON(index_key->type == NULL);
kenter("%d,%s,", keyring->serial, index_key->type->name);
- if (index_key->type == &key_type_keyring)
- mutex_unlock(&keyring_serialise_link_lock);
-
if (edit) {
if (!edit->dead_leaf) {
key_payload_reserve(keyring,
@@ -1323,6 +1326,9 @@ void __key_link_end(struct key *keyring,
assoc_array_cancel_edit(edit);
}
up_write(&keyring->sem);
+
+ if (index_key->type == &key_type_keyring)
+ mutex_unlock(&keyring_serialise_link_lock);
}
/*
@@ -1358,7 +1364,7 @@ static int __key_link_check_restriction(struct key *keyring, struct key *key)
*/
int key_link(struct key *keyring, struct key *key)
{
- struct assoc_array_edit *edit;
+ struct assoc_array_edit *edit = NULL;
int ret;
kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
@@ -1366,17 +1372,24 @@ int key_link(struct key *keyring, struct key *key)
key_check(keyring);
key_check(key);
+ ret = __key_link_lock(keyring, &key->index_key);
+ if (ret < 0)
+ goto error;
+
ret = __key_link_begin(keyring, &key->index_key, &edit);
- if (ret == 0) {
- kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
- ret = __key_link_check_restriction(keyring, key);
- if (ret == 0)
- ret = __key_link_check_live_key(keyring, key);
- if (ret == 0)
- __key_link(key, &edit);
- __key_link_end(keyring, &key->index_key, edit);
- }
+ if (ret < 0)
+ goto error_end;
+
+ kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
+ ret = __key_link_check_restriction(keyring, key);
+ if (ret == 0)
+ ret = __key_link_check_live_key(keyring, key);
+ if (ret == 0)
+ __key_link(key, &edit);
+error_end:
+ __key_link_end(keyring, &key->index_key, edit);
+error:
kleave(" = %d {%d,%d}", ret, keyring->serial, refcount_read(&keyring->usage));
return ret;
}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..232505d591dd 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -372,6 +372,9 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
if (dest_keyring) {
+ ret = __key_link_lock(dest_keyring, &ctx->index_key);
+ if (ret < 0)
+ goto link_lock_failed;
ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
if (ret < 0)
goto link_prealloc_failed;
@@ -423,6 +426,8 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
return ret;
link_prealloc_failed:
+ __key_link_end(dest_keyring, &ctx->index_key, edit);
+link_lock_failed:
mutex_unlock(&user->cons_lock);
key_put(key);
kleave(" = %d [prelink]", ret);
^ permalink raw reply related
* [PATCH 07/10] keys: Add a keyctl to move a key between keyrings [ver #2]
From: David Howells @ 2019-05-30 17:26 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Add a keyctl to atomically move a link to a key from one keyring to
another. The key must exist in "from" keyring and a flag can be given to
cause the operation to fail if there's a matching key already in the "to"
keyring.
This can be done with:
keyctl(KEYCTL_MOVE,
key_serial_t key,
key_serial_t from_keyring,
key_serial_t to_keyring,
unsigned int flags);
The key being moved must grant Link permission and both keyrings must grant
Write permission.
flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
displacement of a matching key from the "to" keyring.
Signed-off-by: David Howells <dhowells@redhat.com>
---
Documentation/security/keys/core.rst | 21 +++++++
include/linux/key.h | 5 ++
include/uapi/linux/keyctl.h | 3 +
security/keys/compat.c | 3 +
security/keys/internal.h | 3 +
security/keys/keyctl.c | 52 ++++++++++++++++
security/keys/keyring.c | 108 ++++++++++++++++++++++++++++++++++
7 files changed, 195 insertions(+)
diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 9521c4207f01..823d29bf44f7 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -577,6 +577,27 @@ The keyctl syscall functions are:
added.
+ * Move a key from one keyring to another::
+
+ long keyctl(KEYCTL_MOVE,
+ key_serial_t id,
+ key_serial_t from_ring_id,
+ key_serial_t to_ring_id,
+ unsigned int flags);
+
+ Move the key specified by "id" from the keyring specified by
+ "from_ring_id" to the keyring specified by "to_ring_id". If the two
+ keyrings are the same, nothing is done.
+
+ "flags" can have KEYCTL_MOVE_EXCL set in it to cause the operation to fail
+ with EEXIST if a matching key exists in the destination keyring, otherwise
+ such a key will be replaced.
+
+ A process must have link permission on the key for this function to be
+ successful and write permission on both keyrings. Any errors that can
+ occur from KEYCTL_LINK also apply on the destination keyring here.
+
+
* Unlink a key or keyring from another keyring::
long keyctl(KEYCTL_UNLINK, key_serial_t keyring, key_serial_t key);
diff --git a/include/linux/key.h b/include/linux/key.h
index 1f09aad1c98c..612e1cf84049 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -310,6 +310,11 @@ extern int key_update(key_ref_t key,
extern int key_link(struct key *keyring,
struct key *key);
+extern int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags);
+
extern int key_unlink(struct key *keyring,
struct key *key);
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index f45ee0f69c0c..fd9fb11b312b 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -67,6 +67,7 @@
#define KEYCTL_PKEY_SIGN 27 /* Create a public key signature */
#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key signature */
#define KEYCTL_RESTRICT_KEYRING 29 /* Restrict keys allowed to link to a keyring */
+#define KEYCTL_MOVE 30 /* Move keys between keyrings */
/* keyctl structures */
struct keyctl_dh_params {
@@ -112,4 +113,6 @@ struct keyctl_pkey_params {
__u32 __spare[7];
};
+#define KEYCTL_MOVE_EXCL 0x00000001 /* Do not displace from the to-keyring */
+
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 9482df601dc3..b326bc4f84d7 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -159,6 +159,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
return keyctl_pkey_verify(compat_ptr(arg2), compat_ptr(arg3),
compat_ptr(arg4), compat_ptr(arg5));
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 25cdd0cbdc06..b54a58c025ae 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -95,6 +95,8 @@ extern void key_type_put(struct key_type *ktype);
extern int __key_link_lock(struct key *keyring,
const struct keyring_index_key *index_key);
+extern int __key_move_lock(struct key *l_keyring, struct key *u_keyring,
+ const struct keyring_index_key *index_key);
extern int __key_link_begin(struct key *keyring,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
@@ -217,6 +219,7 @@ extern long keyctl_update_key(key_serial_t, const void __user *, size_t);
extern long keyctl_revoke_key(key_serial_t);
extern long keyctl_keyring_clear(key_serial_t);
extern long keyctl_keyring_link(key_serial_t, key_serial_t);
+extern long keyctl_keyring_move(key_serial_t, key_serial_t, key_serial_t, unsigned int);
extern long keyctl_keyring_unlink(key_serial_t, key_serial_t);
extern long keyctl_describe_key(key_serial_t, char __user *, size_t);
extern long keyctl_keyring_search(key_serial_t, const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0f947bcbad46..bbfe7d92d41c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -572,6 +572,52 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
return ret;
}
+/*
+ * Move a link to a key from one keyring to another, displacing any matching
+ * key from the destination keyring.
+ *
+ * The key must grant the caller Link permission and both keyrings must grant
+ * the caller Write permission. There must also be a link in the from keyring
+ * to the key. If both keyrings are the same, nothing is done.
+ *
+ * If successful, 0 will be returned.
+ */
+long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
+ key_serial_t to_ringid, unsigned int flags)
+{
+ key_ref_t key_ref, from_ref, to_ref;
+ long ret;
+
+ if (flags & ~KEYCTL_MOVE_EXCL)
+ return -EINVAL;
+
+ key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
+
+ from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+ if (IS_ERR(from_ref)) {
+ ret = PTR_ERR(from_ref);
+ goto error2;
+ }
+
+ to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ if (IS_ERR(to_ref)) {
+ ret = PTR_ERR(to_ref);
+ goto error3;
+ }
+
+ ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
+ key_ref_to_ptr(to_ref), flags);
+
+ key_ref_put(to_ref);
+error3:
+ key_ref_put(from_ref);
+error2:
+ key_ref_put(key_ref);
+ return ret;
+}
+
/*
* Return a description of a key to userspace.
*
@@ -1772,6 +1818,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
(const void __user *)arg4,
(const void __user *)arg5);
+ case KEYCTL_MOVE:
+ return keyctl_keyring_move((key_serial_t)arg2,
+ (key_serial_t)arg3,
+ (key_serial_t)arg4,
+ (unsigned int)arg5);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8838c300ea7b..dc7397998ba5 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1221,6 +1221,40 @@ int __key_link_lock(struct key *keyring,
return 0;
}
+/*
+ * Lock keyrings for move (link/unlink combination).
+ */
+int __key_move_lock(struct key *l_keyring, struct key *u_keyring,
+ const struct keyring_index_key *index_key)
+ __acquires(&l_keyring->sem)
+ __acquires(&u_keyring->sem)
+ __acquires(&keyring_serialise_link_lock)
+{
+ if (l_keyring->type != &key_type_keyring ||
+ u_keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ /* We have to be very careful here to take the keyring locks in the
+ * right order, lest we open ourselves to deadlocking against another
+ * move operation.
+ */
+ if (l_keyring < u_keyring) {
+ down_write(&l_keyring->sem);
+ down_write_nested(&u_keyring->sem, 1);
+ } else {
+ down_write(&u_keyring->sem);
+ down_write_nested(&l_keyring->sem, 1);
+ }
+
+ /* Serialise link/link calls to prevent parallel calls causing a cycle
+ * when linking two keyring in opposite orders.
+ */
+ if (index_key->type == &key_type_keyring)
+ mutex_lock(&keyring_serialise_link_lock);
+
+ return 0;
+}
+
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
@@ -1489,6 +1523,80 @@ int key_unlink(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_unlink);
+/**
+ * key_move - Move a key from one keyring to another
+ * @key: The key to move
+ * @from_keyring: The keyring to remove the link from.
+ * @to_keyring: The keyring to make the link in.
+ * @flags: Qualifying flags, such as KEYCTL_MOVE_EXCL.
+ *
+ * Make a link in @to_keyring to a key, such that the keyring holds a reference
+ * on that key and the key can potentially be found by searching that keyring
+ * whilst simultaneously removing a link to the key from @from_keyring.
+ *
+ * This function will write-lock both keyring's semaphores and will consume
+ * some of the user's key data quota to hold the link on @to_keyring.
+ *
+ * Returns 0 if successful, -ENOTDIR if either keyring isn't a keyring,
+ * -EKEYREVOKED if either keyring has been revoked, -ENFILE if the second
+ * keyring is full, -EDQUOT if there is insufficient key data quota remaining
+ * to add another link or -ENOMEM if there's insufficient memory. If
+ * KEYCTL_MOVE_EXCL is set, then -EEXIST will be returned if there's already a
+ * matching key in @to_keyring.
+ *
+ * It is assumed that the caller has checked that it is permitted for a link to
+ * be made (the keyring should have Write permission and the key Link
+ * permission).
+ */
+int key_move(struct key *key,
+ struct key *from_keyring,
+ struct key *to_keyring,
+ unsigned int flags)
+{
+ struct assoc_array_edit *from_edit = NULL, *to_edit = NULL;
+ int ret;
+
+ kenter("%d,%d,%d", key->serial, from_keyring->serial, to_keyring->serial);
+
+ if (from_keyring == to_keyring)
+ return 0;
+
+ key_check(key);
+ key_check(from_keyring);
+ key_check(to_keyring);
+
+ ret = __key_move_lock(from_keyring, to_keyring, &key->index_key);
+ if (ret < 0)
+ goto out;
+ ret = __key_unlink_begin(from_keyring, key, &from_edit);
+ if (ret < 0)
+ goto error;
+ ret = __key_link_begin(to_keyring, &key->index_key, &to_edit);
+ if (ret < 0)
+ goto error;
+
+ ret = -EEXIST;
+ if (to_edit->dead_leaf && (flags & KEYCTL_MOVE_EXCL))
+ goto error;
+
+ ret = __key_link_check_restriction(to_keyring, key);
+ if (ret < 0)
+ goto error;
+ ret = __key_link_check_live_key(to_keyring, key);
+ if (ret < 0)
+ goto error;
+
+ __key_unlink(from_keyring, key, &from_edit);
+ __key_link(key, &to_edit);
+error:
+ __key_link_end(to_keyring, &key->index_key, to_edit);
+ __key_unlink_end(from_keyring, key, from_edit);
+out:
+ kleave(" = %d", ret);
+ return ret;
+}
+EXPORT_SYMBOL(key_move);
+
/**
* keyring_clear - Clear a keyring
* @keyring: The keyring to clear.
^ permalink raw reply related
* [PATCH 08/10] keys: Grant Link permission to possessers of request_key auth keys [ver #2]
From: David Howells @ 2019-05-30 17:26 UTC (permalink / raw)
To: keyrings
Cc: James Morris, dhowells, linux-security-module, linux-kernel,
ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
Grant Link permission to the possessers of request_key authentication keys,
thereby allowing a daemon that is servicing upcalls to arrange things such
that only the necessary auth key is passed to the actual service program
and not all the daemon's pending auth keys.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/request_key_auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 572c7a60473a..ec5226557023 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -204,7 +204,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
authkey = key_alloc(&key_type_request_key_auth, desc,
cred->fsuid, cred->fsgid, cred,
- KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+ KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_LINK |
KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
if (IS_ERR(authkey)) {
ret = PTR_ERR(authkey);
^ permalink raw reply related
* [PATCH 09/10] KEYS: reuse keyring_index_key::desc_len in lookup_user_key() [ver #2]
From: David Howells @ 2019-05-30 17:26 UTC (permalink / raw)
To: keyrings
Cc: Eric Biggers, James Morris, dhowells, linux-security-module,
linux-kernel, ebiggers
In-Reply-To: <155923711088.949.14909672457214372214.stgit@warthog.procyon.org.uk>
From: Eric Biggers <ebiggers@google.com>
When lookup_user_key() checks whether the key is possessed, it should
use the key's existing index_key including the 'desc_len' field, rather
than recomputing the 'desc_len'. This doesn't change the behavior; this
way is just simpler and faster.
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
---
security/keys/process_keys.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index ba5d3172cafe..39aaa21462bf 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -688,9 +688,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
key_ref = make_key_ref(key, 0);
/* check to see if we possess the key */
- ctx.index_key.type = key->type;
- ctx.index_key.description = key->description;
- ctx.index_key.desc_len = strlen(key->description);
+ ctx.index_key = key->index_key;
ctx.match_data.raw_data = key;
kdebug("check possessed");
skey_ref = search_process_keyrings(&ctx);
^ permalink raw reply related
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