* [PATCH v10 07/12] ima: Add modsig appraise_type option for module-style appended signatures
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Introduce the modsig keyword to the IMA policy syntax to specify that
a given hook should expect the file to have the IMA signature appended
to it. Here is how it can be used in a rule:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
With this rule, IMA will accept either a signature stored in the extended
attribute or an appended signature.
For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified. The actual modsig implementation
will be introduced separately.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
Documentation/ABI/testing/ima_policy | 6 +++++-
security/integrity/ima/Kconfig | 10 +++++++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9 ++++++++
security/integrity/ima/ima_modsig.c | 31 ++++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 12 +++++++++--
security/integrity/integrity.h | 1 +
7 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..9d1dfd0a8891 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -37,7 +37,7 @@ Description:
euid:= decimal value
fowner:= decimal value
lsm: are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] [imasig|modsig]
pcr:= decimal value
default policy:
@@ -103,3 +103,7 @@ Description:
measure func=KEXEC_KERNEL_CHECK pcr=4
measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+ Example of appraise rule allowing modsig appended signatures:
+
+ appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..bba19f9ea184 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -231,6 +231,16 @@ config IMA_APPRAISE_BOOTPARAM
This option enables the different "ima_appraise=" modes
(eg. fix, log) from the boot command line.
+config IMA_APPRAISE_MODSIG
+ bool "Support module-style signatures for appraisal"
+ depends on IMA_APPRAISE
+ default n
+ help
+ Adds support for signatures appended to files. The format of the
+ appended signature is the same used for signed kernel modules.
+ The modsig keyword can be used in the IMA policy to allow a hook
+ to accept such signatures.
+
config IMA_TRUSTED_KEYRING
bool "Require all keys on the .ima keyring be signed (deprecated)"
depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..31d57cdf2421 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_IMA) += ima.o
ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_template.o ima_template_lib.o
ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..0c3e5a59270f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -293,6 +293,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#else
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ return false;
+}
+#endif /* CONFIG_IMA_APPRAISE_MODSIG */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
new file mode 100644
index 000000000000..87503bfe8c8b
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2019 IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann <bauerman@linux.ibm.com>
+ */
+
+#include "ima.h"
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file(), because only
+ * they preload the contents of the file in a buffer. FILE_CHECK does that in
+ * some cases, but not when reached from vfs_open(). POLICY_CHECK can support
+ * it, but it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ switch (func) {
+ case KEXEC_KERNEL_CHECK:
+ case KEXEC_INITRAMFS_CHECK:
+ case MODULE_CHECK:
+ return true;
+ default:
+ return false;
+ }
+}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..fca7a3f23321 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1038,6 +1038,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
ima_log_string(ab, "appraise_type", args[0].from);
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
+ else if (ima_hook_supports_modsig(entry->func) &&
+ strcmp(args[0].from, "imasig|modsig") == 0)
+ entry->flags |= IMA_DIGSIG_REQUIRED
+ | IMA_MODSIG_ALLOWED;
else
result = -EINVAL;
break;
@@ -1330,8 +1334,12 @@ int ima_policy_show(struct seq_file *m, void *v)
}
}
}
- if (entry->flags & IMA_DIGSIG_REQUIRED)
- seq_puts(m, "appraise_type=imasig ");
+ if (entry->flags & IMA_DIGSIG_REQUIRED) {
+ if (entry->flags & IMA_MODSIG_ALLOWED)
+ seq_puts(m, "appraise_type=imasig|modsig ");
+ else
+ seq_puts(m, "appraise_type=imasig ");
+ }
if (entry->flags & IMA_PERMIT_DIRECTIO)
seq_puts(m, "permit_directio ");
rcu_read_unlock();
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 88a29f72a74f..0e7330a36a9d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -36,6 +36,7 @@
#define IMA_NEW_FILE 0x04000000
#define EVM_IMMUTABLE_DIGSIG 0x08000000
#define IMA_FAIL_UNVERIFIABLE_SIGS 0x10000000
+#define IMA_MODSIG_ALLOWED 0x20000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
IMA_HASH | IMA_APPRAISE_SUBMASK)
^ permalink raw reply related
* [PATCH v10 09/12] ima: Implement support for module-style appended signatures
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Implement the appraise_type=imasig|modsig option, allowing IMA to read and
verify modsig signatures.
In case a file has both an xattr signature and an appended modsig, IMA will
only use the appended signature if the key used by the xattr signature
isn't present in the IMA or platform keyring.
Because modsig verification needs to convert from an integrity keyring id
to the keyring itself, add an integrity_keyring_from_id() function in
digsig.c so that integrity_modsig_verify() can use it.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
security/integrity/digsig.c | 39 ++++++++++++---
security/integrity/ima/Kconfig | 3 ++
security/integrity/ima/ima.h | 22 ++++++++-
security/integrity/ima/ima_appraise.c | 50 +++++++++++++++++--
security/integrity/ima/ima_main.c | 11 ++++-
security/integrity/ima/ima_modsig.c | 71 +++++++++++++++++++++++++++
security/integrity/ima/ima_policy.c | 12 ++---
security/integrity/integrity.h | 19 +++++++
8 files changed, 206 insertions(+), 21 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e19c2eb72c51..ce79d2f6bc7e 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -43,11 +43,10 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
#define restrict_link_to_ima restrict_link_by_builtin_trusted
#endif
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
- const char *digest, int digestlen)
+static struct key *integrity_keyring_from_id(const unsigned int id)
{
- if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
- return -EINVAL;
+ if (id >= INTEGRITY_KEYRING_MAX)
+ return ERR_PTR(-EINVAL);
if (!keyring[id]) {
keyring[id] =
@@ -56,23 +55,49 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
int err = PTR_ERR(keyring[id]);
pr_err("no %s keyring: %d\n", keyring_name[id], err);
keyring[id] = NULL;
- return err;
+ return ERR_PTR(err);
}
}
+ return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+ const char *digest, int digestlen)
+{
+ struct key *keyring;
+
+ if (siglen < 2)
+ return -EINVAL;
+
+ keyring = integrity_keyring_from_id(id);
+ if (IS_ERR(keyring))
+ return PTR_ERR(keyring);
+
switch (sig[1]) {
case 1:
/* v1 API expect signature without xattr type */
- return digsig_verify(keyring[id], sig + 1, siglen - 1,
+ return digsig_verify(keyring, sig + 1, siglen - 1,
digest, digestlen);
case 2:
- return asymmetric_verify(keyring[id], sig, siglen,
+ return asymmetric_verify(keyring, sig, siglen,
digest, digestlen);
}
return -EOPNOTSUPP;
}
+int integrity_modsig_verify(const unsigned int id, const struct modsig *modsig)
+{
+ struct key *keyring;
+
+ keyring = integrity_keyring_from_id(id);
+ if (IS_ERR(keyring))
+ return PTR_ERR(keyring);
+
+ return ima_modsig_verify(keyring, modsig);
+}
+
static int __integrity_init_keyring(const unsigned int id, key_perm_t perm,
struct key_restriction *restriction)
{
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index bba19f9ea184..0fb542455698 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -234,6 +234,9 @@ config IMA_APPRAISE_BOOTPARAM
config IMA_APPRAISE_MODSIG
bool "Support module-style signatures for appraisal"
depends on IMA_APPRAISE
+ depends on INTEGRITY_ASYMMETRIC_KEYS
+ select PKCS7_MESSAGE_PARSER
+ select MODULE_SIG_FORMAT
default n
help
Adds support for signatures appended to files. The format of the
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 0c3e5a59270f..9f69befd8674 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -191,6 +191,10 @@ enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
};
+extern const char *const func_tokens[];
+
+struct modsig;
+
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr);
@@ -240,7 +244,7 @@ int ima_appraise_measurement(enum ima_hooks func,
struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len);
+ int xattr_len, const struct modsig *modsig);
int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -256,7 +260,8 @@ static inline int ima_appraise_measurement(enum ima_hooks func,
struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len)
+ int xattr_len,
+ const struct modsig *modsig)
{
return INTEGRITY_UNKNOWN;
}
@@ -295,11 +300,24 @@ static inline int ima_read_xattr(struct dentry *dentry,
#ifdef CONFIG_IMA_APPRAISE_MODSIG
bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+ struct modsig **modsig);
+void ima_free_modsig(struct modsig *modsig);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
{
return false;
}
+
+static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
+ loff_t buf_len, struct modsig **modsig)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void ima_free_modsig(struct modsig *modsig)
+{
+}
#endif /* CONFIG_IMA_APPRAISE_MODSIG */
/* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index b3837e26bb27..b8cc5897f16a 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -279,6 +279,33 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
return rc;
}
+/*
+ * modsig_verify - verify modsig signature
+ *
+ * Verify whether the signature matches the file contents.
+ *
+ * Return 0 on success, error code otherwise.
+ */
+static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
+ enum integrity_status *status, const char **cause)
+{
+ int rc;
+
+ rc = integrity_modsig_verify(INTEGRITY_KEYRING_IMA, modsig);
+ if (IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING) && rc &&
+ func == KEXEC_KERNEL_CHECK)
+ rc = integrity_modsig_verify(INTEGRITY_KEYRING_PLATFORM,
+ modsig);
+ if (rc) {
+ *cause = "invalid-signature";
+ *status = INTEGRITY_FAIL;
+ } else {
+ *status = INTEGRITY_PASS;
+ }
+
+ return rc;
+}
+
/*
* ima_appraise_measurement - appraise file measurement
*
@@ -291,7 +318,7 @@ int ima_appraise_measurement(enum ima_hooks func,
struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len)
+ int xattr_len, const struct modsig *modsig)
{
static const char op[] = "appraise_data";
const char *cause = "unknown";
@@ -299,11 +326,14 @@ int ima_appraise_measurement(enum ima_hooks func,
struct inode *inode = d_backing_inode(dentry);
enum integrity_status status = INTEGRITY_UNKNOWN;
int rc = xattr_len;
+ bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig;
- if (!(inode->i_opflags & IOP_XATTR))
+ /* If not appraising a modsig, we need an xattr. */
+ if (!(inode->i_opflags & IOP_XATTR) && !try_modsig)
return INTEGRITY_UNKNOWN;
- if (rc <= 0) {
+ /* If reading the xattr failed and there's no modsig, error out. */
+ if (rc <= 0 && !try_modsig) {
if (rc && rc != -ENODATA)
goto out;
@@ -326,6 +356,10 @@ int ima_appraise_measurement(enum ima_hooks func,
case INTEGRITY_UNKNOWN:
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
+ /* It's fine not to have xattrs when using a modsig. */
+ if (try_modsig)
+ break;
+ /* fall through */
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
cause = "missing-HMAC";
goto out;
@@ -340,6 +374,14 @@ int ima_appraise_measurement(enum ima_hooks func,
rc = xattr_verify(func, iint, xattr_value, xattr_len, &status,
&cause);
+ /*
+ * If we have a modsig and either no imasig or the imasig's key isn't
+ * known, then try verifying the modsig.
+ */
+ if (status != INTEGRITY_PASS && try_modsig &&
+ (!xattr_value || rc == -ENOKEY))
+ rc = modsig_verify(func, modsig, &status, &cause);
+
out:
/*
* File signatures on some filesystems can not be properly verified.
@@ -356,7 +398,7 @@ int ima_appraise_measurement(enum ima_hooks func,
op, cause, rc, 0);
} else if (status != INTEGRITY_PASS) {
/* Fix mode, but don't replace file signatures. */
- if ((ima_appraise & IMA_APPRAISE_FIX) &&
+ if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
(!xattr_value ||
xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
if (!ima_fix_xattr(dentry, iint))
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..722d4e9c72ac 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -181,6 +181,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
int rc = 0, action, must_appraise = 0;
int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
struct evm_ima_xattr_data *xattr_value = NULL;
+ struct modsig *modsig = NULL;
int xattr_len = 0;
bool violation_check;
enum hash_algo hash_algo;
@@ -277,10 +278,15 @@ static int process_measurement(struct file *file, const struct cred *cred,
template_desc = ima_template_desc_current();
if ((action & IMA_APPRAISE_SUBMASK) ||
- strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
+ strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
/* read 'security.ima' */
xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+ /* Read the appended modsig if allowed by the policy. */
+ if (iint->flags & IMA_MODSIG_ALLOWED)
+ ima_read_modsig(func, buf, size, &modsig);
+ }
+
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
@@ -296,7 +302,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
inode_lock(inode);
rc = ima_appraise_measurement(func, iint, file, pathname,
- xattr_value, xattr_len);
+ xattr_value, xattr_len, modsig);
inode_unlock(inode);
}
if (action & IMA_AUDIT)
@@ -310,6 +316,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
rc = -EACCES;
mutex_unlock(&iint->mutex);
kfree(xattr_value);
+ ima_free_modsig(modsig);
out:
if (pathbuf)
__putname(pathbuf);
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index 87503bfe8c8b..ac0f44fb52ce 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -8,8 +8,17 @@
* Thiago Jung Bauermann <bauerman@linux.ibm.com>
*/
+#include <linux/types.h>
+#include <linux/module_signature.h>
+#include <keys/asymmetric-type.h>
+#include <crypto/pkcs7.h>
+
#include "ima.h"
+struct modsig {
+ struct pkcs7_message *pkcs7_msg;
+};
+
/**
* ima_hook_supports_modsig - can the policy allow modsig for this hook?
*
@@ -29,3 +38,65 @@ bool ima_hook_supports_modsig(enum ima_hooks func)
return false;
}
}
+
+/*
+ * ima_read_modsig - Read modsig from buf.
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+ struct modsig **modsig)
+{
+ const size_t marker_len = strlen(MODULE_SIG_STRING);
+ const struct module_signature *sig;
+ struct modsig *hdr;
+ size_t sig_len;
+ const void *p;
+ int rc;
+
+ if (buf_len <= marker_len + sizeof(*sig))
+ return -ENOENT;
+
+ p = buf + buf_len - marker_len;
+ if (memcmp(p, MODULE_SIG_STRING, marker_len))
+ return -ENOENT;
+
+ buf_len -= marker_len;
+ sig = (const struct module_signature *) (p - sizeof(*sig));
+
+ rc = mod_check_sig(sig, buf_len, func_tokens[func]);
+ if (rc)
+ return rc;
+
+ sig_len = be32_to_cpu(sig->sig_len);
+ buf_len -= sig_len + sizeof(*sig);
+
+ hdr = kmalloc(sizeof(*hdr), GFP_KERNEL);
+ if (!hdr)
+ return -ENOMEM;
+
+ hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
+ if (IS_ERR(hdr->pkcs7_msg)) {
+ kfree(hdr);
+ return PTR_ERR(hdr->pkcs7_msg);
+ }
+
+ *modsig = hdr;
+
+ return 0;
+}
+
+int ima_modsig_verify(struct key *keyring, const struct modsig *modsig)
+{
+ return verify_pkcs7_message_sig(NULL, 0, modsig->pkcs7_msg, keyring,
+ VERIFYING_MODULE_SIGNATURE, NULL, NULL);
+}
+
+void ima_free_modsig(struct modsig *modsig)
+{
+ if (!modsig)
+ return;
+
+ pkcs7_free_message(modsig->pkcs7_msg);
+ kfree(modsig);
+}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fca7a3f23321..a7a20a8c15c1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1144,6 +1144,12 @@ void ima_delete_rules(void)
}
}
+#define __ima_hook_stringify(str) (#str),
+
+const char *const func_tokens[] = {
+ __ima_hooks(__ima_hook_stringify)
+};
+
#ifdef CONFIG_IMA_READ_POLICY
enum {
mask_exec = 0, mask_write, mask_read, mask_append
@@ -1156,12 +1162,6 @@ static const char *const mask_tokens[] = {
"MAY_APPEND"
};
-#define __ima_hook_stringify(str) (#str),
-
-static const char *const func_tokens[] = {
- __ima_hooks(__ima_hook_stringify)
-};
-
void *ima_policy_start(struct seq_file *m, loff_t *pos)
{
loff_t l = *pos;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0e7330a36a9d..c6e7f41db470 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -153,10 +153,13 @@ int integrity_kernel_read(struct file *file, loff_t offset,
extern struct dentry *integrity_dir;
+struct modsig;
+
#ifdef CONFIG_INTEGRITY_SIGNATURE
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
const char *digest, int digestlen);
+int integrity_modsig_verify(unsigned int id, const struct modsig *modsig);
int __init integrity_init_keyring(const unsigned int id);
int __init integrity_load_x509(const unsigned int id, const char *path);
@@ -171,6 +174,12 @@ static inline int integrity_digsig_verify(const unsigned int id,
return -EOPNOTSUPP;
}
+static inline int integrity_modsig_verify(unsigned int id,
+ const struct modsig *modsig)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int integrity_init_keyring(const unsigned int id)
{
return 0;
@@ -196,6 +205,16 @@ static inline int asymmetric_verify(struct key *keyring, const char *sig,
}
#endif
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+int ima_modsig_verify(struct key *keyring, const struct modsig *modsig);
+#else
+static inline int ima_modsig_verify(struct key *keyring,
+ const struct modsig *modsig)
+{
+ return -EOPNOTSUPP;
+}
+#endif
+
#ifdef CONFIG_IMA_LOAD_X509
void __init ima_load_x509(void);
#else
^ permalink raw reply related
* [PATCH v10 11/12] ima: Define ima-modsig template
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Define new "d-modsig" template field which holds the digest that is
expected to match the one contained in the modsig, and also new "modsig"
template field which holds the appended file signature.
Add a new "ima-modsig" defined template descriptor with the new fields as
well as the ones from the "ima-sig" descriptor.
Change ima_store_measurement() to accept a struct modsig * argument so that
it can be passed along to the templates via struct ima_event_data.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
Documentation/security/IMA-templates.rst | 7 ++-
security/integrity/ima/ima.h | 21 +++++++-
security/integrity/ima/ima_api.c | 6 ++-
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_modsig.c | 19 +++++++
security/integrity/ima/ima_policy.c | 42 +++++++++++++++-
security/integrity/ima/ima_template.c | 7 ++-
security/integrity/ima/ima_template_lib.c | 60 ++++++++++++++++++++++-
security/integrity/ima/ima_template_lib.h | 4 ++
9 files changed, 158 insertions(+), 10 deletions(-)
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..8da20b444be0 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -68,15 +68,18 @@ descriptors by adding their identifier to the format string
- 'd-ng': the digest of the event, calculated with an arbitrary hash
algorithm (field format: [<hash algo>:]digest, where the digest
prefix is shown only if the hash algorithm is not SHA1 or MD5);
+ - 'd-modsig': the digest of the event without the appended modsig;
- 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature.
+ - 'sig': the file signature;
+ - 'modsig' the appended file signature.
Below, there is the list of defined template descriptors:
- "ima": its format is ``d|n``;
- "ima-ng" (default): its format is ``d-ng|n-ng``;
- - "ima-sig": its format is ``d-ng|n-ng|sig``.
+ - "ima-sig": its format is ``d-ng|n-ng|sig``;
+ - "ima-modsig": its format is ``d-ng|n-ng|sig|d-modsig|modsig``.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e051477badf4..4e51290b149d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -64,6 +64,7 @@ struct ima_event_data {
const unsigned char *filename;
struct evm_ima_xattr_data *xattr_value;
int xattr_len;
+ const struct modsig *modsig;
const char *violation;
};
@@ -205,7 +206,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, int pcr);
+ int xattr_len, const struct modsig *modsig,
+ int pcr);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
@@ -303,6 +305,10 @@ bool ima_hook_supports_modsig(enum ima_hooks func);
int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
struct modsig **modsig);
void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size);
+int ima_get_modsig_digest(const struct modsig *modsig, enum hash_algo *algo,
+ const u8 **digest, u32 *digest_size);
+int ima_modsig_serialize(const struct modsig *modsig, const void **data,
+ u32 *data_len);
void ima_free_modsig(struct modsig *modsig);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
@@ -321,6 +327,19 @@ static inline void ima_collect_modsig(struct modsig *modsig, const void *buf,
{
}
+static inline int ima_get_modsig_digest(const struct modsig *modsig,
+ enum hash_algo *algo,
+ const u8 **digest, u32 *digest_size)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int ima_modsig_serialize(const struct modsig *modsig,
+ const void **data, u32 *data_len)
+{
+ return -EOPNOTSUPP;
+}
+
static inline void ima_free_modsig(struct modsig *modsig)
{
}
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 7c01b0a57a5a..3ef48d516f02 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -281,7 +281,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
void ima_store_measurement(struct integrity_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, int pcr)
+ int xattr_len, const struct modsig *modsig,
+ int pcr)
{
static const char op[] = "add_template_measure";
static const char audit_cause[] = "ENOMEM";
@@ -291,7 +292,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
struct ima_event_data event_data = { .iint = iint, .file = file,
.filename = filename,
.xattr_value = xattr_value,
- .xattr_len = xattr_len };
+ .xattr_len = xattr_len,
+ .modsig = modsig };
int violation = 0;
if (iint->measured_pcrs & (0x1 << pcr))
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b5e44d03a0e3..8e6475854351 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -298,7 +298,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname,
- xattr_value, xattr_len, pcr);
+ xattr_value, xattr_len, modsig, pcr);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
inode_lock(inode);
rc = ima_appraise_measurement(func, iint, file, pathname,
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index 22a49f127437..3956a40ba60c 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -140,6 +140,25 @@ int ima_modsig_verify(struct key *keyring, const struct modsig *modsig)
VERIFYING_MODULE_SIGNATURE, NULL, NULL);
}
+int ima_get_modsig_digest(const struct modsig *modsig, enum hash_algo *algo,
+ const u8 **digest, u32 *digest_size)
+{
+ *algo = modsig->hash_algo;
+ *digest = modsig->digest;
+ *digest_size = modsig->digest_size;
+
+ return 0;
+}
+
+int ima_modsig_serialize(const struct modsig *modsig, const void **data,
+ u32 *data_len)
+{
+ *data = &modsig->raw_pkcs7;
+ *data_len = modsig->raw_pkcs7_len;
+
+ return 0;
+}
+
void ima_free_modsig(struct modsig *modsig)
{
if (!modsig)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a7a20a8c15c1..65989ffc05b1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -10,6 +10,9 @@
* - initialize default measure policy rules
*
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/init.h>
#include <linux/list.h>
#include <linux/fs.h>
@@ -756,6 +759,40 @@ static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
ima_log_string_op(ab, key, value, NULL);
}
+/*
+ * Validating the appended signature included in the measurement list requires
+ * the file hash calculated without the appended signature (i.e., the 'd-modsig'
+ * field). Therefore, notify the user if they have the 'modsig' field but not
+ * the 'd-modsig' field in the template.
+ */
+static void check_current_template_modsig(void)
+{
+#define MSG "template with 'modsig' field also needs 'd-modsig' field\n"
+ struct ima_template_desc *template;
+ bool has_modsig, has_dmodsig;
+ static bool checked;
+ int i;
+
+ /* We only need to notify the user once. */
+ if (checked)
+ return;
+
+ has_modsig = has_dmodsig = false;
+ template = ima_template_desc_current();
+ for (i = 0; i < template->num_fields; i++) {
+ if (!strcmp(template->fields[i]->field_id, "modsig"))
+ has_modsig = true;
+ else if (!strcmp(template->fields[i]->field_id, "d-modsig"))
+ has_dmodsig = true;
+ }
+
+ if (has_modsig && !has_dmodsig)
+ pr_notice(MSG);
+
+ checked = true;
+#undef MSG
+}
+
static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
@@ -1039,10 +1076,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
if ((strcmp(args[0].from, "imasig")) == 0)
entry->flags |= IMA_DIGSIG_REQUIRED;
else if (ima_hook_supports_modsig(entry->func) &&
- strcmp(args[0].from, "imasig|modsig") == 0)
+ strcmp(args[0].from, "imasig|modsig") == 0) {
entry->flags |= IMA_DIGSIG_REQUIRED
| IMA_MODSIG_ALLOWED;
- else
+ check_current_template_modsig();
+ } else
result = -EINVAL;
break;
case Opt_permit_directio:
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b631b8bc7624..b05a14821a08 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
{.name = "ima-ng", .fmt = "d-ng|n-ng"},
{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
+ {.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
{.name = "", .fmt = ""}, /* placeholder for a custom format */
};
@@ -43,8 +44,12 @@ static const struct ima_template_field supported_fields[] = {
.field_show = ima_show_template_string},
{.field_id = "sig", .field_init = ima_eventsig_init,
.field_show = ima_show_template_sig},
+ {.field_id = "d-modsig", .field_init = ima_eventdigest_modsig_init,
+ .field_show = ima_show_template_digest_ng},
+ {.field_id = "modsig", .field_init = ima_eventmodsig_init,
+ .field_show = ima_show_template_sig},
};
-#define MAX_TEMPLATE_NAME_LEN 15
+#define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|d-modisg|modsig")
static struct ima_template_desc *ima_template;
static struct ima_template_desc *lookup_template_desc(const char *name);
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 513b457ae900..f549710f58ce 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -223,7 +223,8 @@ int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
return 0;
}
-static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8 hash_algo,
+static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
+ u8 hash_algo,
struct ima_field_data *field_data)
{
/*
@@ -326,6 +327,41 @@ int ima_eventdigest_ng_init(struct ima_event_data *event_data,
hash_algo, field_data);
}
+/*
+ * This function writes the digest of the file which is expected to match the
+ * digest contained in the file's embedded signature.
+ */
+int ima_eventdigest_modsig_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data)
+{
+ enum hash_algo hash_algo;
+ const u8 *cur_digest;
+ u32 cur_digestsize;
+
+ if (!event_data->modsig)
+ return 0;
+
+ if (event_data->violation) {
+ /* Recording a violation. */
+ hash_algo = HASH_ALGO_SHA1;
+ cur_digest = NULL;
+ cur_digestsize = 0;
+ } else {
+ int rc;
+
+ rc = ima_get_modsig_digest(event_data->modsig, &hash_algo,
+ &cur_digest, &cur_digestsize);
+ if (rc)
+ return rc;
+ else if (hash_algo == HASH_ALGO__LAST || cur_digestsize == 0)
+ /* There was some error collecting the digest. */
+ return -EINVAL;
+ }
+
+ return ima_eventdigest_init_common(cur_digest, cur_digestsize,
+ hash_algo, field_data);
+}
+
static int ima_eventname_init_common(struct ima_event_data *event_data,
struct ima_field_data *field_data,
bool size_limit)
@@ -389,3 +425,25 @@ int ima_eventsig_init(struct ima_event_data *event_data,
return ima_write_template_field_data(xattr_value, event_data->xattr_len,
DATA_FMT_HEX, field_data);
}
+
+int ima_eventmodsig_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data)
+{
+ const void *data;
+ u32 data_len;
+ int rc;
+
+ if (!event_data->modsig)
+ return 0;
+
+ /*
+ * The xattr_value for IMA_MODSIG is a runtime structure containing
+ * pointers. Get its raw data instead.
+ */
+ rc = ima_modsig_serialize(event_data->modsig, &data, &data_len);
+ if (rc)
+ return rc;
+
+ return ima_write_template_field_data(data, data_len,
+ DATA_FMT_HEX, field_data);
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6a3d8b831deb..1d7c690ebae5 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -38,8 +38,12 @@ int ima_eventname_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
int ima_eventdigest_ng_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
+int ima_eventdigest_modsig_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data);
int ima_eventname_ng_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
int ima_eventsig_init(struct ima_event_data *event_data,
struct ima_field_data *field_data);
+int ima_eventmodsig_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data);
#endif /* __LINUX_IMA_TEMPLATE_LIB_H */
^ permalink raw reply related
* [PATCH v10 12/12] ima: Store the measurement again when appraising a modsig
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
If the IMA template contains the "modsig" or "d-modsig" field, then the
modsig should be added to the measurement list when the file is appraised.
And that is what normally happens, but if a measurement rule caused a file
containing a modsig to be measured before a different rule causes it to be
appraised, the resulting measurement entry will not contain the modsig
because it is only fetched during appraisal. When the appraisal rule
triggers, it won't store a new measurement containing the modsig because
the file was already measured.
We need to detect that situation and store an additional measurement with
the modsig. This is done by adding an IMA_MEASURE action flag if we read a
modsig and the IMA template contains a modsig field.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 19 +++++++++++++++----
security/integrity/ima/ima_main.c | 14 +++++++++++---
security/integrity/ima/ima_template.c | 24 ++++++++++++++++++++++++
4 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4e51290b149d..4e504c011b69 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -148,6 +148,7 @@ int ima_init_crypto(void);
void ima_putc(struct seq_file *m, void *data, int datalen);
void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
struct ima_template_desc *ima_template_desc_current(void);
+bool ima_template_has_modsig(void);
int ima_restore_measurement_entry(struct ima_template_entry *entry);
int ima_restore_measurement_list(loff_t bufsize, void *buf);
int ima_measurements_show(struct seq_file *m, void *v);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 3ef48d516f02..663805887fac 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -212,6 +212,14 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
char digest[IMA_MAX_DIGEST_SIZE];
} hash;
+ /*
+ * Always collect the modsig, because IMA might have already collected
+ * the file digest without collecting the modsig in a previous
+ * measurement rule.
+ */
+ if (modsig)
+ ima_collect_modsig(modsig, buf, size);
+
if (iint->flags & IMA_COLLECTED)
goto out;
@@ -245,9 +253,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
memcpy(iint->ima_hash, &hash, length);
iint->version = i_version;
- if (modsig)
- ima_collect_modsig(modsig, buf, size);
-
/* Possibly temporary failure due to type of read (eg. O_DIRECT) */
if (!result)
iint->flags |= IMA_COLLECTED;
@@ -296,7 +301,13 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
.modsig = modsig };
int violation = 0;
- if (iint->measured_pcrs & (0x1 << pcr))
+ /*
+ * We still need to store the measurement in the case of MODSIG because
+ * we only have its contents to put in the list at the time of
+ * appraisal, but a file measurement from earlier might already exist in
+ * the measurement list.
+ */
+ if (iint->measured_pcrs & (0x1 << pcr) && !modsig)
return;
result = ima_alloc_init_template(&event_data, &entry);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8e6475854351..f91ed4189f98 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -282,9 +282,17 @@ static int process_measurement(struct file *file, const struct cred *cred,
/* read 'security.ima' */
xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
- /* Read the appended modsig if allowed by the policy. */
- if (iint->flags & IMA_MODSIG_ALLOWED)
- ima_read_modsig(func, buf, size, &modsig);
+ /*
+ * Read the appended modsig, if allowed by the policy, and allow
+ * an additional measurement list entry, if needed, based on the
+ * template format.
+ */
+ if (iint->flags & IMA_MODSIG_ALLOWED) {
+ rc = ima_read_modsig(func, buf, size, &modsig);
+
+ if (!rc && ima_template_has_modsig())
+ action |= IMA_MEASURE;
+ }
}
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b05a14821a08..db3b4257e58b 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -57,6 +57,26 @@ static int template_desc_init_fields(const char *template_fmt,
const struct ima_template_field ***fields,
int *num_fields);
+/* Whether the current template has fields referencing a file's signature. */
+static bool template_has_modsig;
+
+static bool find_modsig_in_template(void)
+{
+ int i;
+
+ for (i = 0; i < ima_template->num_fields; i++)
+ if (!strcmp(ima_template->fields[i]->field_id, "modsig") ||
+ !strcmp(ima_template->fields[i]->field_id, "d-modsig"))
+ return true;
+
+ return false;
+}
+
+bool ima_template_has_modsig(void)
+{
+ return template_has_modsig;
+}
+
static int __init ima_template_setup(char *str)
{
struct ima_template_desc *template_desc;
@@ -89,6 +109,8 @@ static int __init ima_template_setup(char *str)
}
ima_template = template_desc;
+ template_has_modsig = find_modsig_in_template();
+
return 1;
}
__setup("ima_template=", ima_template_setup);
@@ -108,6 +130,7 @@ static int __init ima_template_fmt_setup(char *str)
builtin_templates[num_templates - 1].fmt = str;
ima_template = builtin_templates + num_templates - 1;
+ template_has_modsig = find_modsig_in_template();
return 1;
}
@@ -230,6 +253,7 @@ struct ima_template_desc *ima_template_desc_current(void)
ima_init_template_list();
ima_template =
lookup_template_desc(CONFIG_IMA_DEFAULT_TEMPLATE);
+ template_has_modsig = find_modsig_in_template();
}
return ima_template;
}
^ permalink raw reply related
* [PATCH v10 10/12] ima: Collect modsig
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Obtain the modsig and calculate its corresponding hash in
ima_collect_measurement().
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
security/integrity/ima/ima.h | 8 ++++-
security/integrity/ima/ima_api.c | 5 ++-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_modsig.c | 50 ++++++++++++++++++++++++++-
5 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9f69befd8674..e051477badf4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -201,7 +201,7 @@ int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
- enum hash_algo algo);
+ enum hash_algo algo, struct modsig *modsig);
void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
@@ -302,6 +302,7 @@ static inline int ima_read_xattr(struct dentry *dentry,
bool ima_hook_supports_modsig(enum ima_hooks func);
int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
struct modsig **modsig);
+void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size);
void ima_free_modsig(struct modsig *modsig);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
@@ -315,6 +316,11 @@ static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
return -EOPNOTSUPP;
}
+static inline void ima_collect_modsig(struct modsig *modsig, const void *buf,
+ loff_t size)
+{
+}
+
static inline void ima_free_modsig(struct modsig *modsig)
{
}
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 0639d0631f2c..7c01b0a57a5a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -198,7 +198,7 @@ int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
*/
int ima_collect_measurement(struct integrity_iint_cache *iint,
struct file *file, void *buf, loff_t size,
- enum hash_algo algo)
+ enum hash_algo algo, struct modsig *modsig)
{
const char *audit_cause = "failed";
struct inode *inode = file_inode(file);
@@ -245,6 +245,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
memcpy(iint->ima_hash, &hash, length);
iint->version = i_version;
+ if (modsig)
+ ima_collect_modsig(modsig, buf, size);
+
/* Possibly temporary failure due to type of read (eg. O_DIRECT) */
if (!result)
iint->flags |= IMA_COLLECTED;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index b8cc5897f16a..119034a59627 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -437,7 +437,7 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
!(iint->flags & IMA_HASH))
return;
- rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo);
+ rc = ima_collect_measurement(iint, file, NULL, 0, ima_hash_algo, NULL);
if (rc < 0)
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 722d4e9c72ac..b5e44d03a0e3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -289,7 +289,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
- rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
+ rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
if (rc != 0 && rc != -EBADF && rc != -EINVAL)
goto out_locked;
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index ac0f44fb52ce..22a49f127437 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -17,6 +17,19 @@
struct modsig {
struct pkcs7_message *pkcs7_msg;
+
+ enum hash_algo hash_algo;
+
+ /* This digest will go in the 'd-modsig' field of the IMA template. */
+ const u8 *digest;
+ u32 digest_size;
+
+ /*
+ * This is what will go to the measurement list if the template requires
+ * storing the signature.
+ */
+ int raw_pkcs7_len;
+ u8 raw_pkcs7[];
};
/**
@@ -71,7 +84,8 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
sig_len = be32_to_cpu(sig->sig_len);
buf_len -= sig_len + sizeof(*sig);
- hdr = kmalloc(sizeof(*hdr), GFP_KERNEL);
+ /* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
+ hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
if (!hdr)
return -ENOMEM;
@@ -81,11 +95,45 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
return PTR_ERR(hdr->pkcs7_msg);
}
+ memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
+ hdr->raw_pkcs7_len = sig_len;
+
+ /* We don't know the hash algorithm yet. */
+ hdr->hash_algo = HASH_ALGO__LAST;
+
*modsig = hdr;
return 0;
}
+/**
+ * ima_collect_modsig - Calculate the file hash without the appended signature.
+ *
+ * Since the modsig is part of the file contents, the hash used in its signature
+ * isn't the same one ordinarily calculated by IMA. Therefore PKCS7 code
+ * calculates a separate one for signature verification.
+ */
+void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size)
+{
+ int rc;
+
+ /*
+ * Provide the file contents (minus the appended sig) so that the PKCS7
+ * code can calculate the file hash.
+ */
+ size -= modsig->raw_pkcs7_len + strlen(MODULE_SIG_STRING) +
+ sizeof(struct module_signature);
+ rc = pkcs7_supply_detached_data(modsig->pkcs7_msg, buf, size);
+ if (rc)
+ return;
+
+ /* Ask the PKCS7 code to calculate the file hash. */
+ rc = pkcs7_get_digest(modsig->pkcs7_msg, &modsig->digest,
+ &modsig->digest_size, &modsig->hash_algo);
+ if (rc)
+ return;
+}
+
int ima_modsig_verify(struct key *keyring, const struct modsig *modsig)
{
return verify_pkcs7_message_sig(NULL, 0, modsig->pkcs7_msg, keyring,
^ permalink raw reply related
* [PATCH v10 08/12] ima: Factor xattr_verify() out of ima_appraise_measurement()
From: Thiago Jung Bauermann @ 2019-04-18 3:51 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190418035120.2354-1-bauerman@linux.ibm.com>
Verify xattr signature in a separate function so that the logic in
ima_appraise_measurement() remains clear when it gains the ability to also
verify an appended module signature.
The code in the switch statement is unchanged except for having to
dereference the status and cause variables (since they're now pointers),
and fixing the style of a block comment to appease checkpatch.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
security/integrity/ima/ima_appraise.c | 141 +++++++++++++++-----------
1 file changed, 81 insertions(+), 60 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ea8fa29f07d3..b3837e26bb27 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -202,6 +202,83 @@ int ima_read_xattr(struct dentry *dentry,
return ret;
}
+/*
+ * xattr_verify - verify xattr digest or signature
+ *
+ * Verify whether the hash or signature matches the file contents.
+ *
+ * Return 0 on success, error code otherwise.
+ */
+static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
+ struct evm_ima_xattr_data *xattr_value, int xattr_len,
+ enum integrity_status *status, const char **cause)
+{
+ int rc = -EINVAL, hash_start = 0;
+
+ switch (xattr_value->type) {
+ case IMA_XATTR_DIGEST_NG:
+ /* first byte contains algorithm id */
+ hash_start = 1;
+ /* fall through */
+ case IMA_XATTR_DIGEST:
+ if (iint->flags & IMA_DIGSIG_REQUIRED) {
+ *cause = "IMA-signature-required";
+ *status = INTEGRITY_FAIL;
+ break;
+ }
+ clear_bit(IMA_DIGSIG, &iint->atomic_flags);
+ if (xattr_len - sizeof(xattr_value->type) - hash_start >=
+ iint->ima_hash->length)
+ /*
+ * xattr length may be longer. md5 hash in previous
+ * version occupied 20 bytes in xattr, instead of 16
+ */
+ rc = memcmp(&xattr_value->data[hash_start],
+ iint->ima_hash->digest,
+ iint->ima_hash->length);
+ else
+ rc = -EINVAL;
+ if (rc) {
+ *cause = "invalid-hash";
+ *status = INTEGRITY_FAIL;
+ break;
+ }
+ *status = INTEGRITY_PASS;
+ break;
+ case EVM_IMA_XATTR_DIGSIG:
+ set_bit(IMA_DIGSIG, &iint->atomic_flags);
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+ (const char *)xattr_value,
+ xattr_len,
+ iint->ima_hash->digest,
+ iint->ima_hash->length);
+ if (rc == -EOPNOTSUPP) {
+ *status = INTEGRITY_UNKNOWN;
+ break;
+ }
+ if (IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING) && rc &&
+ func == KEXEC_KERNEL_CHECK)
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_PLATFORM,
+ (const char *)xattr_value,
+ xattr_len,
+ iint->ima_hash->digest,
+ iint->ima_hash->length);
+ if (rc) {
+ *cause = "invalid-signature";
+ *status = INTEGRITY_FAIL;
+ } else {
+ *status = INTEGRITY_PASS;
+ }
+ break;
+ default:
+ *status = INTEGRITY_UNKNOWN;
+ *cause = "unknown-ima-data";
+ break;
+ }
+
+ return rc;
+}
+
/*
* ima_appraise_measurement - appraise file measurement
*
@@ -221,7 +298,7 @@ int ima_appraise_measurement(enum ima_hooks func,
struct dentry *dentry = file_dentry(file);
struct inode *inode = d_backing_inode(dentry);
enum integrity_status status = INTEGRITY_UNKNOWN;
- int rc = xattr_len, hash_start = 0;
+ int rc = xattr_len;
if (!(inode->i_opflags & IOP_XATTR))
return INTEGRITY_UNKNOWN;
@@ -259,65 +336,9 @@ int ima_appraise_measurement(enum ima_hooks func,
WARN_ONCE(true, "Unexpected integrity status %d\n", status);
}
- switch (xattr_value->type) {
- case IMA_XATTR_DIGEST_NG:
- /* first byte contains algorithm id */
- hash_start = 1;
- /* fall through */
- case IMA_XATTR_DIGEST:
- if (iint->flags & IMA_DIGSIG_REQUIRED) {
- cause = "IMA-signature-required";
- status = INTEGRITY_FAIL;
- break;
- }
- clear_bit(IMA_DIGSIG, &iint->atomic_flags);
- if (xattr_len - sizeof(xattr_value->type) - hash_start >=
- iint->ima_hash->length)
- /* xattr length may be longer. md5 hash in previous
- version occupied 20 bytes in xattr, instead of 16
- */
- rc = memcmp(&xattr_value->data[hash_start],
- iint->ima_hash->digest,
- iint->ima_hash->length);
- else
- rc = -EINVAL;
- if (rc) {
- cause = "invalid-hash";
- status = INTEGRITY_FAIL;
- break;
- }
- status = INTEGRITY_PASS;
- break;
- case EVM_IMA_XATTR_DIGSIG:
- set_bit(IMA_DIGSIG, &iint->atomic_flags);
- rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
- (const char *)xattr_value,
- xattr_len,
- iint->ima_hash->digest,
- iint->ima_hash->length);
- if (rc == -EOPNOTSUPP) {
- status = INTEGRITY_UNKNOWN;
- break;
- }
- if (IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING) && rc &&
- func == KEXEC_KERNEL_CHECK)
- rc = integrity_digsig_verify(INTEGRITY_KEYRING_PLATFORM,
- (const char *)xattr_value,
- xattr_len,
- iint->ima_hash->digest,
- iint->ima_hash->length);
- if (rc) {
- cause = "invalid-signature";
- status = INTEGRITY_FAIL;
- } else {
- status = INTEGRITY_PASS;
- }
- break;
- default:
- status = INTEGRITY_UNKNOWN;
- cause = "unknown-ima-data";
- break;
- }
+ if (xattr_value)
+ rc = xattr_verify(func, iint, xattr_value, xattr_len, &status,
+ &cause);
out:
/*
^ permalink raw reply related
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Andy Lutomirski @ 2019-04-18 4:41 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, Nadav Amit, Ingo Molnar, Khalid Aziz,
Juerg Haefliger, Tycho Andersen, jsteckli, Kees Cook,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris hyser, Tyler Hicks, David Woodhouse, Andrew Cooper,
Jon Masters, Boris Ostrovsky, iommu, X86 ML,
linux-alpha@vger.kernel.org, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <CAHk-=whUwOjFW6RjHVM8kNOv1QVLJuHj2Dda0=mpLPdJ1UyatQ@mail.gmail.com>
On Wed, Apr 17, 2019 at 5:00 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Apr 17, 2019 at 4:42 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Wed, 17 Apr 2019, Linus Torvalds wrote:
> >
> > > With SMEP, user space pages are always NX.
> >
> > We talk past each other. The user space page in the ring3 valid virtual
> > address space (non negative) is of course protected by SMEP.
> >
> > The attack utilizes the kernel linear mapping of the physical
> > memory. I.e. user space address 0x43210 has a kernel equivalent at
> > 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid
> > kernel address and that is mapped X --> game over. SMEP does not help
> > there.
>
> Oh, agreed.
>
> But that would simply be a kernel bug. We should only map kernel pages
> executable when we have kernel code in them, and we should certainly
> not allow those pages to be mapped writably in user space.
>
> That kind of "executable in kernel, writable in user" would be a
> horrendous and major bug.
>
> So i think it's a non-issue.
>
> > From the top of my head I'd say this is a non issue as those kernel address
> > space mappings _should_ be NX, but we got bitten by _should_ in the past:)
>
> I do agree that bugs can happen, obviously, and we might have missed something.
>
> But in the context of XPFO, I would argue (*very* strongly) that the
> likelihood of the above kind of bug is absolutely *miniscule* compared
> to the likelihood that we'd have something wrong in the software
> implementation of XPFO.
>
> So if the argument is "we might have bugs in software", then I think
> that's an argument _against_ XPFO rather than for it.
>
I don't think this type of NX goof was ever the argument for XPFO.
The main argument I've heard is that a malicious user program writes a
ROP payload into user memory (regular anonymous user memory) and then
gets the kernel to erroneously set RSP (*not* RIP) to point there.
I find this argument fairly weak for a couple reasons. First, if
we're worried about this, let's do in-kernel CFI, not XPFO, to
mitigate it. Second, I don't see why the exact same attack can't be
done using, say, page cache, and unless I'm missing something, XPFO
doesn't protect page cache. Or network buffers, or pipe buffers, etc.
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Kees Cook @ 2019-04-18 5:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Thomas Gleixner, Nadav Amit, Ingo Molnar,
Khalid Aziz, Juerg Haefliger, Tycho Andersen, Julian Stecklina,
Kees Cook, Konrad Rzeszutek Wilk, Juerg Haefliger,
deepa.srinivasan, chris hyser, Tyler Hicks, David Woodhouse,
Andrew Cooper, Jon Masters, Boris Ostrovsky, iommu, X86 ML,
linux-alpha@vger.kernel.org, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Peter Zijlstra, Dave Hansen, Borislav Petkov,
H. Peter Anvin, Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <CALCETrXm9PuUTEEmzA8bQJmg=PHC_2XSywECittVhXbMJS1rSA@mail.gmail.com>
On Wed, Apr 17, 2019 at 11:41 PM Andy Lutomirski <luto@kernel.org> wrote:
> I don't think this type of NX goof was ever the argument for XPFO.
> The main argument I've heard is that a malicious user program writes a
> ROP payload into user memory (regular anonymous user memory) and then
> gets the kernel to erroneously set RSP (*not* RIP) to point there.
Well, more than just ROP. Any of the various attack primitives. The NX
stuff is about moving RIP: SMEP-bypassing. But there is still basic
SMAP-bypassing for putting a malicious structure in userspace and
having the kernel access it via the linear mapping, etc.
> I find this argument fairly weak for a couple reasons. First, if
> we're worried about this, let's do in-kernel CFI, not XPFO, to
CFI is getting much closer. Getting the kernel happy under Clang, LTO,
and CFI is under active development. (It's functional for arm64
already, and pieces have been getting upstreamed.)
> mitigate it. Second, I don't see why the exact same attack can't be
> done using, say, page cache, and unless I'm missing something, XPFO
> doesn't protect page cache. Or network buffers, or pipe buffers, etc.
My understanding is that it's much easier to feel out the linear
mapping address than for the others. But yes, all of those same attack
primitives are possible in other memory areas (though most are NX),
and plenty of exploits have done such things.
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Thomas Gleixner @ 2019-04-18 6:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Nadav Amit, Ingo Molnar, Khalid Aziz, juergh, Tycho Andersen,
jsteckli, Kees Cook, Konrad Rzeszutek Wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, Tyler Hicks, David Woodhouse,
Andrew Cooper, Jon Masters, Boris Ostrovsky, iommu, X86 ML,
linux-alpha@vger.kernel.org, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Andy Lutomirski, Peter Zijlstra, Dave Hansen,
Borislav Petkov, H. Peter Anvin, Arjan van de Ven,
Greg Kroah-Hartman
In-Reply-To: <CAHk-=whUwOjFW6RjHVM8kNOv1QVLJuHj2Dda0=mpLPdJ1UyatQ@mail.gmail.com>
On Wed, 17 Apr 2019, Linus Torvalds wrote:
> On Wed, Apr 17, 2019 at 4:42 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 17 Apr 2019, Linus Torvalds wrote:
> > > With SMEP, user space pages are always NX.
> >
> > We talk past each other. The user space page in the ring3 valid virtual
> > address space (non negative) is of course protected by SMEP.
> >
> > The attack utilizes the kernel linear mapping of the physical
> > memory. I.e. user space address 0x43210 has a kernel equivalent at
> > 0xfxxxxxxxxxx. So if the attack manages to trick the kernel to that valid
> > kernel address and that is mapped X --> game over. SMEP does not help
> > there.
>
> Oh, agreed.
>
> But that would simply be a kernel bug. We should only map kernel pages
> executable when we have kernel code in them, and we should certainly
> not allow those pages to be mapped writably in user space.
>
> That kind of "executable in kernel, writable in user" would be a
> horrendous and major bug.
>
> So i think it's a non-issue.
Pretty much.
> > From the top of my head I'd say this is a non issue as those kernel address
> > space mappings _should_ be NX, but we got bitten by _should_ in the past:)
>
> I do agree that bugs can happen, obviously, and we might have missed something.
>
> But in the context of XPFO, I would argue (*very* strongly) that the
> likelihood of the above kind of bug is absolutely *miniscule* compared
> to the likelihood that we'd have something wrong in the software
> implementation of XPFO.
>
> So if the argument is "we might have bugs in software", then I think
> that's an argument _against_ XPFO rather than for it.
No argument from my side. We better spend time to make sure that a bogus
kernel side X mapping is caught, like we catch other things.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: Daniel Axtens @ 2019-04-18 6:38 UTC (permalink / raw)
To: Andrew Donnellan, Matthew Garrett, jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
linuxppc-dev, Michael Ellerman, cmr
In-Reply-To: <059c523e-926c-24ee-0935-198031712145@au1.ibm.com>
Hi Andrew,
>> + If CONFIG_LOCK_DOWN_KERNEL is enabled, the kernel can be
>> + moved to a more locked down state at runtime by writing to
>> + this attribute. Valid values are:
>> +
>> + integrity:
>> + The kernel will disable functionality that allows
>> + userland to modify the running kernel image, other
>> + than through the loading or execution of appropriately
>> + signed objects.
>> +
>> + confidentiality:
>> + The kernel will disable all functionality disabled by
>> + the integrity mode, but additionally will disable
>> + features that potentially permit userland to obtain
>> + confidential information stored within the kernel.
>
> [+ linuxppc, mpe, dja, cmr]
>
> I'm thinking about whether we should lock down the powerpc xmon debug
> monitor - intuitively, I think the answer is yes if for no other reason
> than Least Astonishment, when lockdown is enabled you probably don't
> expect xmon to keep letting you access kernel memory.
>
> Semantically though, xmon is not a userspace process - it's in kernel
> and reads debug commands/outputs debug data directly from/to the
> console. Is that a threat vector that this series cares about?
I guess there are 2 ways you could think about lockdown:
- It adds a security boundary between the kernel and UID 0, so that
userland cannot compromise the integrity/confidentiality of the
locked down kernel.
- It is a bundle of related security boundaries so that the
integrity/confidentiality of a running, locked down kernel cannot be
compromised, even by a privileged, physically present user.
You're right that techincally xmon is in the kernel and on the console
rather than in userland, so it doesn't fall within the first concept of
lockdown. But I think usecases for lockdown tend to expect something
more like the second concept.
IOW, lockdown is a trapdoor - once you've locked down a kernel, you
can't get out of lockdown (except by rebooting). xmon would allow you to
get out of the trapdoor, so I think it should be restricted by lockdown.
Regards,
Daniel
>
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Jan Kara @ 2019-04-18 10:53 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Jan Kara, Amir Goldstein, linux-fsdevel, Al Viro,
Matthew Bobrowski, LSM List, overlayfs
In-Reply-To: <CAJfpegvYnnjv7uoQjL1avx5tO-0H0ZecmVK9Sgk-F1NoW0HAuw@mail.gmail.com>
On Wed 17-04-19 16:14:32, Miklos Szeredi wrote:
> On Wed, Apr 17, 2019 at 4:06 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 17-04-19 14:14:58, Miklos Szeredi wrote:
> > > On Wed, Apr 17, 2019 at 1:30 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > > > > > I'm not so sure about directory pre-modification hooks. Given the amount of
> > > > > > problems we face with applications using fanotify permission events and
> > > > > > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > > > > > you want to use such hooks for recording (and persisting) that some change
> > > > > > is going to happen and provide crash-consistency guarantees for such
> > > > > > journal?
> > > > > >
> > > > >
> > > > > That's the general idea.
> > > > > I have two use cases for pre-modification hooks:
> > > > > 1. VFS level snapshots
> > > > > 2. persistent change tracking
> > > > >
> > > > > TBH, I did not consider implementing any of the above in userspace,
> > > > > so I do not have a specific interest in extending the fanotify API.
> > > > > I am actually interested in pre-modify fsnotify hooks (not fanotify),
> > > > > that a snapshot or change tracking subsystem can register with.
> > > > > An in-kernel fsnotify event handler can set a flag in current task
> > > > > struct to circumvent system deadlocks on nested filesystem access.
> > > >
> > > > OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
> > > > handlers stay within the kernel, I'm fine with that. After all this is what
> > > > LSMs are already doing. Just exposing this to userspace for arbitration is
> > > > what I have a problem with.
> > >
> > > There's one more usecase that I'd like to explore: providing coherent
> > > view of host filesystem in virtualized environments. This requires
> > > that guest is synchronously notified when the host filesystem changes.
> > > I do agree, however, that adding sync hooks to userspace is
> > > problematic.
> > >
> > > One idea would be to use shared memory instead of a procedural
> > > notification. I.e. application (hypervisor) registers a pointer to a
> > > version number that the kernel associates with the given inode. When
> > > the inode is changed, then the version number is incremented. The
> > > guest kernel can then look at the version number when verifying cache
> > > validity. That way perfect coherency is guaranteed between host and
> > > guest filesystems without allowing a broken guest or even a broken
> > > hypervisor to DoS the host.
> >
> > Well, statx() and looking at i_version can do this for you. So I guess
> > that's too slow for your purposes?
>
> Okay, missing piece of information: we want to make use of the dcache
> and icache in the guest kernel, otherwise lookup/stat will be
> painfully slow. That would preclude doing statx() or anything else
> that requires a synchronous round trip to the host for the likely case
> of a valid cache.
Ok, understood.
> > Also how many inodes do you want to
> > monitor like this?
>
> Everything that's in the guest caches. Which means: a lot.
Yeah, but that would mean also non-trivial amount of memory pinned for this
communication channel... And AFAIU the cost of invalidation going to the
guest isn't so critical as that isn't expected to be that frequent. It is
only the cost of 'is_valid' check that needs to be low to make the caching
in the guest useful. So won't it be better if we just had some kind of ring
buffer for invalidation events going from host to guest, host could just
queue event there (i.e., event processing from the host would have well
bounded time like processing normal fsnotify events has), guest would be
consuming events and updating its validity information. If the buffer
overflows, it means "invalidate all" which is expensive for the guest but
if buffers are reasonably sized, it should not happen frequently...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Avoiding merge conflicts while adding new docs - Was: Re: [PATCH 00/57] Convert files to ReST
From: Mauro Carvalho Chehab @ 2019-04-18 12:42 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
linux-riscv, linux-fbdev, linux-s390, Greg Kroah-Hartman,
linux-watchdog, xdp-newbies, linux-samsung-soc, linux-acpi,
linux-stm32, bpf, linux-ide, linux-pm, dri-devel, linux-scsi,
linux-gpio, linuxppc-dev, x86, dm-devel, target-devel, netdev,
kexec, linux-fpga, linux-rdma, linux-kbuild, Thomas Gleixner,
linux-security-module, linux-usb, linux-arm-kernel
In-Reply-To: <cover.1555382110.git.mchehab+samsung@kernel.org>
Jon,
Em Mon, 15 Apr 2019 23:55:25 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:
> I have a separate patch series with do the actual rename and
> adjustment of references. I opted to submit this first, as it
> sounds easier to merge this way, as each subsystem maintainer
> can apply the conversion directly on their trees (or at docs
> tree), avoiding merge conflects.
Based on the number of feedbacks we have about this, I'm
considering to submit a second version of my conversion patch
series that would be doing the rename together with each patch,
adding the rst files to an index file.
However, doing that would produce lots of warnings. We have
already lots of those at linux-next:
checking consistency...
docs/Documentation/accelerators/ocxl.rst: WARNING: document isn't included in any toctree
docs/Documentation/admin-guide/mm/numaperf.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/allocators.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/attributes.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/bigalloc.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/bitmaps.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/blockgroup.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/blocks.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/checksums.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/directory.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/eainode.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/group_descr.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/ifork.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/inlinedata.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/inodes.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/journal.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/mmp.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/special_inodes.rst: WARNING: document isn't included in any toctree
docs/Documentation/filesystems/ext4/super.rst: WARNING: document isn't included in any toctree
docs/Documentation/fmc/index.rst: WARNING: document isn't included in any toctree
docs/Documentation/gpu/msm-crash-dump.rst: WARNING: document isn't included in any toctree
docs/Documentation/interconnect/interconnect.rst: WARNING: document isn't included in any toctree
docs/Documentation/laptops/lg-laptop.rst: WARNING: document isn't included in any toctree
docs/Documentation/virtual/kvm/amd-memory-encryption.rst: WARNING: document isn't included in any toctree
docs/Documentation/virtual/kvm/vcpu-requests.rst: WARNING: document isn't included in any toctree
After thinking a little bit and doing some tests, I think a good solution
would be to add ":orphan:" markup to the new .rst files that were not
added yet into the main body (e. g. something like the enclosed example).
That will make Sphinx suppress the:
"WARNING: document isn't included in any toctree"
warning for the new files, while they're not included at the main indexes.
This way, maintainers can do all the hard work of doing/applying the ReST
file conversion/addition patch series on their own trees, and, once
everything gets merged, submit a patch against the latest docs-next
tree, removing the :orphan: tag and add them to the common index.rst
files.
That should largely avoid merging conflicts.
For example, assuming that someone converts the stuff at
Documentation/accounting and rename the text files there to
RST (I'm actually doing that), he could add the enclosed change on
the top of its index file:
diff --git a/Documentation/accounting/index.rst b/Documentation/accounting/index.rst
index e7dda5afa73f..e1f6284b5ff3 100644
--- a/Documentation/accounting/index.rst
+++ b/Documentation/accounting/index.rst
@@ -1,3 +1,5 @@
+:orphan:
+
==========
Accounting
==========
With would make Sphinx to ignore the subdir index file while
reporting errors. It will still build the documentation, allowing
the developer to test the changes.
One of the advantages is that checking the orphaned docs is as
easy as running:
$ git grep -l ":orphan:" Documentation/
...
Documentation/accounting/index.rst
...
Making easy for people to check the orphaned files and send a fixup
patch if something got orphaned after the merge window and send a
fixes patches to be applied upstream.
What do you think?
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH v2 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-04-18 13:02 UTC (permalink / raw)
To: Laura Abbott
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, Linux Kbuild mailing list,
Nick Desaulniers, Kostya Serebryany, Dmitriy Vyukov, Kees Cook,
Sandeep Patil, Kernel Hardening
In-Reply-To: <497b1201-b2ae-5e0c-d191-ff1830d92fc1@redhat.com>
On Mon, Apr 8, 2019 at 6:43 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 3/8/19 5:27 AM, Alexander Potapenko wrote:
> > This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> > without the need to pass any boot parameters.
> >
> > No performance optimizations are done at the moment to reduce double
> > initialization of memory regions.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Kostya Serebryany <kcc@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Sandeep Patil <sspatil@android.com>
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kbuild@vger.kernel.org
> > Cc: kernel-hardening@lists.openwall.com
> > ---
> > mm/page_poison.c | 5 +++++
> > mm/slub.c | 2 ++
> > security/Kconfig.initmem | 11 +++++++++++
> > 3 files changed, 18 insertions(+)
> >
> > diff --git a/mm/page_poison.c b/mm/page_poison.c
> > index 21d4f97cb49b..a1985f33f635 100644
> > --- a/mm/page_poison.c
> > +++ b/mm/page_poison.c
> > @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
> >
> > static int __init early_page_poison_param(char *buf)
> > {
> > +#ifdef CONFIG_INIT_ALL_HEAP
> > + want_page_poisoning = true;
> > + return 0;
> > +#else
> > if (!buf)
> > return -EINVAL;
> > return strtobool(buf, &want_page_poisoning);
> > +#endif
> > }
> > early_param("page_poison", early_page_poison_param);
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 1b08fbcb7e61..00e0197d3f35 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
> > if (*str == ',')
> > slub_debug_slabs = str + 1;
> > out:
> > + if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
> > + slub_debug |= SLAB_POISON;
> > return 1;
> > }
> >
>
> I've looked at doing something similar in the past (failing to find
> the thread this morning...) and while this will work, it has pretty
> serious performance issues. It's not actually the poisoning which
> is expensive but that turning on debugging removes the cpu slab
> which has significant performance penalties.
>
> I'd rather go back to the proposal of just poisoning the slab
> at alloc/free without using SLAB_POISON.
Hi Laura,
May I wonder what were the performance numbers you were seeing?
I've found this patch:
https://www.openwall.com/lists/kernel-hardening/2016/01/26/1, but
that's around 100% slowdown.
> Thanks,
> Laura
>
>
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > index 27aec394365e..5ce49663777a 100644
> > --- a/security/Kconfig.initmem
> > +++ b/security/Kconfig.initmem
> > @@ -13,6 +13,17 @@ config INIT_ALL_MEMORY
> >
> > if INIT_ALL_MEMORY
> >
> > +config INIT_ALL_HEAP
> > + bool "Initialize all heap"
> > + depends on INIT_ALL_MEMORY
> > + select CONFIG_PAGE_POISONING
> > + select CONFIG_PAGE_POISONING_NO_SANITY
> > + select CONFIG_PAGE_POISONING_ZERO
> > + select CONFIG_SLUB_DEBUG
> > + default y
> > + help
> > + Enable page poisoning and slub poisoning by default.
> > +
> > config INIT_ALL_STACK
> > bool "Initialize all stack"
> > depends on INIT_ALL_MEMORY
> >
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Stephen Smalley @ 2019-04-18 13:39 UTC (permalink / raw)
To: Casey Schaufler, Oleg Nesterov, Paul Moore
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <939a45a6-fc1b-0ac3-759b-0e7c3ce3d670@schaufler-ca.com>
On 4/17/19 12:42 PM, Casey Schaufler wrote:
> On 4/17/2019 9:27 AM, Oleg Nesterov wrote:
>> On 04/17, Paul Moore wrote:
>>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 04/17, Paul Moore wrote:
>>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>>> the task's credentials are not the same as its real_cred;
>>>> What about other modules? I have no idea what smack_setprocattr() is,
>>>> but it too does prepare_creds/commit creds.
>>>>
>>>> it seems that the simplest workaround should simply add the additional
>>>> cred == real_cred into proc_pid_attr_write().
>>> Yes, that is simple, but I worry about what other LSMs might want to
>>> do. While I believe failing if the effective creds are not the same
>>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>>> about what other LSMs may want to do. After all,
>>> proc_pid_attr_write() doesn't change the the creds itself, that is
>>> something the specific LSMs do.
>> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
>> something is already wrong?
>>
>> In fact, I think that something is already wrong if it is not called by
>> user-space directly. Too late to ask, but why is this /proc/self/attr/
>> magic not implemented via syscall(s) ?
>
> Shell scripts, for one thing. It's a straightforward and appropriate
> use of the /proc interface. System calls would require additional change
> to existing programs, whereas using the /proc interface allows a good
> deal to be done in the containing scripts.
It is fairly awkward/fragile to use these interfaces from shell scripts
due to limitations of the interface (e.g. no partial writes, thereby
breaking if the shell splits up the data into multiple write() calls)
and due to the fact that most of the attributes (except for current) are
typically reset/cleared upon exec to prevent undue caller/callee
influence. It works well enough for echo -n "somelabel" >
/proc/self/attr/current but not much else, and even that doesn't work
without the -n due to multiple write() calls.
Just for the record, this functionality was originally implemented via
separate system calls at least for SELinux (in its original kernel
patch), then multiplexed through the single LSM security system call
upon porting to LSM (before merging to mainline), but viro and others
strongly favored using /proc as the interface for getting/setting any
new process attributes. Understandable, but it does impose some
limitations, including a dependency on having a writable proc mount in
the namespace of any process that needs to set any of these attributes,
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Khalid Aziz @ 2019-04-18 14:34 UTC (permalink / raw)
To: Kees Cook, Andy Lutomirski
Cc: Linus Torvalds, Thomas Gleixner, Nadav Amit, Ingo Molnar,
Juerg Haefliger, Tycho Andersen, Julian Stecklina,
Konrad Rzeszutek Wilk, Juerg Haefliger, deepa.srinivasan,
chris hyser, Tyler Hicks, David Woodhouse, Andrew Cooper,
Jon Masters, Boris Ostrovsky, iommu, X86 ML,
linux-alpha@vger.kernel.org, open list:DOCUMENTATION,
Linux List Kernel Mailing, Linux-MM, LSM List, Khalid Aziz,
Andrew Morton, Peter Zijlstra, Dave Hansen, Borislav Petkov,
H. Peter Anvin, Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <CAGXu5jL-qJtW7eH8S2yhqciE+J+FWz8HHzTrGJTgVUbd55n6dQ@mail.gmail.com>
On 4/17/19 11:41 PM, Kees Cook wrote:
> On Wed, Apr 17, 2019 at 11:41 PM Andy Lutomirski <luto@kernel.org> wrote:
>> I don't think this type of NX goof was ever the argument for XPFO.
>> The main argument I've heard is that a malicious user program writes a
>> ROP payload into user memory (regular anonymous user memory) and then
>> gets the kernel to erroneously set RSP (*not* RIP) to point there.
>
> Well, more than just ROP. Any of the various attack primitives. The NX
> stuff is about moving RIP: SMEP-bypassing. But there is still basic
> SMAP-bypassing for putting a malicious structure in userspace and
> having the kernel access it via the linear mapping, etc.
>
>> I find this argument fairly weak for a couple reasons. First, if
>> we're worried about this, let's do in-kernel CFI, not XPFO, to
>
> CFI is getting much closer. Getting the kernel happy under Clang, LTO,
> and CFI is under active development. (It's functional for arm64
> already, and pieces have been getting upstreamed.)
>
CFI theoretically offers protection with fairly low overhead. I have not
played much with CFI in clang. I agree with Linus that probability of
bugs in XPFO implementation itself is a cause of concern. If CFI in
Clang can provide us the same level of protection as XPFO does, I
wouldn't want to push for an expensive change like XPFO.
If Clang/CFI can't get us there for extended period of time, does it
make sense to continue to poke at XPFO?
Thanks,
Khalid
^ permalink raw reply
* [PATCH 0/3] RFC: add init_allocations=1 boot option
From: Alexander Potapenko @ 2019-04-18 15:42 UTC (permalink / raw)
To: akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
Following the recent discussions here's another take at initializing
pages and heap objects with zeroes. This is needed to prevent possible
information leaks and make the control-flow bugs that depend on
uninitialized values more deterministic.
The patchset introduces a new boot option, init_allocations, which
makes page allocator and SL[AOU]B initialize newly allocated memory.
init_allocations=0 doesn't (hopefully) add any overhead to the
allocation fast path (no noticeable slowdown on hackbench).
With only the the first of the proposed patches the slowdown numbers are:
- 1.1% (stdev 0.2%) sys time slowdown building Linux kernel
- 3.1% (stdev 0.3%) sys time slowdown on af_inet_loopback benchmark
- 9.4% (stdev 0.5%) sys time slowdown on hackbench
The second patch introduces a GFP flag that allows to disable
initialization for certain allocations. The third page is an example of
applying it to af_unix.c, which helps hackbench greatly.
Slowdown numbers for the whole patchset are:
- 1.8% (stdev 0.8%) on kernel build
- 6.5% (stdev 0.2%) on af_inet_loopback
- 0.12% (stdev 0.6%) on hackbench
Alexander Potapenko (3):
mm: security: introduce the init_allocations=1 boot option
gfp: mm: introduce __GFP_NOINIT
net: apply __GFP_NOINIT to AF_UNIX sk_buff allocations
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/gfp.h | 6 ++++-
include/linux/mm.h | 8 +++++++
include/linux/slab_def.h | 1 +
include/linux/slub_def.h | 1 +
include/net/sock.h | 5 +++++
kernel/kexec_core.c | 4 ++--
mm/dmapool.c | 2 +-
mm/page_alloc.c | 18 ++++++++++++++-
mm/slab.c | 14 ++++++------
mm/slab.h | 1 +
mm/slab_common.c | 15 +++++++++++++
mm/slob.c | 3 ++-
mm/slub.c | 9 ++++----
net/core/sock.c | 31 +++++++++++++++++++++-----
net/unix/af_unix.c | 13 ++++++-----
16 files changed, 104 insertions(+), 29 deletions(-)
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply
* [PATCH 1/3] mm: security: introduce the init_allocations=1 boot option
From: Alexander Potapenko @ 2019-04-18 15:42 UTC (permalink / raw)
To: akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190418154208.131118-1-glider@google.com>
This option adds the possibility to initialize newly allocated pages and
heap objects with zeroes. This is needed to prevent possible information
leaks and make the control-flow bugs that depend on uninitialized values
more deterministic.
Initialization is done at allocation time at the places where checks for
__GFP_ZERO are performed. We don't initialize slab caches with
constructors to preserve their semantics. To reduce runtime costs of
checking cachep->ctor we replace a call to memset with a call to
cachep->poison_fn, which is only executed if the memory block needs to
be initialized.
For kernel testing purposes filling allocations with a nonzero pattern
would be more suitable, but may require platform-specific code. To have
a simple baseline we've decided to start with zero-initialization.
No performance optimizations are done at the moment to reduce double
initialization of memory regions.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Qian Cai <cai@lca.pw>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 8 ++++++++
include/linux/slab_def.h | 1 +
include/linux/slub_def.h | 1 +
kernel/kexec_core.c | 2 +-
mm/dmapool.c | 2 +-
mm/page_alloc.c | 18 +++++++++++++++++-
mm/slab.c | 12 ++++++------
mm/slab.h | 1 +
mm/slab_common.c | 15 +++++++++++++++
mm/slob.c | 2 +-
mm/slub.c | 8 ++++----
net/core/sock.c | 2 +-
13 files changed, 58 insertions(+), 16 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index e1379949e663..f31234906be2 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -127,7 +127,7 @@ __malloc void *_uverbs_alloc(struct uverbs_attr_bundle *bundle, size_t size,
res = (void *)pbundle->internal_buffer + pbundle->internal_used;
pbundle->internal_used =
ALIGN(new_used, sizeof(*pbundle->internal_buffer));
- if (flags & __GFP_ZERO)
+ if (want_init_memory(flags))
memset(res, 0, size);
return res;
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 76769749b5a5..b38b71a5efaa 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2597,6 +2597,14 @@ static inline void kernel_poison_pages(struct page *page, int numpages,
int enable) { }
#endif
+DECLARE_STATIC_KEY_FALSE(init_allocations);
+static inline bool want_init_memory(gfp_t flags)
+{
+ if (static_branch_unlikely(&init_allocations))
+ return true;
+ return flags & __GFP_ZERO;
+}
+
#ifdef CONFIG_DEBUG_PAGEALLOC
extern bool _debug_pagealloc_enabled;
extern void __kernel_map_pages(struct page *page, int numpages, int enable);
diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index 9a5eafb7145b..9dfe9eb639d7 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -37,6 +37,7 @@ struct kmem_cache {
/* constructor func */
void (*ctor)(void *obj);
+ void (*poison_fn)(struct kmem_cache *c, void *object);
/* 4) cache creation/removal */
const char *name;
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index d2153789bd9f..afb928cb7c20 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -99,6 +99,7 @@ struct kmem_cache {
gfp_t allocflags; /* gfp flags to use on each alloc */
int refcount; /* Refcount for slab cache destroy */
void (*ctor)(void *);
+ void (*poison_fn)(struct kmem_cache *c, void *object);
unsigned int inuse; /* Offset to metadata */
unsigned int align; /* Alignment */
unsigned int red_left_pad; /* Left redzone padding size */
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index d7140447be75..be84f5f95c97 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
arch_kexec_post_alloc_pages(page_address(pages), count,
gfp_mask);
- if (gfp_mask & __GFP_ZERO)
+ if (want_init_memory(gfp_mask))
for (i = 0; i < count; i++)
clear_highpage(pages + i);
}
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 76a160083506..796e38160d39 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -381,7 +381,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
#endif
spin_unlock_irqrestore(&pool->lock, flags);
- if (mem_flags & __GFP_ZERO)
+ if (want_init_memory(mem_flags))
memset(retval, 0, pool->size);
return retval;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d96ca5bc555b..e2a21d866ac9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -133,6 +133,22 @@ unsigned long totalcma_pages __read_mostly;
int percpu_pagelist_fraction;
gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
+bool want_init_allocations __read_mostly;
+EXPORT_SYMBOL(want_init_allocations);
+DEFINE_STATIC_KEY_FALSE(init_allocations);
+
+static int __init early_init_allocations(char *buf)
+{
+ int ret;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &want_init_allocations);
+ if (want_init_allocations)
+ static_branch_enable(&init_allocations);
+ return ret;
+}
+early_param("init_allocations", early_init_allocations);
/*
* A cached value of the page's pageblock's migratetype, used when the page is
@@ -2014,7 +2030,7 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
post_alloc_hook(page, order, gfp_flags);
- if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
+ if (!free_pages_prezeroed() && want_init_memory(gfp_flags))
for (i = 0; i < (1 << order); i++)
clear_highpage(page + i);
diff --git a/mm/slab.c b/mm/slab.c
index 47a380a486ee..dcc5b73cf767 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3331,8 +3331,8 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
local_irq_restore(save_flags);
ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
- if (unlikely(flags & __GFP_ZERO) && ptr)
- memset(ptr, 0, cachep->object_size);
+ if (unlikely(want_init_memory(flags)) && ptr)
+ cachep->poison_fn(cachep, ptr);
slab_post_alloc_hook(cachep, flags, 1, &ptr);
return ptr;
@@ -3388,8 +3388,8 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
prefetchw(objp);
- if (unlikely(flags & __GFP_ZERO) && objp)
- memset(objp, 0, cachep->object_size);
+ if (unlikely(want_init_memory(flags)) && objp)
+ cachep->poison_fn(cachep, objp);
slab_post_alloc_hook(cachep, flags, 1, &objp);
return objp;
@@ -3596,9 +3596,9 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
cache_alloc_debugcheck_after_bulk(s, flags, size, p, _RET_IP_);
/* Clear memory outside IRQ disabled section */
- if (unlikely(flags & __GFP_ZERO))
+ if (unlikely(want_init_memory(flags)))
for (i = 0; i < size; i++)
- memset(p[i], 0, s->object_size);
+ s->poison_fn(s, p[i]);
slab_post_alloc_hook(s, flags, size, p);
/* FIXME: Trace call missing. Christoph would like a bulk variant */
diff --git a/mm/slab.h b/mm/slab.h
index 43ac818b8592..3b541e8970ee 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -27,6 +27,7 @@ struct kmem_cache {
const char *name; /* Slab name for sysfs */
int refcount; /* Use counter */
void (*ctor)(void *); /* Called on object slot creation */
+ void (*poison_fn)(struct kmem_cache *c, void *object);
struct list_head list; /* List of all slab caches on the system */
};
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 58251ba63e4a..37810114b2ea 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -360,6 +360,16 @@ struct kmem_cache *find_mergeable(unsigned int size, unsigned int align,
return NULL;
}
+static void poison_zero(struct kmem_cache *c, void *object)
+{
+ memset(object, 0, c->object_size);
+}
+
+static void poison_dont(struct kmem_cache *c, void *object)
+{
+ /* Do nothing. Use for caches with constructors. */
+}
+
static struct kmem_cache *create_cache(const char *name,
unsigned int object_size, unsigned int align,
slab_flags_t flags, unsigned int useroffset,
@@ -381,6 +391,10 @@ static struct kmem_cache *create_cache(const char *name,
s->size = s->object_size = object_size;
s->align = align;
s->ctor = ctor;
+ if (ctor)
+ s->poison_fn = poison_dont;
+ else
+ s->poison_fn = poison_zero;
s->useroffset = useroffset;
s->usersize = usersize;
@@ -974,6 +988,7 @@ void __init create_boot_cache(struct kmem_cache *s, const char *name,
s->align = calculate_alignment(flags, ARCH_KMALLOC_MINALIGN, size);
s->useroffset = useroffset;
s->usersize = usersize;
+ s->poison_fn = poison_zero;
slab_init_memcg_params(s);
diff --git a/mm/slob.c b/mm/slob.c
index 307c2c9feb44..18981a71e962 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -330,7 +330,7 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node)
BUG_ON(!b);
spin_unlock_irqrestore(&slob_lock, flags);
}
- if (unlikely(gfp & __GFP_ZERO))
+ if (unlikely(want_init_memory(gfp)))
memset(b, 0, size);
return b;
}
diff --git a/mm/slub.c b/mm/slub.c
index d30ede89f4a6..e4efb6575510 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2750,8 +2750,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
stat(s, ALLOC_FASTPATH);
}
- if (unlikely(gfpflags & __GFP_ZERO) && object)
- memset(object, 0, s->object_size);
+ if (unlikely(want_init_memory(gfpflags)) && object)
+ s->poison_fn(s, object);
slab_post_alloc_hook(s, gfpflags, 1, &object);
@@ -3172,11 +3172,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
local_irq_enable();
/* Clear memory outside IRQ disabled fastpath loop */
- if (unlikely(flags & __GFP_ZERO)) {
+ if (unlikely(want_init_memory(flags))) {
int j;
for (j = 0; j < i; j++)
- memset(p[j], 0, s->object_size);
+ s->poison_fn(s, p[j]);
}
/* memcg and kmem_cache debug support */
diff --git a/net/core/sock.c b/net/core/sock.c
index 782343bb925b..99b288a19b39 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1601,7 +1601,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
if (!sk)
return sk;
- if (priority & __GFP_ZERO)
+ if (want_init_memory(priority))
sk_prot_clear_nulls(sk, prot->obj_size);
} else
sk = kmalloc(prot->obj_size, priority);
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 2/3] gfp: mm: introduce __GFP_NOINIT
From: Alexander Potapenko @ 2019-04-18 15:42 UTC (permalink / raw)
To: akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190418154208.131118-1-glider@google.com>
When passed to an allocator (either pagealloc or SL[AOU]B), __GFP_NOINIT
tells it to not initialize the requested memory if the init_allocations
boot option is enabled. This can be useful in the cases the newly
allocated memory is going to be initialized by the caller right away.
__GFP_NOINIT basically defeats the hardening against information leaks
provided by the init_allocations feature, so one should use it with
caution.
This patch also adds __GFP_NOINIT to alloc_pages() calls in SL[AOU]B.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Qian Cai <cai@lca.pw>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
include/linux/gfp.h | 6 +++++-
include/linux/mm.h | 2 +-
kernel/kexec_core.c | 2 +-
mm/slab.c | 2 +-
mm/slob.c | 1 +
mm/slub.c | 1 +
6 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index fdab7de7490d..66d7f5604fe2 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -44,6 +44,7 @@ struct vm_area_struct;
#else
#define ___GFP_NOLOCKDEP 0
#endif
+#define ___GFP_NOINIT 0x1000000u
/* If the above are modified, __GFP_BITS_SHIFT may need updating */
/*
@@ -208,16 +209,19 @@ struct vm_area_struct;
* %__GFP_COMP address compound page metadata.
*
* %__GFP_ZERO returns a zeroed page on success.
+ *
+ * %__GFP_NOINIT requests non-initialized memory from the underlying allocator.
*/
#define __GFP_NOWARN ((__force gfp_t)___GFP_NOWARN)
#define __GFP_COMP ((__force gfp_t)___GFP_COMP)
#define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)
+#define __GFP_NOINIT ((__force gfp_t)___GFP_NOINIT)
/* Disable lockdep for GFP context tracking */
#define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
/* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
+#define __GFP_BITS_SHIFT (25)
#define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
/**
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b38b71a5efaa..8f03334a9033 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2601,7 +2601,7 @@ DECLARE_STATIC_KEY_FALSE(init_allocations);
static inline bool want_init_memory(gfp_t flags)
{
if (static_branch_unlikely(&init_allocations))
- return true;
+ return !(flags & __GFP_NOINIT);
return flags & __GFP_ZERO;
}
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index be84f5f95c97..f9d1f1236cd0 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -302,7 +302,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
{
struct page *pages;
- pages = alloc_pages(gfp_mask & ~__GFP_ZERO, order);
+ pages = alloc_pages((gfp_mask & ~__GFP_ZERO) | __GFP_NOINIT, order);
if (pages) {
unsigned int count, i;
diff --git a/mm/slab.c b/mm/slab.c
index dcc5b73cf767..762cb0e7bcc1 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1393,7 +1393,7 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
struct page *page;
int nr_pages;
- flags |= cachep->allocflags;
+ flags |= (cachep->allocflags | __GFP_NOINIT);
page = __alloc_pages_node(nodeid, flags, cachep->gfporder);
if (!page) {
diff --git a/mm/slob.c b/mm/slob.c
index 18981a71e962..867d2d68a693 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -192,6 +192,7 @@ static void *slob_new_pages(gfp_t gfp, int order, int node)
{
void *page;
+ gfp |= __GFP_NOINIT;
#ifdef CONFIG_NUMA
if (node != NUMA_NO_NODE)
page = __alloc_pages_node(node, gfp, order);
diff --git a/mm/slub.c b/mm/slub.c
index e4efb6575510..a79b4cb768a2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1493,6 +1493,7 @@ static inline struct page *alloc_slab_page(struct kmem_cache *s,
struct page *page;
unsigned int order = oo_order(oo);
+ flags |= __GFP_NOINIT;
if (node == NUMA_NO_NODE)
page = alloc_pages(flags, order);
else
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 3/3] RFC: net: apply __GFP_NOINIT to AF_UNIX sk_buff allocations
From: Alexander Potapenko @ 2019-04-18 15:42 UTC (permalink / raw)
To: akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190418154208.131118-1-glider@google.com>
Add sock_alloc_send_pskb_noinit(), which is similar to
sock_alloc_send_pskb(), but allocates with __GFP_NOINIT.
This helps reduce the slowdown on hackbench from 9% to 0.1%.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Qian Cai <cai@lca.pw>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Eric Dumazet <edumazet@google.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
include/net/sock.h | 5 +++++
net/core/sock.c | 29 +++++++++++++++++++++++++----
net/unix/af_unix.c | 13 +++++++------
3 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 8de5ee258b93..37fcdda23884 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1612,6 +1612,11 @@ struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
unsigned long data_len, int noblock,
int *errcode, int max_page_order);
+struct sk_buff *sock_alloc_send_pskb_noinit(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order);
void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
void sock_kfree_s(struct sock *sk, void *mem, int size);
void sock_kzfree_s(struct sock *sk, void *mem, int size);
diff --git a/net/core/sock.c b/net/core/sock.c
index 99b288a19b39..0a2af1e1fa1c 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2187,9 +2187,11 @@ static long sock_wait_for_wmem(struct sock *sk, long timeo)
* Generic send/receive buffer handlers
*/
-struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
- unsigned long data_len, int noblock,
- int *errcode, int max_page_order)
+struct sk_buff *sock_alloc_send_pskb_internal(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order, gfp_t gfp)
{
struct sk_buff *skb;
long timeo;
@@ -2218,7 +2220,7 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
timeo = sock_wait_for_wmem(sk, timeo);
}
skb = alloc_skb_with_frags(header_len, data_len, max_page_order,
- errcode, sk->sk_allocation);
+ errcode, sk->sk_allocation | gfp);
if (skb)
skb_set_owner_w(skb, sk);
return skb;
@@ -2229,8 +2231,27 @@ struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
*errcode = err;
return NULL;
}
+
+struct sk_buff *sock_alloc_send_pskb(struct sock *sk, unsigned long header_len,
+ unsigned long data_len, int noblock,
+ int *errcode, int max_page_order)
+{
+ return sock_alloc_send_pskb_internal(sk, header_len, data_len,
+ noblock, errcode, max_page_order, /*gfp*/0);
+}
EXPORT_SYMBOL(sock_alloc_send_pskb);
+struct sk_buff *sock_alloc_send_pskb_noinit(struct sock *sk,
+ unsigned long header_len,
+ unsigned long data_len,
+ int noblock, int *errcode,
+ int max_page_order)
+{
+ return sock_alloc_send_pskb_internal(sk, header_len, data_len,
+ noblock, errcode, max_page_order, /*gfp*/__GFP_NOINIT);
+}
+EXPORT_SYMBOL(sock_alloc_send_pskb_noinit);
+
struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
int noblock, int *errcode)
{
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ddb838a1b74c..9a45824c3c48 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1627,9 +1627,9 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
BUILD_BUG_ON(SKB_MAX_ALLOC < PAGE_SIZE);
}
- skb = sock_alloc_send_pskb(sk, len - data_len, data_len,
- msg->msg_flags & MSG_DONTWAIT, &err,
- PAGE_ALLOC_COSTLY_ORDER);
+ skb = sock_alloc_send_pskb_noinit(sk, len - data_len, data_len,
+ msg->msg_flags & MSG_DONTWAIT, &err,
+ PAGE_ALLOC_COSTLY_ORDER);
if (skb == NULL)
goto out;
@@ -1824,9 +1824,10 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
data_len = min_t(size_t, size, PAGE_ALIGN(data_len));
- skb = sock_alloc_send_pskb(sk, size - data_len, data_len,
- msg->msg_flags & MSG_DONTWAIT, &err,
- get_order(UNIX_SKB_FRAGS_SZ));
+ skb = sock_alloc_send_pskb_noinit(sk, size - data_len, data_len,
+ msg->msg_flags & MSG_DONTWAIT,
+ &err,
+ get_order(UNIX_SKB_FRAGS_SZ));
if (!skb)
goto out_err;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: [PATCH 0/3] RFC: add init_allocations=1 boot option
From: Alexander Potapenko @ 2019-04-18 15:44 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter, Dmitriy Vyukov, Kees Cook,
Laura Abbott
Cc: Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190418154208.131118-1-glider@google.com>
On Thu, Apr 18, 2019 at 5:42 PM Alexander Potapenko <glider@google.com> wrote:
>
> Following the recent discussions here's another take at initializing
> pages and heap objects with zeroes. This is needed to prevent possible
> information leaks and make the control-flow bugs that depend on
> uninitialized values more deterministic.
>
> The patchset introduces a new boot option, init_allocations, which
> makes page allocator and SL[AOU]B initialize newly allocated memory.
> init_allocations=0 doesn't (hopefully) add any overhead to the
> allocation fast path (no noticeable slowdown on hackbench).
>
> With only the the first of the proposed patches the slowdown numbers are:
> - 1.1% (stdev 0.2%) sys time slowdown building Linux kernel
> - 3.1% (stdev 0.3%) sys time slowdown on af_inet_loopback benchmark
> - 9.4% (stdev 0.5%) sys time slowdown on hackbench
>
> The second patch introduces a GFP flag that allows to disable
> initialization for certain allocations. The third page is an example of
> applying it to af_unix.c, which helps hackbench greatly.
>
> Slowdown numbers for the whole patchset are:
> - 1.8% (stdev 0.8%) on kernel build
> - 6.5% (stdev 0.2%) on af_inet_loopback
> - 0.12% (stdev 0.6%) on hackbench
>
>
> Alexander Potapenko (3):
> mm: security: introduce the init_allocations=1 boot option
> gfp: mm: introduce __GFP_NOINIT
> net: apply __GFP_NOINIT to AF_UNIX sk_buff allocations
Oops, I was hoping git send-email would pull all the Cc: tags from the
patches and actually use them.
> drivers/infiniband/core/uverbs_ioctl.c | 2 +-
> include/linux/gfp.h | 6 ++++-
> include/linux/mm.h | 8 +++++++
> include/linux/slab_def.h | 1 +
> include/linux/slub_def.h | 1 +
> include/net/sock.h | 5 +++++
> kernel/kexec_core.c | 4 ++--
> mm/dmapool.c | 2 +-
> mm/page_alloc.c | 18 ++++++++++++++-
> mm/slab.c | 14 ++++++------
> mm/slab.h | 1 +
> mm/slab_common.c | 15 +++++++++++++
> mm/slob.c | 3 ++-
> mm/slub.c | 9 ++++----
> net/core/sock.c | 31 +++++++++++++++++++++-----
> net/unix/af_unix.c | 13 ++++++-----
> 16 files changed, 104 insertions(+), 29 deletions(-)
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 1/3] mm: security: introduce the init_allocations=1 boot option
From: Dave Hansen @ 2019-04-18 16:35 UTC (permalink / raw)
To: Alexander Potapenko, akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190418154208.131118-2-glider@google.com>
On 4/18/19 8:42 AM, Alexander Potapenko wrote:
> This option adds the possibility to initialize newly allocated pages and
> heap objects with zeroes. This is needed to prevent possible information
> leaks and make the control-flow bugs that depend on uninitialized values
> more deterministic.
Isn't it better to do this at free time rather than allocation time? If
doing it at free, you can't even have information leaks for pages that
are in the allocator.
^ permalink raw reply
* Re: [PATCH 1/3] mm: security: introduce the init_allocations=1 boot option
From: Alexander Potapenko @ 2019-04-18 16:43 UTC (permalink / raw)
To: Dave Hansen
Cc: Andrew Morton, Christoph Lameter, Dmitriy Vyukov, Kees Cook,
Laura Abbott, Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <981d439a-1107-2730-f27e-17635ee4a125@intel.com>
On Thu, Apr 18, 2019 at 6:35 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 4/18/19 8:42 AM, Alexander Potapenko wrote:
> > This option adds the possibility to initialize newly allocated pages and
> > heap objects with zeroes. This is needed to prevent possible information
> > leaks and make the control-flow bugs that depend on uninitialized values
> > more deterministic.
>
> Isn't it better to do this at free time rather than allocation time? If
> doing it at free, you can't even have information leaks for pages that
> are in the allocator.
I should have mentioned this in the patch description, as this
question is being asked every time I send a patch :)
If we want to avoid double initialization and take advantage of
__GFP_NOINIT (see the second and third patches in the series) we need
to do initialize the memory at allocation time, because free() and
free_pages() don't accept GFP flags.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 1/3] mm: security: introduce the init_allocations=1 boot option
From: Alexander Potapenko @ 2019-04-18 16:50 UTC (permalink / raw)
To: Dave Hansen
Cc: Andrew Morton, Christoph Lameter, Dmitriy Vyukov, Kees Cook,
Laura Abbott, Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=URD0WL+RE90ZE2FZM4=p2zE9V+YA2RW-LrWnuqYTwvKQ@mail.gmail.com>
On Thu, Apr 18, 2019 at 6:43 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Thu, Apr 18, 2019 at 6:35 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 4/18/19 8:42 AM, Alexander Potapenko wrote:
> > > This option adds the possibility to initialize newly allocated pages and
> > > heap objects with zeroes. This is needed to prevent possible information
> > > leaks and make the control-flow bugs that depend on uninitialized values
> > > more deterministic.
> >
> > Isn't it better to do this at free time rather than allocation time? If
> > doing it at free, you can't even have information leaks for pages that
> > are in the allocator.
> I should have mentioned this in the patch description, as this
> question is being asked every time I send a patch :)
> If we want to avoid double initialization and take advantage of
> __GFP_NOINIT (see the second and third patches in the series) we need
> to do initialize the memory at allocation time, because free() and
> free_pages() don't accept GFP flags.
On a second thought, double zeroing on memory reclaim should be quite rare.
Most of the speedup we gain with __GFP_NOINIT is because we assume
it's safe to not initialize memory that'll be overwritten anyway.
I'll need to check how e.g. hackbench behaves if we choose to zero
memory on free() (my guess would be it'll be slower than with
__GFP_NOINIT hack, albeit a little safer)
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 2/3] gfp: mm: introduce __GFP_NOINIT
From: Dave Hansen @ 2019-04-18 16:52 UTC (permalink / raw)
To: Alexander Potapenko, akpm, cl, dvyukov, keescook, labbott
Cc: linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190418154208.131118-3-glider@google.com>
On 4/18/19 8:42 AM, Alexander Potapenko wrote:
> __GFP_NOINIT basically defeats the hardening against information leaks
> provided by the init_allocations feature, so one should use it with
> caution.
Even more than that, shouldn't we try to use it only in places where
there is a demonstrated benefit, like performance data?
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index be84f5f95c97..f9d1f1236cd0 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -302,7 +302,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
> {
> struct page *pages;
>
> - pages = alloc_pages(gfp_mask & ~__GFP_ZERO, order);
> + pages = alloc_pages((gfp_mask & ~__GFP_ZERO) | __GFP_NOINIT, order);
> if (pages) {
> unsigned int count, i;
While this is probably not super security-sensitive, it's also not
performance sensitive.
> diff --git a/mm/slab.c b/mm/slab.c
> index dcc5b73cf767..762cb0e7bcc1 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1393,7 +1393,7 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
> struct page *page;
> int nr_pages;
>
> - flags |= cachep->allocflags;
> + flags |= (cachep->allocflags | __GFP_NOINIT);
>
> page = __alloc_pages_node(nodeid, flags, cachep->gfporder);
> if (!page) {
> diff --git a/mm/slob.c b/mm/slob.c
> index 18981a71e962..867d2d68a693 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -192,6 +192,7 @@ static void *slob_new_pages(gfp_t gfp, int order, int node)
> {
> void *page;
>
> + gfp |= __GFP_NOINIT;
> #ifdef CONFIG_NUMA
> if (node != NUMA_NO_NODE)
> page = __alloc_pages_node(node, gfp, order
> diff --git a/mm/slub.c b/mm/slub.c
> index e4efb6575510..a79b4cb768a2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1493,6 +1493,7 @@ static inline struct page *alloc_slab_page(struct kmem_cache *s,
> struct page *page;
> unsigned int order = oo_order(oo);
>
> + flags |= __GFP_NOINIT;
> if (node == NUMA_NO_NODE)
> page = alloc_pages(flags, order);
> else
>
These sl*b ones seem like a bad idea. We already have rules that sl*b
allocations must be initialized by callers, and we have reasonably
frequent bugs where the rules are broken.
Setting __GFP_NOINIT might be reasonable to do, though, for slabs that
have a constructor. We have much higher confidence that *those* are
going to get initialized properly.
^ permalink raw reply
* Re: [PATCH V32 01/27] Add the ability to lock down access to the running kernel image
From: Matthew Garrett @ 2019-04-18 19:35 UTC (permalink / raw)
To: Andrew Donnellan
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells,
Linux API, Andy Lutomirski, linuxppc-dev, Michael Ellerman,
Daniel Axtens, cmr
In-Reply-To: <059c523e-926c-24ee-0935-198031712145@au1.ibm.com>
On Tue, Apr 16, 2019 at 1:40 AM Andrew Donnellan
<andrew.donnellan@au1.ibm.com> wrote:
> I'm thinking about whether we should lock down the powerpc xmon debug
> monitor - intuitively, I think the answer is yes if for no other reason
> than Least Astonishment, when lockdown is enabled you probably don't
> expect xmon to keep letting you access kernel memory.
The original patchset contained a sysrq hotkey to allow physically
present users to disable lockdown, so I'm not super concerned about
this case - I could definitely be convinced otherwise, though.
^ 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