* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: Alexander Lobakin @ 2021-07-05 12:52 UTC (permalink / raw)
To: John Wood
Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
Andrew Morton, linux-doc, linux-kernel, linux-security-module,
linux-kselftest, linux-arch, linux-hardening, kernel-hardening
In-Reply-To: <20210704140108.GA2742@ubuntu>
From: John Wood <john.wood@gmx.com>
Date: Sun, 4 Jul 2021 16:01:08 +0200
> On Sat, Jul 03, 2021 at 12:59:28PM +0200, John Wood wrote:
> > Hi,
> >
> > On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
> > >
> > > On the other hand, it leaves a potentional window for attackers to
> > > perform brute force from xattr-incapable filesystems. So at the end
> > > of the day I think that the current implementation (a strong
> > > rejection of such filesystems) is way more secure than having
> > > a fallback I proposed.
> >
> > I've been thinking more about this: that the Brute LSM depends on xattr
> > support and I don't like this part. I want that brute force attacks can
> > be detected and mitigated on every system (with minimal dependencies).
> > So, now I am working in a solution without this drawback. I have some
> > ideas but I need to work on it.
>
> I have been coding and testing a bit my ideas but:
>
> Trying to track the applications faults info using kernel memory ends up
> in an easy to abuse system (denied of service due to large amount of memor=
> y
> in use) :(
>
> So, I continue with the v8 idea: xattr to track application crashes info.
>
> > > I'm planning to make a patch which will eliminate such weird rootfs
> > > type selection and just always use more feature-rich tmpfs if it's
> > > compiled in. So, as an alternative, you could add it to your series
> > > as a preparatory change and just add a Kconfig dependency on
> > > CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> > > without messing with any fallbacks at all.
> > > What do you think?
> >
> > Great. But I hope this patch will not be necessary for Brute LSM :)
>
> My words are no longer valid ;)
Ok, so here's the patch that prefers tmpfs for rootfs over ramfs
if it's built-in (which is true for 99% of systems): [0]
For now it hasn't been reviewed by anyone yet, will see. I'm running
my system with this patch for several days already and there were no
issues with rootfs or Brute so far.
[0] https://lore.kernel.org/lkml/20210702233727.21301-1-alobakin@pm.me/
> Thanks,
> John Wood
Thanks,
Al
^ permalink raw reply
* [PATCH] ima: Support euid keyword for buffer measurement
From: Roberto Sassu @ 2021-07-05 11:56 UTC (permalink / raw)
To: zohar
Cc: tusharsu, nramas, linux-integrity, linux-security-module,
linux-kernel, Roberto Sassu
This patch makes the 'euid' keyword available for buffer measurement rules,
in the same way as for other rules. Currently, there is only support for
the 'uid' keyword.
With this change, buffer measurement (or non-measurement) can depend also
on the process effective UID.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_policy.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..fdaa030fb04b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -480,6 +480,16 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule,
if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
return false;
+ if (rule->flags & IMA_EUID) {
+ if (has_capability_noaudit(current, CAP_SETUID)) {
+ if (!rule->uid_op(cred->euid, rule->uid)
+ && !rule->uid_op(cred->suid, rule->uid)
+ && !rule->uid_op(cred->uid, rule->uid))
+ return false;
+ } else if (!rule->uid_op(cred->euid, rule->uid))
+ return false;
+ }
+
switch (rule->func) {
case KEY_CHECK:
if (!rule->keyrings)
@@ -1153,7 +1163,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
- if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+ if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_EUID | IMA_PCR |
IMA_LABEL))
return false;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
From: Roberto Sassu @ 2021-07-05 9:09 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210705090922.3321178-1-roberto.sassu@huawei.com>
This patch adds the 'digest' and 'digest_len' parameters to
ima_measure_critical_data() and process_buffer_measurement(), so that
callers can get the digest of the passed buffer.
These functions calculate the digest even if there is no suitable rule in
the IMA policy and, in this case, they simply return 1 before generating a
new measurement entry.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 5 +--
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_init.c | 3 +-
security/integrity/ima/ima_main.c | 36 ++++++++++++++------
security/integrity/ima/ima_queue_keys.c | 2 +-
security/selinux/ima.c | 6 ++--
8 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 60492263aa64..b6ab66a546ae 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
extern int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash);
+ bool hash, u8 *digest, size_t digest_len);
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
extern void ima_appraise_parse_cmdline(void);
@@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {
static inline int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash)
+ bool hash, u8 *digest,
+ size_t digest_len)
{
return -ENOENT;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 03db221324c3..2f4c20b16ad7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
- bool buf_hash);
+ bool buf_hash, u8 *digest, size_t digest_len);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..63bec42c353f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -357,7 +357,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
"blacklisted-hash", NONE,
- pcr, NULL, false);
+ pcr, NULL, false, NULL, 0);
}
return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index c985418698a4..f6aa0b47a772 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -62,5 +62,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
*/
process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
keyring->description, KEY_CHECK, 0,
- keyring->description, false);
+ keyring->description, false, NULL, 0);
}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5076a7d9d23e..b26fa67476b4 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -154,7 +154,8 @@ int __init ima_init(void)
ima_init_key_queue();
ima_measure_critical_data("kernel_info", "kernel_version",
- UTS_RELEASE, strlen(UTS_RELEASE), false);
+ UTS_RELEASE, strlen(UTS_RELEASE), false,
+ NULL, 0);
return rc;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b512c06d8ee1..360266da5a10 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
* @pcr: pcr to extend the measurement
* @func_data: func specific data, may be NULL
* @buf_hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
*
* Based on policy, either the buffer data or buffer data hash is measured
*
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
*/
int process_buffer_measurement(struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
- bool buf_hash)
+ bool buf_hash, u8 *digest, size_t digest_len)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
int action = 0;
u32 secid;
- if (!ima_policy_flag)
+ if (digest && digest_len < digest_hash_len)
+ return -EINVAL;
+
+ if (!ima_policy_flag && !digest)
return -ENOENT;
template = ima_template_desc_buf();
@@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
action = ima_get_action(mnt_userns, inode, current_cred(),
secid, 0, func, &pcr, &template,
func_data);
- if (!(action & IMA_MEASURE))
+ if (!(action & IMA_MEASURE) && !digest)
return -ENOENT;
}
@@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
event_data.buf_len = digest_hash_len;
}
+ if (digest)
+ memcpy(digest, iint.ima_hash->digest, digest_hash_len);
+
+ if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
+ return 1;
+
ret = ima_alloc_init_template(&event_data, &entry, template);
if (ret < 0) {
audit_cause = "alloc_entry";
@@ -964,7 +976,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
- NULL, false);
+ NULL, false, NULL, 0);
fdput(f);
}
@@ -975,26 +987,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
* @buf: pointer to buffer data
* @buf_len: length of buffer data (in bytes)
* @hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
*
* Measure data critical to the integrity of the kernel into the IMA log
* and extend the pcr. Examples of critical data could be various data
* structures, policies, and states stored in kernel memory that can
* impact the integrity of the system.
*
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
*/
int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash)
+ bool hash, u8 *digest, size_t digest_len)
{
if (!event_name || !event_label || !buf || !buf_len)
return -ENOPARAM;
return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
event_name, CRITICAL_DATA, 0,
- event_label, hash);
+ event_label, hash, digest,
+ digest_len);
}
static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 979ef6c71f3d..93056c03bf5a 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -165,7 +165,7 @@ void ima_process_queued_keys(void)
entry->keyring_name,
KEY_CHECK, 0,
entry->keyring_name,
- false);
+ false, NULL, 0);
list_del(&entry->list);
ima_free_key_entry(entry);
}
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
index 34d421861bfc..727c4e43219d 100644
--- a/security/selinux/ima.c
+++ b/security/selinux/ima.c
@@ -86,7 +86,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
}
ima_measure_critical_data("selinux", "selinux-state",
- state_str, strlen(state_str), false);
+ state_str, strlen(state_str), false,
+ NULL, 0);
kfree(state_str);
@@ -103,7 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
}
ima_measure_critical_data("selinux", "selinux-policy-hash",
- policy, policy_len, true);
+ policy, policy_len, true,
+ NULL, 0);
vfree(policy);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v3 2/3] ima: Return int in the functions to measure a buffer
From: Roberto Sassu @ 2021-07-05 9:09 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210705090922.3321178-1-roberto.sassu@huawei.com>
ima_measure_critical_data() and process_buffer_measurement() currently
don't return a result. A caller wouldn't be able to know whether those
functions were executed successfully.
This patch modifies the return type from void to int, and returns 0 if the
buffer has been successfully measured, a negative value otherwise.
Also, this patch does not modify the behavior of existing callers by
processing the returned value. For those, the return value is ignored.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 15 +++++++-----
security/integrity/ima/ima.h | 10 ++++----
security/integrity/ima/ima_main.c | 40 ++++++++++++++++++-------------
3 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 81e830d01ced..60492263aa64 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
-extern void ima_measure_critical_data(const char *event_label,
- const char *event_name,
- const void *buf, size_t buf_len,
- bool hash);
+extern int ima_measure_critical_data(const char *event_label,
+ const char *event_name,
+ const void *buf, size_t buf_len,
+ bool hash);
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
extern void ima_appraise_parse_cmdline(void);
@@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size
static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
-static inline void ima_measure_critical_data(const char *event_label,
+static inline int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash) {}
+ bool hash)
+{
+ return -ENOENT;
+}
#endif /* CONFIG_IMA */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..03db221324c3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
-void process_buffer_measurement(struct user_namespace *mnt_userns,
- struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data,
- bool buf_hash);
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+ struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool buf_hash);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8ef1fa357e0c..b512c06d8ee1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-/*
+/**
* process_buffer_measurement - Measure the buffer or the buffer data hash
* @mnt_userns: user namespace of the mount the inode was found from
* @inode: inode associated with the object being measured (NULL for KEY_CHECK)
@@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
* @buf_hash: measure buffer data hash
*
* Based on policy, either the buffer data or buffer data hash is measured
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
*/
-void process_buffer_measurement(struct user_namespace *mnt_userns,
- struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data,
- bool buf_hash)
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+ struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool buf_hash)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
u32 secid;
if (!ima_policy_flag)
- return;
+ return -ENOENT;
template = ima_template_desc_buf();
if (!template) {
@@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
secid, 0, func, &pcr, &template,
func_data);
if (!(action & IMA_MEASURE))
- return;
+ return -ENOENT;
}
if (!pcr)
@@ -937,7 +940,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
func_measure_str(func),
audit_cause, ret, 0, ret);
- return;
+ return ret;
}
/**
@@ -977,18 +980,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
* and extend the pcr. Examples of critical data could be various data
* structures, policies, and states stored in kernel memory that can
* impact the integrity of the system.
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
*/
-void ima_measure_critical_data(const char *event_label,
- const char *event_name,
- const void *buf, size_t buf_len,
- bool hash)
+int ima_measure_critical_data(const char *event_label,
+ const char *event_name,
+ const void *buf, size_t buf_len,
+ bool hash)
{
if (!event_name || !event_label || !buf || !buf_len)
- return;
+ return -ENOPARAM;
- process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name,
- CRITICAL_DATA, 0, event_label,
- hash);
+ return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
+ event_name, CRITICAL_DATA, 0,
+ event_label, hash);
}
static int __init init_ima(void)
--
2.25.1
^ permalink raw reply related
* [PATCH v3 1/3] ima: Introduce ima_get_current_hash_algo()
From: Roberto Sassu @ 2021-07-05 9:09 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210705090922.3321178-1-roberto.sassu@huawei.com>
This patch introduces the new function ima_get_current_hash_algo(), that
callers in the other kernel subsystems might use to obtain the hash
algorithm selected by IMA.
Its primary use will be to determine which algorithm has been used to
calculate the digest written by ima_measure_critical_data() to the location
passed as a new parameter (in a subsequent patch).
Since the hash algorithm does not change after the IMA setup phase, there
is no risk of races (obtaining a digest calculated with a different
algorithm than the one returned).
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 7 +++++++
security/integrity/ima/ima_main.c | 5 +++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 61d5723ec303..81e830d01ced 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,9 +11,11 @@
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/kexec.h>
+#include <crypto/hash_info.h>
struct linux_binprm;
#ifdef CONFIG_IMA
+extern enum hash_algo ima_get_current_hash_algo(void);
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask);
extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
@@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void)
#endif
#else
+static inline enum hash_algo ima_get_current_hash_algo(void)
+{
+ return HASH_ALGO__LAST;
+}
+
static inline int ima_bprm_check(struct linux_binprm *bprm)
{
return 0;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..8ef1fa357e0c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -76,6 +76,11 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);
+enum hash_algo ima_get_current_hash_algo(void)
+{
+ return ima_hash_algo;
+}
+
/* Prevent mmap'ing a file execute that is already mmap'ed write */
static int mmap_violation_check(enum ima_hooks func, struct file *file,
char **pathbuf, const char **pathname,
--
2.25.1
^ permalink raw reply related
* [PATCH v3 0/3] ima: Provide more info about buffer measurement
From: Roberto Sassu @ 2021-07-05 9:09 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
This patch set provides more information about buffer measurement.
First, it introduces the new function ima_get_current_hash_algo(), to
obtain the algorithm used to calculate the buffer digest (patch 1).
Second, it changes the type of return value of ima_measure_critical_data()
and process_buffer_measurement() from void to int, to signal to the callers
whether or not the buffer has been measured, or just the digest has been
calculated and written to the supplied location (patch 2).
Lastly, it adds two new parameters to the functions above ('digest' and
'digest_len'), so that those functions can write the buffer digest to the
location supplied by the callers (patch 3).
This patch set replaces the patch 'ima: Add digest, algo, measured
parameters to ima_measure_critical_data()' in:
https://lore.kernel.org/linux-integrity/20210625165614.2284243-1-roberto.sassu@huawei.com/
Changelog
v2:
- remove assignments of ima_measure_critical_data() and
process_buffer_measurement() return values (suggested by Lakshmi)
v1:
- add digest_len parameter to ima_measure_critical_data() and
process_buffer_measurement() (suggested by Lakshmi)
- fix doc formatting issues
Huawei Digest Lists patch set:
- introduce ima_get_current_hash_algo() (suggested by Mimi)
- remove algo and measured parameters from ima_measure_critical_data() and
process_buffer_measurement() (suggested by Mimi)
- return an integer from ima_measure_critical_data() and
process_buffer_measurement() (suggested by Mimi)
- correctly check when process_buffer_measurement() should return earlier
Roberto Sassu (3):
ima: Introduce ima_get_current_hash_algo()
ima: Return int in the functions to measure a buffer
ima: Add digest and digest_len params to the functions to measure a
buffer
include/linux/ima.h | 23 +++++--
security/integrity/ima/ima.h | 10 +--
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_init.c | 3 +-
security/integrity/ima/ima_main.c | 67 ++++++++++++++------
security/integrity/ima/ima_queue_keys.c | 2 +-
security/selinux/ima.c | 6 +-
8 files changed, 78 insertions(+), 37 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v3 2/2] perf: Refactor permissions check into perf_check_permission()
From: Marco Elver @ 2021-07-05 8:44 UTC (permalink / raw)
To: elver, peterz
Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
linux-perf-users, ebiederm, omosnace, serge,
linux-security-module
In-Reply-To: <20210705084453.2151729-1-elver@google.com>
Refactor the permission check in perf_event_open() into a helper
perf_check_permission(). This makes the permission check logic more
readable (because we no longer have a negated disjunction). Add a
comment mentioning the ptrace check also checks the uid.
No functional change intended.
Signed-off-by: Marco Elver <elver@google.com>
---
v3:
* Introduce this patch to refactor the permissions checking logic to
make it more readable (reported by Eric W. Biederman).
---
kernel/events/core.c | 58 ++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f79ee82e644a..3008b986994b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11912,6 +11912,37 @@ __perf_event_ctx_lock_double(struct perf_event *group_leader,
return gctx;
}
+static bool
+perf_check_permission(struct perf_event_attr *attr, struct task_struct *task)
+{
+ unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+ bool is_capable = perfmon_capable();
+
+ if (attr->sigtrap) {
+ /*
+ * perf_event_attr::sigtrap sends signals to the other task.
+ * Require the current task to also have CAP_KILL.
+ */
+ rcu_read_lock();
+ is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+ rcu_read_unlock();
+
+ /*
+ * If the required capabilities aren't available, checks for
+ * ptrace permissions: upgrade to ATTACH, since sending signals
+ * can effectively change the target task.
+ */
+ ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+ }
+
+ /*
+ * Preserve ptrace permission check for backwards compatibility. The
+ * ptrace check also includes checks that the current task and other
+ * task have matching uids, and is therefore not done here explicitly.
+ */
+ return is_capable || ptrace_may_access(task, ptrace_mode);
+}
+
/**
* sys_perf_event_open - open a performance event, associate it to a task/cpu
*
@@ -12152,43 +12183,18 @@ SYSCALL_DEFINE5(perf_event_open,
}
if (task) {
- unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
- bool is_capable;
-
err = down_read_interruptible(&task->signal->exec_update_lock);
if (err)
goto err_file;
- is_capable = perfmon_capable();
- if (attr.sigtrap) {
- /*
- * perf_event_attr::sigtrap sends signals to the other
- * task. Require the current task to also have
- * CAP_KILL.
- */
- rcu_read_lock();
- is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
- rcu_read_unlock();
-
- /*
- * If the required capabilities aren't available, checks
- * for ptrace permissions: upgrade to ATTACH, since
- * sending signals can effectively change the target
- * task.
- */
- ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
- }
-
/*
- * Preserve ptrace permission check for backwards compatibility.
- *
* We must hold exec_update_lock across this and any potential
* perf_install_in_context() call for this new event to
* serialize against exec() altering our credentials (and the
* perf_event_exit_task() that could imply).
*/
err = -EACCES;
- if (!is_capable && !ptrace_may_access(task, ptrace_mode))
+ if (!perf_check_permission(&attr, task))
goto err_cred;
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* [PATCH v3 1/2] perf: Fix required permissions if sigtrap is requested
From: Marco Elver @ 2021-07-05 8:44 UTC (permalink / raw)
To: elver, peterz
Cc: tglx, mingo, dvyukov, glider, kasan-dev, linux-kernel, mingo,
acme, mark.rutland, alexander.shishkin, jolsa, namhyung,
linux-perf-users, ebiederm, omosnace, serge,
linux-security-module, stable
If perf_event_open() is called with another task as target and
perf_event_attr::sigtrap is set, and the target task's user does not
match the calling user, also require the CAP_KILL capability or
PTRACE_MODE_ATTACH permissions.
Otherwise, with the CAP_PERFMON capability alone it would be possible
for a user to send SIGTRAP signals via perf events to another user's
tasks. This could potentially result in those tasks being terminated if
they cannot handle SIGTRAP signals.
Note: The check complements the existing capability check, but is not
supposed to supersede the ptrace_may_access() check. At a high level we
now have:
capable of CAP_PERFMON and (CAP_KILL if sigtrap)
OR
ptrace_may_access(...) // also checks for same thread-group and uid
Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
Cc: <stable@vger.kernel.org> # 5.13+
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
v3:
* Upgrade ptrace mode check to ATTACH if attr.sigtrap, otherwise it's
possible to change the target task (send signal) even if only read
ptrace permissions were granted (reported by Eric W. Biederman).
v2: https://lkml.kernel.org/r/20210701083842.580466-1-elver@google.com
* Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
* Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
capability in target task's ns (reported by Ondrej Mosnacek).
v1: https://lkml.kernel.org/r/20210630093709.3612997-1-elver@google.com
---
kernel/events/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index fe88d6eea3c2..f79ee82e644a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12152,10 +12152,33 @@ SYSCALL_DEFINE5(perf_event_open,
}
if (task) {
+ unsigned int ptrace_mode = PTRACE_MODE_READ_REALCREDS;
+ bool is_capable;
+
err = down_read_interruptible(&task->signal->exec_update_lock);
if (err)
goto err_file;
+ is_capable = perfmon_capable();
+ if (attr.sigtrap) {
+ /*
+ * perf_event_attr::sigtrap sends signals to the other
+ * task. Require the current task to also have
+ * CAP_KILL.
+ */
+ rcu_read_lock();
+ is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+ rcu_read_unlock();
+
+ /*
+ * If the required capabilities aren't available, checks
+ * for ptrace permissions: upgrade to ATTACH, since
+ * sending signals can effectively change the target
+ * task.
+ */
+ ptrace_mode = PTRACE_MODE_ATTACH_REALCREDS;
+ }
+
/*
* Preserve ptrace permission check for backwards compatibility.
*
@@ -12165,7 +12188,7 @@ SYSCALL_DEFINE5(perf_event_open,
* perf_event_exit_task() that could imply).
*/
err = -EACCES;
- if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+ if (!is_capable && !ptrace_may_access(task, ptrace_mode))
goto err_cred;
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* Re: [syzbot] general protection fault in legacy_parse_param
From: Dmitry Vyukov @ 2021-07-05 5:52 UTC (permalink / raw)
To: Paul Moore
Cc: Casey Schaufler, syzbot, linux-security-module, Stephen Smalley,
selinux, David Howells, linux-fsdevel, linux-kernel,
syzkaller-bugs, viro
In-Reply-To: <CAHC9VhSq88YjA-VGSTKkc4hkc_KOK=mnoAYiX1us6O6U0gFzAQ@mail.gmail.com>
On Sun, Jul 4, 2021 at 4:14 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sat, Jul 3, 2021 at 6:16 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 7/2/2021 10:51 PM, Dmitry Vyukov wrote:
> > > On Sat, Jul 3, 2021 at 7:41 AM syzbot
> > > <syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com> wrote:
> > >> Hello,
> > >>
> > >> syzbot found the following issue on:
> > >>
> > >> HEAD commit: 62fb9874 Linux 5.13
> > >> git tree: upstream
> > >> console output: https://syzkaller.appspot.com/x/log.txt?x=12ffa118300000
> > >> kernel config: https://syzkaller.appspot.com/x/.config?x=19404adbea015a58
> > >> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
> > >> compiler: Debian clang version 11.0.1-2
> > >>
> > >> Unfortunately, I don't have any reproducer for this issue yet.
> > >>
> > >> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > >> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> > > +Casey for what looks like a smackfs issue
> >
> > This is from the new mount infrastructure introduced by
> > David Howells in November 2018. It makes sense that there
> > may be a problem in SELinux as well, as the code was introduced
> > by the same developer at the same time for the same purpose.
> >
> > > The crash was triggered by this test case:
> > >
> > > 21:55:33 executing program 1:
> > > r0 = fsopen(&(0x7f0000000040)='ext3\x00', 0x1)
> > > fsconfig$FSCONFIG_SET_STRING(r0, 0x1, &(0x7f00000002c0)='smackfsroot',
> > > &(0x7f0000000300)='default_permissions', 0x0)
> > >
> > > And I think the issue is in smack_fs_context_parse_param():
> > > https://elixir.bootlin.com/linux/latest/source/security/smack/smack_lsm.c#L691
> > >
> > > But it seems that selinux_fs_context_parse_param() contains the same issue:
> > > https://elixir.bootlin.com/linux/latest/source/security/selinux/hooks.c#L2919
> > > +So selinux maintainers as well.
> > >
> > >> general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
> > >> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> > >> CPU: 0 PID: 20300 Comm: syz-executor.1 Not tainted 5.13.0-syzkaller #0
> > >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > >> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
> > >> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
> > >> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
> > >> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
> > >> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
> > >> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
> > >> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
> > >> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
> > >> FS: 00007fe01ae27700(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
> > >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> CR2: 00000000005645a8 CR3: 0000000018afc000 CR4: 00000000001506f0
> > >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > >> Call Trace:
> > >> legacy_parse_param+0x461/0x7e0 fs/fs_context.c:537
> > >> vfs_parse_fs_param+0x1e5/0x460 fs/fs_context.c:117
>
> It's Sunday morning and perhaps my mind is not yet in a "hey, let's
> look at VFS kernel code!" mindset, but I'm not convinced the problem
> is the 'param->string = NULL' assignment in the LSM hooks. In both
> the case of SELinux and Smack that code ends up returning either a 0
> (Smack) or a 1 (SELinux) - that's a little odd in it's own way, but I
> don't believe it is relevant here - either way these return values are
> not equal to -ENOPARAM so we should end up returning early from
> vfs_parse_fs_param before it calls down into legacy_parse_param():
>
> Taken from https://elixir.bootlin.com/linux/latest/source/fs/fs_context.c#L109 :
>
> ret = security_fs_context_parse_param(fc, param);
> if (ret != -ENOPARAM)
> /* Param belongs to the LSM or is disallowed by the LSM; so
> * don't pass to the FS.
> */
> return ret;
>
> if (fc->ops->parse_param) {
> ret = fc->ops->parse_param(fc, param);
> if (ret != -ENOPARAM)
> return ret;
> }
Hi Paul,
You are right.
I almost connected the dots, but not exactly.
Now that I read more code around, setting "param->string = NULL" in
smack_fs_context_parse_param() looks correct to me (the fs copies and
takes ownership of the string).
I don't see how the crash happened...
> > >> vfs_fsconfig_locked fs/fsopen.c:265 [inline]
> > >> __do_sys_fsconfig fs/fsopen.c:439 [inline]
> > >> __se_sys_fsconfig+0xba9/0xff0 fs/fsopen.c:314
> > >> do_syscall_64+0x3f/0xb0 arch/x86/entry/common.c:47
> > >> entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >> RIP: 0033:0x4665d9
> > >> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> > >> RSP: 002b:00007fe01ae27188 EFLAGS: 00000246 ORIG_RAX: 00000000000001af
> > >> RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665d9
> > >> RDX: 00000000200002c0 RSI: 0000000000000001 RDI: 0000000000000003
> > >> RBP: 00000000004bfcb9 R08: 0000000000000000 R09: 0000000000000000
> > >> R10: 0000000020000300 R11: 0000000000000246 R12: 000000000056bf80
> > >> R13: 00007ffd4bb7c5bf R14: 00007fe01ae27300 R15: 0000000000022000
> > >> Modules linked in:
> > >> ---[ end trace 5d7119165725bd63 ]---
>
> --
> paul moore
> www.paul-moore.com
^ permalink raw reply
* [PATCH AUTOSEL 5.12 36/80] ima: Don't remove security.ima if file must not be appraised
From: Sasha Levin @ 2021-07-04 23:05 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Roberto Sassu, Mimi Zohar, Sasha Levin, linux-integrity,
linux-security-module
In-Reply-To: <20210704230616.1489200-1-sashal@kernel.org>
From: Roberto Sassu <roberto.sassu@huawei.com>
[ Upstream commit ed1b472fc15aeaa20ddeeb93fd25190014e50d17 ]
Files might come from a remote source and might have xattrs, including
security.ima. It should not be IMA task to decide whether security.ima
should be kept or not. This patch removes the removexattr() system
call in ima_inode_post_setattr().
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/integrity/ima/ima_appraise.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 565e33ff19d0..d7cc6f897746 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -522,8 +522,6 @@ void ima_inode_post_setattr(struct user_namespace *mnt_userns,
return;
action = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, POST_SETATTR);
- if (!action)
- __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_IMA);
iint = integrity_iint_find(inode);
if (iint) {
set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
--
2.30.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.13 38/85] ima: Don't remove security.ima if file must not be appraised
From: Sasha Levin @ 2021-07-04 23:03 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Roberto Sassu, Mimi Zohar, Sasha Levin, linux-integrity,
linux-security-module
In-Reply-To: <20210704230420.1488358-1-sashal@kernel.org>
From: Roberto Sassu <roberto.sassu@huawei.com>
[ Upstream commit ed1b472fc15aeaa20ddeeb93fd25190014e50d17 ]
Files might come from a remote source and might have xattrs, including
security.ima. It should not be IMA task to decide whether security.ima
should be kept or not. This patch removes the removexattr() system
call in ima_inode_post_setattr().
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/integrity/ima/ima_appraise.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 4e5eb0236278..55dac618f2a1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -522,8 +522,6 @@ void ima_inode_post_setattr(struct user_namespace *mnt_userns,
return;
action = ima_must_appraise(mnt_userns, inode, MAY_ACCESS, POST_SETATTR);
- if (!action)
- __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_IMA);
iint = integrity_iint_find(inode);
if (iint) {
set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
--
2.30.2
^ permalink raw reply related
* Re: [syzbot] general protection fault in legacy_parse_param
From: Paul Moore @ 2021-07-04 14:14 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dmitry Vyukov, syzbot, linux-security-module, Stephen Smalley,
selinux, David Howells, linux-fsdevel, linux-kernel,
syzkaller-bugs, viro
In-Reply-To: <d11c276d-65a0-5273-d797-1092e1e2692a@schaufler-ca.com>
On Sat, Jul 3, 2021 at 6:16 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/2/2021 10:51 PM, Dmitry Vyukov wrote:
> > On Sat, Jul 3, 2021 at 7:41 AM syzbot
> > <syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com> wrote:
> >> Hello,
> >>
> >> syzbot found the following issue on:
> >>
> >> HEAD commit: 62fb9874 Linux 5.13
> >> git tree: upstream
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=12ffa118300000
> >> kernel config: https://syzkaller.appspot.com/x/.config?x=19404adbea015a58
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
> >> compiler: Debian clang version 11.0.1-2
> >>
> >> Unfortunately, I don't have any reproducer for this issue yet.
> >>
> >> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> >> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> > +Casey for what looks like a smackfs issue
>
> This is from the new mount infrastructure introduced by
> David Howells in November 2018. It makes sense that there
> may be a problem in SELinux as well, as the code was introduced
> by the same developer at the same time for the same purpose.
>
> > The crash was triggered by this test case:
> >
> > 21:55:33 executing program 1:
> > r0 = fsopen(&(0x7f0000000040)='ext3\x00', 0x1)
> > fsconfig$FSCONFIG_SET_STRING(r0, 0x1, &(0x7f00000002c0)='smackfsroot',
> > &(0x7f0000000300)='default_permissions', 0x0)
> >
> > And I think the issue is in smack_fs_context_parse_param():
> > https://elixir.bootlin.com/linux/latest/source/security/smack/smack_lsm.c#L691
> >
> > But it seems that selinux_fs_context_parse_param() contains the same issue:
> > https://elixir.bootlin.com/linux/latest/source/security/selinux/hooks.c#L2919
> > +So selinux maintainers as well.
> >
> >> general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
> >> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> >> CPU: 0 PID: 20300 Comm: syz-executor.1 Not tainted 5.13.0-syzkaller #0
> >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> >> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
> >> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
> >> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
> >> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
> >> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
> >> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
> >> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
> >> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
> >> FS: 00007fe01ae27700(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
> >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> CR2: 00000000005645a8 CR3: 0000000018afc000 CR4: 00000000001506f0
> >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> Call Trace:
> >> legacy_parse_param+0x461/0x7e0 fs/fs_context.c:537
> >> vfs_parse_fs_param+0x1e5/0x460 fs/fs_context.c:117
It's Sunday morning and perhaps my mind is not yet in a "hey, let's
look at VFS kernel code!" mindset, but I'm not convinced the problem
is the 'param->string = NULL' assignment in the LSM hooks. In both
the case of SELinux and Smack that code ends up returning either a 0
(Smack) or a 1 (SELinux) - that's a little odd in it's own way, but I
don't believe it is relevant here - either way these return values are
not equal to -ENOPARAM so we should end up returning early from
vfs_parse_fs_param before it calls down into legacy_parse_param():
Taken from https://elixir.bootlin.com/linux/latest/source/fs/fs_context.c#L109 :
ret = security_fs_context_parse_param(fc, param);
if (ret != -ENOPARAM)
/* Param belongs to the LSM or is disallowed by the LSM; so
* don't pass to the FS.
*/
return ret;
if (fc->ops->parse_param) {
ret = fc->ops->parse_param(fc, param);
if (ret != -ENOPARAM)
return ret;
}
> >> vfs_fsconfig_locked fs/fsopen.c:265 [inline]
> >> __do_sys_fsconfig fs/fsopen.c:439 [inline]
> >> __se_sys_fsconfig+0xba9/0xff0 fs/fsopen.c:314
> >> do_syscall_64+0x3f/0xb0 arch/x86/entry/common.c:47
> >> entry_SYSCALL_64_after_hwframe+0x44/0xae
> >> RIP: 0033:0x4665d9
> >> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> >> RSP: 002b:00007fe01ae27188 EFLAGS: 00000246 ORIG_RAX: 00000000000001af
> >> RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665d9
> >> RDX: 00000000200002c0 RSI: 0000000000000001 RDI: 0000000000000003
> >> RBP: 00000000004bfcb9 R08: 0000000000000000 R09: 0000000000000000
> >> R10: 0000000020000300 R11: 0000000000000246 R12: 000000000056bf80
> >> R13: 00007ffd4bb7c5bf R14: 00007fe01ae27300 R15: 0000000000022000
> >> Modules linked in:
> >> ---[ end trace 5d7119165725bd63 ]---
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: John Wood @ 2021-07-04 14:01 UTC (permalink / raw)
To: Alexander Lobakin
Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap, Andrew Morton,
linux-doc, linux-kernel, linux-security-module, linux-kselftest,
linux-arch, linux-hardening, kernel-hardening
In-Reply-To: <20210703105928.GA2830@ubuntu>
On Sat, Jul 03, 2021 at 12:59:28PM +0200, John Wood wrote:
> Hi,
>
> On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
> >
> > On the other hand, it leaves a potentional window for attackers to
> > perform brute force from xattr-incapable filesystems. So at the end
> > of the day I think that the current implementation (a strong
> > rejection of such filesystems) is way more secure than having
> > a fallback I proposed.
>
> I've been thinking more about this: that the Brute LSM depends on xattr
> support and I don't like this part. I want that brute force attacks can
> be detected and mitigated on every system (with minimal dependencies).
> So, now I am working in a solution without this drawback. I have some
> ideas but I need to work on it.
I have been coding and testing a bit my ideas but:
Trying to track the applications faults info using kernel memory ends up
in an easy to abuse system (denied of service due to large amount of memory
in use) :(
So, I continue with the v8 idea: xattr to track application crashes info.
> > I'm planning to make a patch which will eliminate such weird rootfs
> > type selection and just always use more feature-rich tmpfs if it's
> > compiled in. So, as an alternative, you could add it to your series
> > as a preparatory change and just add a Kconfig dependency on
> > CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> > without messing with any fallbacks at all.
> > What do you think?
>
> Great. But I hope this patch will not be necessary for Brute LSM :)
My words are no longer valid ;)
Thanks,
John Wood
^ permalink raw reply
* Re: [syzbot] general protection fault in legacy_parse_param
From: Casey Schaufler @ 2021-07-03 22:16 UTC (permalink / raw)
To: Dmitry Vyukov, syzbot, linux-security-module, Paul Moore,
Stephen Smalley, selinux, David Howells
Cc: linux-fsdevel, linux-kernel, syzkaller-bugs, viro
In-Reply-To: <CACT4Y+YysFa1UzT6zw9GGns69WSFgqrL6P_LjUju6ujcJRTaeA@mail.gmail.com>
On 7/2/2021 10:51 PM, Dmitry Vyukov wrote:
> On Sat, Jul 3, 2021 at 7:41 AM syzbot
> <syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com> wrote:
>> Hello,
>>
>> syzbot found the following issue on:
>>
>> HEAD commit: 62fb9874 Linux 5.13
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=12ffa118300000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=19404adbea015a58
>> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
>> compiler: Debian clang version 11.0.1-2
>>
>> Unfortunately, I don't have any reproducer for this issue yet.
>>
>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> +Casey for what looks like a smackfs issue
This is from the new mount infrastructure introduced by
David Howells in November 2018. It makes sense that there
may be a problem in SELinux as well, as the code was introduced
by the same developer at the same time for the same purpose.
>
> The crash was triggered by this test case:
>
> 21:55:33 executing program 1:
> r0 = fsopen(&(0x7f0000000040)='ext3\x00', 0x1)
> fsconfig$FSCONFIG_SET_STRING(r0, 0x1, &(0x7f00000002c0)='smackfsroot',
> &(0x7f0000000300)='default_permissions', 0x0)
>
> And I think the issue is in smack_fs_context_parse_param():
> https://elixir.bootlin.com/linux/latest/source/security/smack/smack_lsm.c#L691
>
> But it seems that selinux_fs_context_parse_param() contains the same issue:
> https://elixir.bootlin.com/linux/latest/source/security/selinux/hooks.c#L2919
> +So selinux maintainers as well.
>
>
>
>> general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
>> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>> CPU: 0 PID: 20300 Comm: syz-executor.1 Not tainted 5.13.0-syzkaller #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
>> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
>> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
>> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
>> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
>> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
>> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
>> FS: 00007fe01ae27700(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00000000005645a8 CR3: 0000000018afc000 CR4: 00000000001506f0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> legacy_parse_param+0x461/0x7e0 fs/fs_context.c:537
>> vfs_parse_fs_param+0x1e5/0x460 fs/fs_context.c:117
>> vfs_fsconfig_locked fs/fsopen.c:265 [inline]
>> __do_sys_fsconfig fs/fsopen.c:439 [inline]
>> __se_sys_fsconfig+0xba9/0xff0 fs/fsopen.c:314
>> do_syscall_64+0x3f/0xb0 arch/x86/entry/common.c:47
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>> RIP: 0033:0x4665d9
>> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
>> RSP: 002b:00007fe01ae27188 EFLAGS: 00000246 ORIG_RAX: 00000000000001af
>> RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665d9
>> RDX: 00000000200002c0 RSI: 0000000000000001 RDI: 0000000000000003
>> RBP: 00000000004bfcb9 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000020000300 R11: 0000000000000246 R12: 000000000056bf80
>> R13: 00007ffd4bb7c5bf R14: 00007fe01ae27300 R15: 0000000000022000
>> Modules linked in:
>> ---[ end trace 5d7119165725bd63 ]---
>> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
>> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
>> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
>> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
>> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
>> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
>> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
>> FS: 00007fe01ae27700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00000000004e4da0 CR3: 0000000018afc000 CR4: 00000000001506e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>
>>
>> ---
>> This report is generated by a bot. It may contain errors.
>> See https://goo.gl/tpsmEJ for more information about syzbot.
>> syzbot engineers can be reached at syzkaller@googlegroups.com.
>>
>> syzbot will keep track of this issue. See:
>> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>>
>> --
>> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000004e5ec705c6318557%40google.com.
^ permalink raw reply
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: John Wood @ 2021-07-03 10:59 UTC (permalink / raw)
To: Alexander Lobakin
Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap, Andrew Morton,
linux-doc, linux-kernel, linux-security-module, linux-kselftest,
linux-arch, linux-hardening, kernel-hardening
In-Reply-To: <20210702170101.16116-1-alobakin@pm.me>
Hi,
On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
>
> On the other hand, it leaves a potentional window for attackers to
> perform brute force from xattr-incapable filesystems. So at the end
> of the day I think that the current implementation (a strong
> rejection of such filesystems) is way more secure than having
> a fallback I proposed.
I've been thinking more about this: that the Brute LSM depends on xattr
support and I don't like this part. I want that brute force attacks can
be detected and mitigated on every system (with minimal dependencies).
So, now I am working in a solution without this drawback. I have some
ideas but I need to work on it.
> I'm planning to make a patch which will eliminate such weird rootfs
> type selection and just always use more feature-rich tmpfs if it's
> compiled in. So, as an alternative, you could add it to your series
> as a preparatory change and just add a Kconfig dependency on
> CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> without messing with any fallbacks at all.
> What do you think?
Great. But I hope this patch will not be necessary for Brute LSM :)
Thanks,
John Wood
^ permalink raw reply
* Re: [syzbot] general protection fault in legacy_parse_param
From: Dmitry Vyukov @ 2021-07-03 5:51 UTC (permalink / raw)
To: syzbot, Casey Schaufler, linux-security-module, Paul Moore,
Stephen Smalley, selinux
Cc: linux-fsdevel, linux-kernel, syzkaller-bugs, viro
In-Reply-To: <0000000000004e5ec705c6318557@google.com>
On Sat, Jul 3, 2021 at 7:41 AM syzbot
<syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 62fb9874 Linux 5.13
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=12ffa118300000
> kernel config: https://syzkaller.appspot.com/x/.config?x=19404adbea015a58
> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
> compiler: Debian clang version 11.0.1-2
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
+Casey for what looks like a smackfs issue
The crash was triggered by this test case:
21:55:33 executing program 1:
r0 = fsopen(&(0x7f0000000040)='ext3\x00', 0x1)
fsconfig$FSCONFIG_SET_STRING(r0, 0x1, &(0x7f00000002c0)='smackfsroot',
&(0x7f0000000300)='default_permissions', 0x0)
And I think the issue is in smack_fs_context_parse_param():
https://elixir.bootlin.com/linux/latest/source/security/smack/smack_lsm.c#L691
But it seems that selinux_fs_context_parse_param() contains the same issue:
https://elixir.bootlin.com/linux/latest/source/security/selinux/hooks.c#L2919
+So selinux maintainers as well.
> general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> CPU: 0 PID: 20300 Comm: syz-executor.1 Not tainted 5.13.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
> FS: 00007fe01ae27700(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000005645a8 CR3: 0000000018afc000 CR4: 00000000001506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> legacy_parse_param+0x461/0x7e0 fs/fs_context.c:537
> vfs_parse_fs_param+0x1e5/0x460 fs/fs_context.c:117
> vfs_fsconfig_locked fs/fsopen.c:265 [inline]
> __do_sys_fsconfig fs/fsopen.c:439 [inline]
> __se_sys_fsconfig+0xba9/0xff0 fs/fsopen.c:314
> do_syscall_64+0x3f/0xb0 arch/x86/entry/common.c:47
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x4665d9
> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007fe01ae27188 EFLAGS: 00000246 ORIG_RAX: 00000000000001af
> RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665d9
> RDX: 00000000200002c0 RSI: 0000000000000001 RDI: 0000000000000003
> RBP: 00000000004bfcb9 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000020000300 R11: 0000000000000246 R12: 000000000056bf80
> R13: 00007ffd4bb7c5bf R14: 00007fe01ae27300 R15: 0000000000022000
> Modules linked in:
> ---[ end trace 5d7119165725bd63 ]---
> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
> FS: 00007fe01ae27700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000004e4da0 CR3: 0000000018afc000 CR4: 00000000001506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000004e5ec705c6318557%40google.com.
^ permalink raw reply
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: Alexander Lobakin @ 2021-07-02 17:08 UTC (permalink / raw)
To: John Wood
Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
Andrew Morton, linux-doc, linux-kernel, linux-security-module,
linux-kselftest, linux-arch, linux-hardening, kernel-hardening
In-Reply-To: <20210702145954.GA4513@ubuntu>
From: John Wood <john.wood@gmx.com>
Date: Fri, 2 Jul 2021 16:59:54 +0200
> Hi,
>
> On Thu, Jul 01, 2021 at 11:55:14PM +0000, Alexander Lobakin wrote:
> > Hi,
> >
> > From: John Wood <john.wood@gmx.com>
> > Date: Sat, 5 Jun 2021 17:04:00 +0200
> >
> > > +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> > > +{
> > > + struct dentry *dentry = file_dentry(bprm->file);
> > > + struct inode *inode = file_inode(bprm->file);
> > > + struct brute_stats stats;
> > > + int rc;
> > > +
> > > + inode_lock(inode);
> > > + rc = brute_get_xattr_stats(dentry, inode, &stats);
> > > + if (WARN_ON_ONCE(rc && rc != -ENODATA))
> > > + goto unlock;
> >
> > I think I caught a problem here. Have you tested this with
> > initramfs?
>
> No, it has not been tested with initramfs :(
>
> > According to init/do_mount.c's
> > init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
> > parameter is not empty, kernel creates rootfs of type ramfs
> > (tmpfs otherwise).
> > The thing about ramfs is that it doesn't support xattrs.
>
> It is a known issue that systems without xattr support are not
> suitable for Brute (there are a note in the documentation).
> However, the purpose is not to panic the system :(
>
> > I'm running this v8 on a regular PC with initramfs and having
> > `root=` in cmdline, and Brute doesn't allow the kernel to run
> > any init processes (/init, /sbin/init, ...) with err == -95
> > (-EOPNOTSUPP) -- I'm getting a
> >
> > WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
> > <snip>
> > Failed to execute /init (error -95)
> >
> > and so on (and a panic at the end).
> >
> > If I omit `root=` from cmdline, then the kernel runs init process
> > just fine -- I guess because initramfs is then placed inside tmpfs
> > with xattr support.
> >
> > As for me, this ramfs/tmpfs selection based on `root=` presence
> > is ridiculous and I don't see or know any reasons behind that.
> > But that's another story, and ramfs might be not the only one
> > system without xattr support.
> > I think Brute should have a fallback here, e.g. it could simply
> > ignore files from xattr-incapable filesystems instead of such
> > WARNING splats and stuff.
>
> Ok, it seems reasonable to me: if the file system doesn't support
> xattr, but Brute is enabled, Brute will do nothing and the system
> will work normally.
On the other hand, it leaves a potentional window for attackers to
perform brute force from xattr-incapable filesystems. So at the end
of the day I think that the current implementation (a strong
rejection of such filesystems) is way more secure than having
a fallback I proposed.
I'm planning to make a patch which will eliminate such weird rootfs
type selection and just always use more feature-rich tmpfs if it's
compiled in. So, as an alternative, you could add it to your series
as a preparatory change and just add a Kconfig dependency on
CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
without messing with any fallbacks at all.
What do you think?
> I will work on it for the next version.
> Thanks for the feedback.
>
> John Wood
Thanks,
Al
^ permalink raw reply
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: John Wood @ 2021-07-02 14:59 UTC (permalink / raw)
To: Alexander Lobakin
Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap, Andrew Morton,
linux-doc, linux-kernel, linux-security-module, linux-kselftest,
linux-arch, linux-hardening, kernel-hardening
In-Reply-To: <20210701234807.50453-1-alobakin@pm.me>
Hi,
On Thu, Jul 01, 2021 at 11:55:14PM +0000, Alexander Lobakin wrote:
> Hi,
>
> From: John Wood <john.wood@gmx.com>
> Date: Sat, 5 Jun 2021 17:04:00 +0200
>
> > +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> > +{
> > + struct dentry *dentry = file_dentry(bprm->file);
> > + struct inode *inode = file_inode(bprm->file);
> > + struct brute_stats stats;
> > + int rc;
> > +
> > + inode_lock(inode);
> > + rc = brute_get_xattr_stats(dentry, inode, &stats);
> > + if (WARN_ON_ONCE(rc && rc != -ENODATA))
> > + goto unlock;
>
> I think I caught a problem here. Have you tested this with
> initramfs?
No, it has not been tested with initramfs :(
> According to init/do_mount.c's
> init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
> parameter is not empty, kernel creates rootfs of type ramfs
> (tmpfs otherwise).
> The thing about ramfs is that it doesn't support xattrs.
It is a known issue that systems without xattr support are not
suitable for Brute (there are a note in the documentation).
However, the purpose is not to panic the system :(
> I'm running this v8 on a regular PC with initramfs and having
> `root=` in cmdline, and Brute doesn't allow the kernel to run
> any init processes (/init, /sbin/init, ...) with err == -95
> (-EOPNOTSUPP) -- I'm getting a
>
> WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
> <snip>
> Failed to execute /init (error -95)
>
> and so on (and a panic at the end).
>
> If I omit `root=` from cmdline, then the kernel runs init process
> just fine -- I guess because initramfs is then placed inside tmpfs
> with xattr support.
>
> As for me, this ramfs/tmpfs selection based on `root=` presence
> is ridiculous and I don't see or know any reasons behind that.
> But that's another story, and ramfs might be not the only one
> system without xattr support.
> I think Brute should have a fallback here, e.g. it could simply
> ignore files from xattr-incapable filesystems instead of such
> WARNING splats and stuff.
Ok, it seems reasonable to me: if the file system doesn't support
xattr, but Brute is enabled, Brute will do nothing and the system
will work normally.
I will work on it for the next version.
Thanks for the feedback.
John Wood
^ permalink raw reply
* Re: [PATCH v2 6/6] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
From: Ahmad Fatoum @ 2021-07-02 12:33 UTC (permalink / raw)
To: Richard Weinberger
Cc: Jonathan Corbet, David Howells, Jarkko Sakkinen, James Bottomley,
Mimi Zohar, kernel, James Morris, Serge E. Hallyn, horia geanta,
aymen sghaier, Herbert Xu, davem, Udit Agarwal, Eric Biggers,
Jan Luebbe, david, Franck Lenormand, Sumit Garg,
open list, ASYMMETRIC KEYS, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM
In-Reply-To: <783613027.15909.1625223222889.JavaMail.zimbra@nod.at>
On 02.07.21 12:53, Richard Weinberger wrote:
> Ahmad,
>
> ----- Ursprüngliche Mail -----
>> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
>>> I'm still think that hard coding the key modifier is not wise.
>>> As I said[0], there are folks out there that want to provide their own modifier,
>>> so it is not only about being binary compatible with other CAAM blob patches in
>>> the wild.
>>
>> I don't think the characterization as a salt is accurate. AFAIU it's more
>> of a namespace, so blobs being loaded are "type-checked" against the modifier.
>
> Well, the CAAM programmer's reference manual states that the blob key is a 128 bit modifier
> and has two purposes:
> 1. It can be used as tag to provide separation between blobs to detect accidental replacement of blobs.
> 2. But it can also be treated as secret to provide additional protection. Because the blob encryption
> key derivation includes the key modifier.
>
> While you have case 1 in mind, I care about case 2. :-)
Ah, using the key modifier as a passphrase didn't occur to me.
>>> I'll happily implement that feature after your patches got merged but IMHO we
>>> should first agree on an interface.
>>> How about allowing another optional parameter to Opt_new and Opt_load
>>
>> Sound good to me. pcrlock for TPM trusted keys has the same interface.
>>
>> I'd prefer the new option to accept strings, not hex though.
>
> Both is possible. If the string starts with "0x" it needs to be decoded to a
> 128 bit key. Otherwise it has to be a up to 16 byte string.
Fine by me. Looking forward to your patches. :-)
Cheers,
Ahmad
>
> Thanks,
> //richard
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* RE: [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
From: Roberto Sassu @ 2021-07-02 10:54 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar@linux.ibm.com, paul@paul-moore.com
Cc: stephen.smalley.work@gmail.com, prsriva02@gmail.com,
tusharsu@linux.microsoft.com, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, selinux@vger.kernel.org
In-Reply-To: <25b1fe6f-a378-fbfb-821a-9ca2c3421b5c@linux.microsoft.com>
> From: Lakshmi Ramasubramanian [mailto:nramas@linux.microsoft.com]
> Sent: Thursday, July 1, 2021 7:27 PM
> On 7/1/2021 5:55 AM, Roberto Sassu wrote:
>
> Hi Roberto,
>
> > This patch adds the 'digest' and 'digest_len' parameters to
> > ima_measure_critical_data() and process_buffer_measurement(), so that
> > callers can get the digest of the passed buffer.
> >
> > These functions calculate the digest even if there is no suitable rule in
> > the IMA policy and, in this case, they simply return 1 before generating a
> > new measurement entry.
> >
>
> > diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> > index 3386e7436440..b4b1dc25e4fb 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
> > * @pcr: pcr to extend the measurement
> > * @func_data: func specific data, may be NULL
> > * @buf_hash: measure buffer data hash
> > + * @digest: buffer digest will be written to
> > + * @digest_len: buffer length
> > *
> > * Based on policy, either the buffer data or buffer data hash is measured
> > *
> > - * Return: 0 if the buffer has been successfully measured, a negative value
> > - * otherwise.
> > + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> > + * has been written to the passed location but not added to a measurement
> entry,
> > + * a negative value otherwise.
> > */
> > int process_buffer_measurement(struct user_namespace *mnt_userns,
> > struct inode *inode, const void *buf, int size,
> > const char *eventname, enum ima_hooks func,
> > int pcr, const char *func_data,
> > - bool buf_hash)
> > + bool buf_hash, u8 *digest, size_t digest_len)
> > {
> > int ret = 0;
> > const char *audit_cause = "ENOMEM";
> > @@ -869,7 +872,10 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > int action = 0;
> > u32 secid;
> >
> > - if (!ima_policy_flag)
> > + if (digest && digest_len < digest_hash_len)
> > + return -EINVAL;
> > +
> > + if (!ima_policy_flag && !digest)
> > return -ENOENT;
>
> Just wanted to check if you have verified this scenario:
>
> If ima_policy_flag is 0, the in-memory ima policy data is not yet
> initialized. In this case calling ima_get_action() will cause kernel
> panic (NULL exception).
>
> Please verify the above issue doesn't exist if the caller passes
> non-NULL digest and ima_policy_flag is 0 (ima policy is not initialized).
Yes, it is fixed with commit 067a436b1b0aa.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> thanks,
> -lakshmi
>
> >
> > template = ima_template_desc_buf();
> > @@ -891,7 +897,7 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > action = ima_get_action(mnt_userns, inode, current_cred(),
> > secid, 0, func, &pcr, &template,
> > func_data);
> > - if (!(action & IMA_MEASURE))
> > + if (!(action & IMA_MEASURE) && !digest)
> > return -ENOENT;
> > }
> >
> > @@ -922,6 +928,12 @@ int process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > event_data.buf_len = digest_hash_len;
> > }
> >
> > + if (digest)
> > + memcpy(digest, iint.ima_hash->digest, digest_hash_len);
> > +
> > + if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
> > + return 1;
> > +
> > ret = ima_alloc_init_template(&event_data, &entry, template);
> > if (ret < 0) {
> > audit_cause = "alloc_entry";
> > @@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> > file_inode(f.file), buf, size,
> > "kexec-cmdline", KEXEC_CMDLINE, 0,
> > - NULL, false);
> > + NULL, false, NULL, 0);
> > fdput(f);
> > }
> >
> > @@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > * @buf: pointer to buffer data
> > * @buf_len: length of buffer data (in bytes)
> > * @hash: measure buffer data hash
> > + * @digest: buffer digest will be written to
> > + * @digest_len: buffer length
> > *
> > * Measure data critical to the integrity of the kernel into the IMA log
> > * and extend the pcr. Examples of critical data could be various data
> > * structures, policies, and states stored in kernel memory that can
> > * impact the integrity of the system.
> > *
> > - * Return: 0 if the buffer has been successfully measured, a negative value
> > - * otherwise.
> > + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> > + * has been written to the passed location but not added to a measurement
> entry,
> > + * a negative value otherwise.
> > */
> > int ima_measure_critical_data(const char *event_label,
> > const char *event_name,
> > const void *buf, size_t buf_len,
> > - bool hash)
> > + bool hash, u8 *digest, size_t digest_len)
> > {
> > if (!event_name || !event_label || !buf || !buf_len)
> > return -ENOPARAM;
> >
> > return process_buffer_measurement(&init_user_ns, NULL, buf,
> buf_len,
> > event_name, CRITICAL_DATA, 0,
> > - event_label, hash);
> > + event_label, hash, digest,
> > + digest_len);
> > }
> >
> > static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> b/security/integrity/ima/ima_queue_keys.c
> > index e3047ce64f39..b02b061c5fac 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
> > entry-
> >keyring_name,
> > KEY_CHECK, 0,
> > entry-
> >keyring_name,
> > - false);
> > + false, NULL, 0);
> > list_del(&entry->list);
> > ima_free_key_entry(entry);
> > }
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index 4db9fa211638..d5d7b3ca9651 100644
> > --- a/security/selinux/ima.c
> > +++ b/security/selinux/ima.c
> > @@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >
> > measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> > state_str, strlen(state_str),
> > - false);
> > + false, NULL, 0);
> >
> > kfree(state_str);
> >
> > @@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> > }
> >
> > measure_rc = ima_measure_critical_data("selinux", "selinux-policy-
> hash",
> > - policy, policy_len, true);
> > + policy, policy_len, true,
> > + NULL, 0);
> >
> > vfree(policy);
> > }
> >
^ permalink raw reply
* Re: [PATCH v2 6/6] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
From: Richard Weinberger @ 2021-07-02 10:53 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Jonathan Corbet, David Howells, Jarkko Sakkinen, James Bottomley,
Mimi Zohar, kernel, James Morris, Serge E. Hallyn, horia geanta,
aymen sghaier, Herbert Xu, davem, Udit Agarwal, Eric Biggers,
Jan Luebbe, david, Franck Lenormand, Sumit Garg,
open list, ASYMMETRIC KEYS, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM
In-Reply-To: <2f608e5a-5a12-6db1-b9bd-a2cd9e3e3671@pengutronix.de>
Ahmad,
----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
>> I'm still think that hard coding the key modifier is not wise.
>> As I said[0], there are folks out there that want to provide their own modifier,
>> so it is not only about being binary compatible with other CAAM blob patches in
>> the wild.
>
> I don't think the characterization as a salt is accurate. AFAIU it's more
> of a namespace, so blobs being loaded are "type-checked" against the modifier.
Well, the CAAM programmer's reference manual states that the blob key is a 128 bit modifier
and has two purposes:
1. It can be used as tag to provide separation between blobs to detect accidental replacement of blobs.
2. But it can also be treated as secret to provide additional protection. Because the blob encryption
key derivation includes the key modifier.
While you have case 1 in mind, I care about case 2. :-)
>> I'll happily implement that feature after your patches got merged but IMHO we
>> should first agree on an interface.
>> How about allowing another optional parameter to Opt_new and Opt_load
>
> Sound good to me. pcrlock for TPM trusted keys has the same interface.
>
> I'd prefer the new option to accept strings, not hex though.
Both is possible. If the string starts with "0x" it needs to be decoded to a
128 bit key. Otherwise it has to be a up to 16 byte string.
Thanks,
//richard
^ permalink raw reply
* RE: [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
From: Roberto Sassu @ 2021-07-02 10:51 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar@linux.ibm.com, paul@paul-moore.com
Cc: stephen.smalley.work@gmail.com, prsriva02@gmail.com,
tusharsu@linux.microsoft.com, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, selinux@vger.kernel.org
In-Reply-To: <75bf7a3f-fd0e-177b-5725-e5f8b1f68966@linux.microsoft.com>
> From: Lakshmi Ramasubramanian [mailto:nramas@linux.microsoft.com]
> Sent: Thursday, July 1, 2021 6:16 PM
> On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> > ima_measure_critical_data() and process_buffer_measurement() currently
> > don't return a result. A caller wouldn't be able to know whether those
> > functions were executed successfully.
> >
> > This patch modifies the return type from void to int, and returns 0 if the
> > buffer has been successfully measured, a negative value otherwise.
> >
> > Also, this patch does not modify the behavior of existing callers by
> > processing the returned value. Instead, the value is stored in a variable
> > marked as __maybe_unused.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > include/linux/ima.h | 15 +++---
> > security/integrity/ima/ima.h | 10 ++--
> > security/integrity/ima/ima_appraise.c | 4 +-
> > security/integrity/ima/ima_asymmetric_keys.c | 4 +-
> > security/integrity/ima/ima_init.c | 6 ++-
> > security/integrity/ima/ima_main.c | 48 ++++++++++++--------
> > security/integrity/ima/ima_queue_keys.c | 15 +++---
> > security/selinux/ima.c | 10 ++--
> > 8 files changed, 66 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 81e830d01ced..60492263aa64 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct
> user_namespace *mnt_userns,
> > extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> > extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> > extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> > -extern void ima_measure_critical_data(const char *event_label,
> > - const char *event_name,
> > - const void *buf, size_t buf_len,
> > - bool hash);
> > +extern int ima_measure_critical_data(const char *event_label,
> > + const char *event_name,
> > + const void *buf, size_t buf_len,
> > + bool hash);
> >
> > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > extern void ima_appraise_parse_cmdline(void);
> > @@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode
> *inode, char *buf, size_t buf_size
> >
> > static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> {}
> >
> > -static inline void ima_measure_critical_data(const char *event_label,
> > +static inline int ima_measure_critical_data(const char *event_label,
> > const char *event_name,
> > const void *buf, size_t buf_len,
> > - bool hash) {}
> > + bool hash)
> > +{
> > + return -ENOENT;
> > +}
> >
> > #endif /* CONFIG_IMA */
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index f0e448ed1f9f..03db221324c3 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -264,11 +264,11 @@ void ima_store_measurement(struct
> integrity_iint_cache *iint, struct file *file,
> > struct evm_ima_xattr_data *xattr_value,
> > int xattr_len, const struct modsig *modsig, int pcr,
> > struct ima_template_desc *template_desc);
> > -void process_buffer_measurement(struct user_namespace *mnt_userns,
> > - struct inode *inode, const void *buf, int size,
> > - const char *eventname, enum ima_hooks
> func,
> > - int pcr, const char *func_data,
> > - bool buf_hash);
> > +int process_buffer_measurement(struct user_namespace *mnt_userns,
> > + struct inode *inode, const void *buf, int size,
> > + const char *eventname, enum ima_hooks func,
> > + int pcr, const char *func_data,
> > + bool buf_hash);
> > void ima_audit_measurement(struct integrity_iint_cache *iint,
> > const unsigned char *filename);
> > int ima_alloc_init_template(struct ima_event_data *event_data,
> > diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> > index ef9dcfce45d4..275a2377743f 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache
> *iint,
> > enum hash_algo hash_algo;
> > const u8 *digest = NULL;
> > u32 digestsize = 0;
> > + int process_rc __maybe_unused;
> > int rc = 0;
> >
> > if (!(iint->flags & IMA_CHECK_BLACKLIST))
> > @@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache
> *iint,
> >
> > rc = is_binary_blacklisted(digest, digestsize);
> > if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> > - process_buffer_measurement(&init_user_ns, NULL,
> digest, digestsize,
> > + process_rc =
> process_buffer_measurement(&init_user_ns,
> I think there is no need to make this change now. If and when
> ima_check_blacklist() needs to look at the return value of p_b_m(), this
> change can be made.
Hi Lakshmi
ok. I was worried about possible warnings. If it is not an issue,
I will remove the assignment.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > + NULL, digest, digestsize,
> > "blacklisted-hash", NONE,
> > pcr, NULL, false);
> > }
> > diff --git a/security/integrity/ima/ima_asymmetric_keys.c
> b/security/integrity/ima/ima_asymmetric_keys.c
> > index c985418698a4..910367cdd920 100644
> > --- a/security/integrity/ima/ima_asymmetric_keys.c
> > +++ b/security/integrity/ima/ima_asymmetric_keys.c
> > @@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key
> *keyring, struct key *key,
> > unsigned long flags, bool create)
> > {
> > bool queued = false;
> > + int ret __maybe_unused;
> >
> > /* Only asymmetric keys are handled by this hook. */
> > if (key->type != &key_type_asymmetric)
> > @@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key
> *keyring, struct key *key,
> > * if the IMA policy is configured to measure a key linked
> > * to the given keyring.
> > */
> > - process_buffer_measurement(&init_user_ns, NULL, payload,
> payload_len,
> > + ret = process_buffer_measurement(&init_user_ns, NULL,
> > + payload, payload_len,
> Same comment as in ima_check_blacklist() - this change be made when
> needed.
>
> > keyring->description, KEY_CHECK, 0,
> > keyring->description, false);
> > }
> > diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> > index 5076a7d9d23e..6790eea88db8 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -118,6 +118,7 @@ void __init ima_load_x509(void)
> >
> > int __init ima_init(void)
> > {
> > + int measure_rc __maybe_unused;
> > int rc;
> >
> > ima_tpm_chip = tpm_default_chip();
> > @@ -153,8 +154,9 @@ int __init ima_init(void)
> >
> > ima_init_key_queue();
> >
> > - ima_measure_critical_data("kernel_info", "kernel_version",
> > - UTS_RELEASE, strlen(UTS_RELEASE), false);
> > + measure_rc = ima_measure_critical_data("kernel_info",
> "kernel_version",
> > + UTS_RELEASE,
> strlen(UTS_RELEASE),
> > + false);
> Same comment as in ima_check_blacklist() - this change be made when
> needed.
>
> >
> > return rc;
> > }
> > diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> > index 8ef1fa357e0c..3386e7436440 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
> > return 0;
> > }
> >
> > -/*
> > +/**
> > * process_buffer_measurement - Measure the buffer or the buffer data
> hash
> > * @mnt_userns: user namespace of the mount the inode was found
> from
> > * @inode: inode associated with the object being measured (NULL for
> KEY_CHECK)
> > @@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
> > * @buf_hash: measure buffer data hash
> > *
> > * Based on policy, either the buffer data or buffer data hash is measured
> > + *
> > + * Return: 0 if the buffer has been successfully measured, a negative value
> > + * otherwise.
> > */
> > -void process_buffer_measurement(struct user_namespace *mnt_userns,
> > - struct inode *inode, const void *buf, int size,
> > - const char *eventname, enum ima_hooks
> func,
> > - int pcr, const char *func_data,
> > - bool buf_hash)
> > +int process_buffer_measurement(struct user_namespace *mnt_userns,
> > + struct inode *inode, const void *buf, int size,
> > + const char *eventname, enum ima_hooks func,
> > + int pcr, const char *func_data,
> > + bool buf_hash)
> > {
> > int ret = 0;
> > const char *audit_cause = "ENOMEM";
> > @@ -867,7 +870,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > u32 secid;
> >
> > if (!ima_policy_flag)
> > - return;
> > + return -ENOENT;
> >
> > template = ima_template_desc_buf();
> > if (!template) {
> > @@ -889,7 +892,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > secid, 0, func, &pcr, &template,
> > func_data);
> > if (!(action & IMA_MEASURE))
> > - return;
> > + return -ENOENT;
> > }
> >
> > if (!pcr)
> > @@ -937,7 +940,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > func_measure_str(func),
> > audit_cause, ret, 0, ret);
> >
> > - return;
> > + return ret;
> > }
> >
> > /**
> > @@ -951,6 +954,7 @@ void process_buffer_measurement(struct
> user_namespace *mnt_userns,
> > void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> > {
> > struct fd f;
> > + int ret __maybe_unused;
> >
> > if (!buf || !size)
> > return;
> > @@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > if (!f.file)
> > return;
> >
> > - process_buffer_measurement(file_mnt_user_ns(f.file),
> file_inode(f.file),
> > - buf, size, "kexec-cmdline", KEXEC_CMDLINE,
> 0,
> > - NULL, false);
> > + ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> > + file_inode(f.file), buf, size,
> > + "kexec-cmdline", KEXEC_CMDLINE, 0,
> > + NULL, false);
> Since the return value of p_b_m() is not used here, this change can be
> made when needed.
>
> > fdput(f);
> > }
> >
> > @@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > * and extend the pcr. Examples of critical data could be various data
> > * structures, policies, and states stored in kernel memory that can
> > * impact the integrity of the system.
> > + *
> > + * Return: 0 if the buffer has been successfully measured, a negative value
> > + * otherwise.
> > */
> > -void ima_measure_critical_data(const char *event_label,
> > - const char *event_name,
> > - const void *buf, size_t buf_len,
> > - bool hash)
> > +int ima_measure_critical_data(const char *event_label,
> > + const char *event_name,
> > + const void *buf, size_t buf_len,
> > + bool hash)
> > {
> > if (!event_name || !event_label || !buf || !buf_len)
> > - return;
> > + return -ENOPARAM;
> >
> > - process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
> event_name,
> > - CRITICAL_DATA, 0, event_label,
> > - hash);
> > + return process_buffer_measurement(&init_user_ns, NULL, buf,
> buf_len,
> > + event_name, CRITICAL_DATA, 0,
> > + event_label, hash);
> > }
> >
> > static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> b/security/integrity/ima/ima_queue_keys.c
> > index 979ef6c71f3d..e3047ce64f39 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
> > {
> > struct ima_key_entry *entry, *tmp;
> > bool process = false;
> > + int ret __maybe_unused;
> >
> > if (ima_process_keys)
> > return;
> > @@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
> >
> > list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
> > if (!timer_expired)
> > - process_buffer_measurement(&init_user_ns, NULL,
> > - entry->payload,
> > - entry->payload_len,
> > - entry->keyring_name,
> > - KEY_CHECK, 0,
> > - entry->keyring_name,
> > - false);
> > + ret = process_buffer_measurement(&init_user_ns,
> NULL,
> > + entry->payload,
> > + entry->payload_len,
> > + entry-
> >keyring_name,
> > + KEY_CHECK, 0,
> > + entry-
> >keyring_name,
> > + false);
> Same comment as above.
>
> > list_del(&entry->list);
> > ima_free_key_entry(entry);
> > }
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index 34d421861bfc..4db9fa211638 100644
> > --- a/security/selinux/ima.c
> > +++ b/security/selinux/ima.c
> > @@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> > char *state_str = NULL;
> > void *policy = NULL;
> > size_t policy_len;
> > + int measure_rc __maybe_unused;
> > int rc = 0;
> >
> > WARN_ON(!mutex_is_locked(&state->policy_mutex));
> > @@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> > return;
> > }
> >
> > - ima_measure_critical_data("selinux", "selinux-state",
> > - state_str, strlen(state_str), false);
> > + measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> > + state_str, strlen(state_str),
> > + false);
> Since the return value of ima_measure_critical_data() is not used here,
> this change can be made when needed.
>
> >
> > kfree(state_str);
> >
> > @@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> > return;
> > }
> >
> > - ima_measure_critical_data("selinux", "selinux-policy-hash",
> > - policy, policy_len, true);
> > + measure_rc = ima_measure_critical_data("selinux", "selinux-policy-
> hash",
> > + policy, policy_len, true);
> Same comment as above.
>
> -lakshmi
>
> >
> > vfree(policy);
> > }
> >
^ permalink raw reply
* Re: [PATCH v2 5/6] crypto: caam - add in-kernel interface for blob generator
From: Ahmad Fatoum @ 2021-07-02 8:03 UTC (permalink / raw)
To: Horia Geantă, Aymen Sghaier, Herbert Xu, David S. Miller
Cc: kernel, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, James Morris, Eric Biggers, Serge E. Hallyn,
Udit Agarwal, Jan Luebbe, David Gstir, Richard Weinberger,
Franck LENORMAND, Sumit Garg, linux-integrity, keyrings,
linux-crypto, linux-kernel, linux-security-module
In-Reply-To: <c65e43cc6a8c60877a183cdea5c9f7e43b8a4434.1624364386.git-series.a.fatoum@pengutronix.de>
Hello Horia,
Hello Aymen,
On 22.06.21 14:37, Ahmad Fatoum wrote:
> The CAAM can be used to protect user-defined data across system reboot:
>
> - When the system is fused and boots into secure state, the master
> key is a unique never-disclosed device-specific key
> - random key is encrypted by key derived from master key
> - data is encrypted using the random key
> - encrypted data and its encrypted random key are stored alongside
> - This blob can now be safely stored in non-volatile memory
>
> On next power-on:
> - blob is loaded into CAAM
> - CAAM writes decrypted data either into memory or key register
>
> Add functions to realize encrypting and decrypting into memory alongside
> the CAAM driver.
>
> They will be used in a later commit as a source for the trusted key
> seal/unseal mechanism.
Are you ok with this patch and security/keys/trusted-keys/trusted_caam.c
introduced in the follow-up commit?
Cheers,
Ahmad
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> To: "Horia Geantă" <horia.geanta@nxp.com>
> To: Aymen Sghaier <aymen.sghaier@nxp.com>
> To: Herbert Xu <herbert@gondor.apana.org.au>
> To: "David S. Miller" <davem@davemloft.net>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Udit Agarwal <udit.agarwal@nxp.com>
> Cc: Jan Luebbe <j.luebbe@pengutronix.de>
> Cc: David Gstir <david@sigma-star.at>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Franck LENORMAND <franck.lenormand@nxp.com>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: linux-integrity@vger.kernel.org
> Cc: keyrings@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> ---
> drivers/crypto/caam/Kconfig | 3 +-
> drivers/crypto/caam/Makefile | 1 +-
> drivers/crypto/caam/blob_gen.c | 230 ++++++++++++++++++++++++++++++++++-
> include/soc/fsl/caam-blob.h | 56 ++++++++-
> 4 files changed, 290 insertions(+)
> create mode 100644 drivers/crypto/caam/blob_gen.c
> create mode 100644 include/soc/fsl/caam-blob.h
>
> diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
> index 84ea7cba5ee5..ea9f8b1ae981 100644
> --- a/drivers/crypto/caam/Kconfig
> +++ b/drivers/crypto/caam/Kconfig
> @@ -151,6 +151,9 @@ config CRYPTO_DEV_FSL_CAAM_RNG_API
> Selecting this will register the SEC4 hardware rng to
> the hw_random API for supplying the kernel entropy pool.
>
> +config CRYPTO_DEV_FSL_CAAM_BLOB_GEN
> + bool
> +
> endif # CRYPTO_DEV_FSL_CAAM_JR
>
> endif # CRYPTO_DEV_FSL_CAAM
> diff --git a/drivers/crypto/caam/Makefile b/drivers/crypto/caam/Makefile
> index 3570286eb9ce..25f7ae5a4642 100644
> --- a/drivers/crypto/caam/Makefile
> +++ b/drivers/crypto/caam/Makefile
> @@ -21,6 +21,7 @@ caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += caamalg_qi.o
> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_AHASH_API) += caamhash.o
> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API) += caamrng.o
> caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_PKC_API) += caampkc.o pkc_desc.o
> +caam_jr-$(CONFIG_CRYPTO_DEV_FSL_CAAM_BLOB_GEN) += blob_gen.o
>
> caam-$(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI) += qi.o
> ifneq ($(CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI),)
> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
> new file mode 100644
> index 000000000000..513d3f90e438
> --- /dev/null
> +++ b/drivers/crypto/caam/blob_gen.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de>
> + * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
> + */
> +
> +#include <linux/device.h>
> +#include <soc/fsl/caam-blob.h>
> +
> +#include "compat.h"
> +#include "desc_constr.h"
> +#include "desc.h"
> +#include "error.h"
> +#include "intern.h"
> +#include "jr.h"
> +#include "regs.h"
> +
> +struct caam_blob_priv {
> + struct device jrdev;
> +};
> +
> +struct caam_blob_job_result {
> + int err;
> + struct completion completion;
> +};
> +
> +static void caam_blob_job_done(struct device *dev, u32 *desc, u32 err, void *context)
> +{
> + struct caam_blob_job_result *res = context;
> + int ecode = 0;
> +
> + dev_dbg(dev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
> +
> + if (err)
> + ecode = caam_jr_strstatus(dev, err);
> +
> + res->err = ecode;
> +
> + /*
> + * Upon completion, desc points to a buffer containing a CAAM job
> + * descriptor which encapsulates data into an externally-storable
> + * blob.
> + */
> + complete(&res->completion);
> +}
> +
> +static u32 *caam_blob_alloc_desc(size_t keymod_len)
> +{
> + size_t len;
> +
> + /* header + (key mod immediate) + 2x pointers + op */
> + len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4;
> +
> + if (len > CAAM_DESC_BYTES_MAX)
> + return NULL;
> +
> + return kzalloc(len, GFP_KERNEL | GFP_DMA);
> +}
> +
> +int caam_encap_blob(struct caam_blob_priv *priv, const char *keymod,
> + void *input, void *output, size_t length)
> +{
> + u32 *desc;
> + struct device *jrdev = &priv->jrdev;
> + dma_addr_t dma_in, dma_out;
> + struct caam_blob_job_result testres;
> + size_t keymod_len = strlen(keymod);
> + int ret;
> +
> + if (length <= CAAM_BLOB_OVERHEAD || keymod_len > CAAM_BLOB_KEYMOD_LENGTH)
> + return -EINVAL;
> +
> + desc = caam_blob_alloc_desc(keymod_len);
> + if (!desc) {
> + dev_err(jrdev, "unable to allocate desc\n");
> + return -ENOMEM;
> + }
> +
> + dma_in = dma_map_single(jrdev, input, length - CAAM_BLOB_OVERHEAD, DMA_TO_DEVICE);
> + if (dma_mapping_error(jrdev, dma_in)) {
> + dev_err(jrdev, "unable to map input DMA buffer\n");
> + ret = -ENOMEM;
> + goto out_free;
> + }
> +
> + dma_out = dma_map_single(jrdev, output, length, DMA_FROM_DEVICE);
> + if (dma_mapping_error(jrdev, dma_out)) {
> + dev_err(jrdev, "unable to map output DMA buffer\n");
> + ret = -ENOMEM;
> + goto out_unmap_in;
> + }
> +
> + /*
> + * A data blob is encrypted using a blob key (BK); a random number.
> + * The BK is used as an AES-CCM key. The initial block (B0) and the
> + * initial counter (Ctr0) are generated automatically and stored in
> + * Class 1 Context DWords 0+1+2+3. The random BK is stored in the
> + * Class 1 Key Register. Operation Mode is set to AES-CCM.
> + */
> +
> + init_job_desc(desc, 0);
> + append_key_as_imm(desc, keymod, keymod_len, keymod_len,
> + CLASS_2 | KEY_DEST_CLASS_REG);
> + append_seq_in_ptr_intlen(desc, dma_in, length - CAAM_BLOB_OVERHEAD, 0);
> + append_seq_out_ptr_intlen(desc, dma_out, length, 0);
> + append_operation(desc, OP_TYPE_ENCAP_PROTOCOL | OP_PCLID_BLOB);
> +
> + print_hex_dump_debug("data@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, input,
> + length - CAAM_BLOB_OVERHEAD, false);
> + print_hex_dump_debug("jobdesc@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, desc,
> + desc_bytes(desc), false);
> +
> + testres.err = 0;
> + init_completion(&testres.completion);
> +
> + ret = caam_jr_enqueue(jrdev, desc, caam_blob_job_done, &testres);
> + if (ret == -EINPROGRESS) {
> + wait_for_completion(&testres.completion);
> + ret = testres.err;
> + print_hex_dump_debug("output@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, output,
> + length, false);
> + }
> +
> + dma_unmap_single(jrdev, dma_out, length, DMA_FROM_DEVICE);
> +out_unmap_in:
> + dma_unmap_single(jrdev, dma_in, length - CAAM_BLOB_OVERHEAD, DMA_TO_DEVICE);
> +out_free:
> + kfree(desc);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(caam_encap_blob);
> +
> +int caam_decap_blob(struct caam_blob_priv *priv, const char *keymod,
> + void *input, void *output, size_t length)
> +{
> + u32 *desc;
> + struct device *jrdev = &priv->jrdev;
> + dma_addr_t dma_in, dma_out;
> + struct caam_blob_job_result testres;
> + size_t keymod_len = strlen(keymod);
> + int ret;
> +
> + if (length <= CAAM_BLOB_OVERHEAD || keymod_len > CAAM_BLOB_KEYMOD_LENGTH)
> + return -EINVAL;
> +
> + desc = caam_blob_alloc_desc(keymod_len);
> + if (!desc) {
> + dev_err(jrdev, "unable to allocate desc\n");
> + return -ENOMEM;
> + }
> +
> + dma_in = dma_map_single(jrdev, input, length, DMA_TO_DEVICE);
> + if (dma_mapping_error(jrdev, dma_in)) {
> + dev_err(jrdev, "unable to map input DMA buffer\n");
> + ret = -ENOMEM;
> + goto out_free;
> + }
> +
> + dma_out = dma_map_single(jrdev, output, length - CAAM_BLOB_OVERHEAD, DMA_FROM_DEVICE);
> + if (dma_mapping_error(jrdev, dma_out)) {
> + dev_err(jrdev, "unable to map output DMA buffer\n");
> + ret = -ENOMEM;
> + goto out_unmap_in;
> + }
> +
> + /*
> + * A data blob is encrypted using a blob key (BK); a random number.
> + * The BK is used as an AES-CCM key. The initial block (B0) and the
> + * initial counter (Ctr0) are generated automatically and stored in
> + * Class 1 Context DWords 0+1+2+3. The random BK is stored in the
> + * Class 1 Key Register. Operation Mode is set to AES-CCM.
> + */
> +
> + init_job_desc(desc, 0);
> + append_key_as_imm(desc, keymod, keymod_len, keymod_len,
> + CLASS_2 | KEY_DEST_CLASS_REG);
> + append_seq_in_ptr(desc, dma_in, length, 0);
> + append_seq_out_ptr(desc, dma_out, length - CAAM_BLOB_OVERHEAD, 0);
> + append_operation(desc, OP_TYPE_DECAP_PROTOCOL | OP_PCLID_BLOB);
> +
> + print_hex_dump_debug("data@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, input,
> + length, false);
> + print_hex_dump_debug("jobdesc@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, desc,
> + desc_bytes(desc), false);
> +
> + testres.err = 0;
> + init_completion(&testres.completion);
> +
> + ret = caam_jr_enqueue(jrdev, desc, caam_blob_job_done, &testres);
> + if (ret == -EINPROGRESS) {
> + wait_for_completion(&testres.completion);
> + ret = testres.err;
> + print_hex_dump_debug("output@"__stringify(__LINE__)": ",
> + DUMP_PREFIX_ADDRESS, 16, 1, output,
> + length - CAAM_BLOB_OVERHEAD, false);
> + }
> +
> + dma_unmap_single(jrdev, dma_out, length - CAAM_BLOB_OVERHEAD, DMA_FROM_DEVICE);
> +out_unmap_in:
> + dma_unmap_single(jrdev, dma_in, length, DMA_TO_DEVICE);
> +out_free:
> + kfree(desc);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(caam_decap_blob);
> +
> +struct caam_blob_priv *caam_blob_gen_init(void)
> +{
> + struct device *jrdev;
> +
> + jrdev = caam_jr_alloc();
> + if (IS_ERR(jrdev))
> + return ERR_CAST(jrdev);
> +
> + return container_of(jrdev, struct caam_blob_priv, jrdev);
> +}
> +EXPORT_SYMBOL(caam_blob_gen_init);
> +
> +void caam_blob_gen_exit(struct caam_blob_priv *priv)
> +{
> + caam_jr_free(&priv->jrdev);
> +}
> +EXPORT_SYMBOL(caam_blob_gen_exit);
> diff --git a/include/soc/fsl/caam-blob.h b/include/soc/fsl/caam-blob.h
> new file mode 100644
> index 000000000000..aebbc9335f64
> --- /dev/null
> +++ b/include/soc/fsl/caam-blob.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
> + */
> +
> +#ifndef __CAAM_BLOB_GEN
> +#define __CAAM_BLOB_GEN
> +
> +#include <linux/types.h>
> +
> +#define CAAM_BLOB_KEYMOD_LENGTH 16
> +#define CAAM_BLOB_OVERHEAD (32 + 16)
> +#define CAAM_BLOB_MAX_LEN 4096
> +
> +struct caam_blob_priv;
> +
> +/** caam_blob_gen_init - initialize blob generation
> + *
> + * returns either pointer to new caam_blob_priv instance
> + * or error pointer
> + */
> +struct caam_blob_priv *caam_blob_gen_init(void);
> +
> +/** caam_blob_gen_init - free blob generation resources
> + *
> + * @priv: instance returned by caam_blob_gen_init
> + */
> +void caam_blob_gen_exit(struct caam_blob_priv *priv);
> +
> +/** caam_encap_blob - encapsulate blob
> + *
> + * @priv: instance returned by caam_blob_gen_init
> + * @keymod: string to use as key modifier for blob encapsulation
> + * can't be longer than CAAM_BLOB_KEYMOD_LENGTH
> + * @input: buffer which CAAM will DMA from
> + * @output: buffer which CAAM will DMA to
> + * @length: buffer length including blob overhead
> + * CAAM_BLOB_OVERHEAD < length <= CAAM_BLOB_MAX_LEN
> + */
> +int caam_encap_blob(struct caam_blob_priv *priv, const char *keymod,
> + void *input, void *output, size_t length);
> +
> +/** caam_decap_blob - decapsulate blob
> + *
> + * @priv: instance returned by caam_blob_gen_init
> + * @keymod: string to use as key modifier for blob decapsulation
> + * can't be longer than CAAM_BLOB_KEYMOD_LENGTH
> + * @input: buffer which CAAM will DMA from
> + * @output: buffer which CAAM will DMA to
> + * @length: buffer length including blob overhead
> + * CAAM_BLOB_OVERHEAD < length <= CAAM_BLOB_MAX_LEN
> + */
> +int caam_decap_blob(struct caam_blob_priv *priv, const char *keymod,
> + void *input, void *output, size_t length);
> +
> +#endif
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v2 6/6] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
From: Ahmad Fatoum @ 2021-07-02 8:00 UTC (permalink / raw)
To: Richard Weinberger
Cc: Jonathan Corbet, David Howells, Jarkko Sakkinen, James Bottomley,
Mimi Zohar, kernel, James Morris, Serge E. Hallyn, horia geanta,
aymen sghaier, Herbert Xu, davem, Udit Agarwal, Eric Biggers,
Jan Luebbe, david, Franck Lenormand, Sumit Garg,
open list, ASYMMETRIC KEYS, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM
In-Reply-To: <1850833581.13438.1625172175436.JavaMail.zimbra@nod.at>
Hello Richard,
On 01.07.21 22:42, Richard Weinberger wrote:
> Ahmad,
>
> ----- Ursprüngliche Mail -----
>> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
>> +static struct caam_blob_priv *blobifier;
>> +
>> +#define KEYMOD "kernel:trusted"
>
> I'm still think that hard coding the key modifier is not wise.
> As I said[0], there are folks out there that want to provide their own modifier,
> so it is not only about being binary compatible with other CAAM blob patches in the wild.
I don't think the characterization as a salt is accurate. AFAIU it's more
of a namespace, so blobs being loaded are "type-checked" against the modifier.
> I'll happily implement that feature after your patches got merged but IMHO we should first agree on an interface.
> How about allowing another optional parameter to Opt_new and Opt_load
Sound good to me. pcrlock for TPM trusted keys has the same interface.
I'd prefer the new option to accept strings, not hex though.
> and having a key modifier per struct trusted_key_payload instance?
Ye, possibly a void *backend_data, which other trust sources could leverage
as well. But that should be separate discussion.
Cheers,
Ahmad
>
> Thanks,
> //richard
>
> [0]
> https://patchwork.kernel.org/project/linux-crypto/patch/319e558e1bd19b80ad6447c167a2c3942bdafea2.1615914058.git-series.a.fatoum@pengutronix.de/#24085397
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v2] perf: Require CAP_KILL if sigtrap is requested
From: Marco Elver @ 2021-07-02 7:20 UTC (permalink / raw)
To: Eric W. Biederman
Cc: peterz, tglx, mingo, kasan-dev, linux-kernel, mingo, acme,
mark.rutland, alexander.shishkin, jolsa, namhyung,
linux-perf-users, omosnace, serge, linux-security-module, stable,
Dmitry Vyukov
In-Reply-To: <87h7hdn24k.fsf@disp2133>
On Thu, 1 Jul 2021 at 23:41, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Marco Elver <elver@google.com> writes:
>
> > If perf_event_open() is called with another task as target and
> > perf_event_attr::sigtrap is set, and the target task's user does not
> > match the calling user, also require the CAP_KILL capability.
> >
> > Otherwise, with the CAP_PERFMON capability alone it would be possible
> > for a user to send SIGTRAP signals via perf events to another user's
> > tasks. This could potentially result in those tasks being terminated if
> > they cannot handle SIGTRAP signals.
> >
> > Note: The check complements the existing capability check, but is not
> > supposed to supersede the ptrace_may_access() check. At a high level we
> > now have:
> >
> > capable of CAP_PERFMON and (CAP_KILL if sigtrap)
> > OR
> > ptrace_may_access() // also checks for same thread-group and uid
>
> Is there anyway we could have a comment that makes the required
> capability checks clear?
>
> Basically I see an inlined version of kill_ok_by_cred being implemented
> without the comments on why the various pieces make sense.
I'll add more comments. It probably also makes sense to factor the
code here into its own helper.
> Certainly ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS) should not
> be a check to allow writing/changing a task. It needs to be
> PTRACE_MODE_ATTACH_REALCREDS, like /proc/self/mem uses.
So if attr.sigtrap the checked ptrace mode needs to switch to
PTRACE_MODE_ATTACH_REALCREDS. Otherwise, it is possible to send a
signal if only read-ptrace permissions are granted.
Is my assumption here correct?
> Now in practice I think your patch probably has the proper checks in
> place for sending a signal but it is far from clear.
Thanks,
-- Marco
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox