* [PATCH v11 03/13] PKCS#7: Introduce pkcs7_get_digest()
From: Thiago Jung Bauermann @ 2019-06-11 6:28 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: <20190611062817.18412-1-bauerman@linux.ibm.com>
IMA will need to access the digest of the PKCS7 message (as calculated by
the kernel) before the signature is verified, so introduce
pkcs7_get_digest() for that purpose.
Also, modify pkcs7_digest() to detect when the digest was already
calculated so that it doesn't have to do redundant work. Verifying that
sinfo->sig->digest isn't NULL is sufficient because both places which
allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info())
use kzalloc() so sig->digest is always initialized to zero.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
crypto/asymmetric_keys/pkcs7_verify.c | 33 +++++++++++++++++++++++++++
include/crypto/pkcs7.h | 4 ++++
2 files changed, 37 insertions(+)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index f7b0980bf02d..3243981152b5 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/asn1.h>
#include <crypto/hash.h>
+#include <crypto/hash_info.h>
#include <crypto/public_key.h>
#include "pkcs7_parser.h"
@@ -33,6 +34,10 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
+ /* The digest was calculated already. */
+ if (sig->digest)
+ return 0;
+
if (!sinfo->sig->hash_algo)
return -ENOPKG;
@@ -121,6 +126,34 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return ret;
}
+int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf, u32 *len,
+ enum hash_algo *hash_algo)
+{
+ struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
+ int i, ret;
+
+ /*
+ * This function doesn't support messages with more than one signature.
+ */
+ if (sinfo == NULL || sinfo->next != NULL)
+ return -EBADMSG;
+
+ ret = pkcs7_digest(pkcs7, sinfo);
+ if (ret)
+ return ret;
+
+ *buf = sinfo->sig->digest;
+ *len = sinfo->sig->digest_size;
+
+ for (i = 0; i < HASH_ALGO__LAST; i++)
+ if (!strcmp(hash_algo_name[i], sinfo->sig->hash_algo)) {
+ *hash_algo = i;
+ break;
+ }
+
+ return 0;
+}
+
/*
* Find the key (X.509 certificate) to use to verify a PKCS#7 message. PKCS#7
* uses the issuer's name and the issuing certificate serial number for
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..3bfe6829eaae 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -13,6 +13,7 @@
#define _CRYPTO_PKCS7_H
#include <linux/verification.h>
+#include <linux/hash_info.h>
#include <crypto/public_key.h>
struct key;
@@ -44,4 +45,7 @@ extern int pkcs7_verify(struct pkcs7_message *pkcs7,
extern int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
const void *data, size_t datalen);
+extern int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf,
+ u32 *len, enum hash_algo *hash_algo);
+
#endif /* _CRYPTO_PKCS7_H */
^ permalink raw reply related
* [PATCH v11 02/13] PKCS#7: Refactor verify_pkcs7_signature()
From: Thiago Jung Bauermann @ 2019-06-11 6:28 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: <20190611062817.18412-1-bauerman@linux.ibm.com>
IMA will need to verify a PKCS#7 signature which has already been parsed.
For this reason, factor out the code which does that from
verify_pkcs7_signature() into a new function which takes a struct
pkcs7_message instead of a data buffer.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
certs/system_keyring.c | 61 ++++++++++++++++++++++++++----------
include/linux/verification.h | 10 ++++++
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index c05c29ae4d5d..4ba82e52e4b4 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -194,33 +194,27 @@ late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
/**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
* @data: The data to be verified (NULL if expecting internal data).
* @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
* @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
* (void *)1UL for all trusted keys).
* @usage: The use to which the key is being put.
* @view_content: Callback to gain access to content.
* @ctx: Context for callback.
*/
-int verify_pkcs7_signature(const void *data, size_t len,
- const void *raw_pkcs7, size_t pkcs7_len,
- struct key *trusted_keys,
- enum key_being_used_for usage,
- int (*view_content)(void *ctx,
- const void *data, size_t len,
- size_t asn1hdrlen),
- void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
{
- struct pkcs7_message *pkcs7;
int ret;
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -273,6 +267,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+ const void *raw_pkcs7, size_t pkcs7_len,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+ view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/include/linux/verification.h b/include/linux/verification.h
index 018fb5f13d44..5e1d41f2b336 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -36,6 +36,7 @@ extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
struct key;
+struct pkcs7_message;
extern int verify_pkcs7_signature(const void *data, size_t len,
const void *raw_pkcs7, size_t pkcs7_len,
@@ -45,6 +46,15 @@ extern int verify_pkcs7_signature(const void *data, size_t len,
const void *data, size_t len,
size_t asn1hdrlen),
void *ctx);
+extern int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data,
+ size_t len,
+ size_t asn1hdrlen),
+ void *ctx);
#ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
^ permalink raw reply related
* [PATCH v11 01/13] MODSIGN: Export module signature definitions
From: Thiago Jung Bauermann @ 2019-06-11 6:28 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: <20190611062817.18412-1-bauerman@linux.ibm.com>
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on either
CONFIG_MODULE_SIG or CONFIG_MODULES.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: Jessica Yu <jeyu@kernel.org>
---
include/linux/module.h | 3 --
include/linux/module_signature.h | 44 +++++++++++++++++++++++++
init/Kconfig | 6 +++-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 46 ++++++++++++++++++++++++++
kernel/module_signing.c | 56 +++++---------------------------
scripts/Makefile | 2 +-
8 files changed, 106 insertions(+), 53 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 188998d3dca9..aa56f531cf1e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..523617fc5b6a
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP, /* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+};
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8b9ffe236e4f..c2286a3c74c5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1852,6 +1852,10 @@ config BASE_SMALL
default 0 if BASE_FULL
default 1 if !BASE_FULL
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
menuconfig MODULES
bool "Enable loadable module support"
option modules
@@ -1929,7 +1933,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
diff --git a/kernel/Makefile b/kernel/Makefile
index 33824f0385b3..f29ae2997a43 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -58,6 +58,7 @@ endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b3aaf5..2712f4d217f5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include <linux/export.h>
#include <linux/extable.h>
#include <linux/moduleloader.h>
+#include <linux/module_signature.h>
#include <linux/trace_events.h>
#include <linux/init.h>
#include <linux/kallsyms.h>
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
new file mode 100644
index 000000000000..4224a1086b7d
--- /dev/null
+++ b/kernel/module_signature.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Module signature checker
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <linux/module_signature.h>
+#include <asm/byteorder.h>
+
+/**
+ * mod_check_sig - check that the given signature is sane
+ *
+ * @ms: Signature to check.
+ * @file_len: Size of the file to which @ms is appended.
+ * @name: What is being checked. Used for error messages.
+ */
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name)
+{
+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+ return -EBADMSG;
+
+ if (ms->id_type != PKEY_ID_PKCS7) {
+ pr_err("%s: Module is not signed with expected PKCS#7 message\n",
+ name);
+ return -ENOPKG;
+ }
+
+ if (ms->algo != 0 ||
+ ms->hash != 0 ||
+ ms->signer_len != 0 ||
+ ms->key_id_len != 0 ||
+ ms->__pad[0] != 0 ||
+ ms->__pad[1] != 0 ||
+ ms->__pad[2] != 0) {
+ pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
+ name);
+ return -EBADMSG;
+ }
+
+ return 0;
+}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b9a926fd86b..cdd04a6b8074 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,37 +11,13 @@
#include <linux/kernel.h>
#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/string.h>
#include <linux/verification.h>
#include <crypto/public_key.h>
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP, /* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
/*
* Verify the signature on a module.
*/
@@ -49,6 +25,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
{
struct module_signature ms;
size_t sig_len, modlen = info->len;
+ int ret;
pr_devel("==>%s(,%zu)\n", __func__, modlen);
@@ -56,32 +33,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
return -EBADMSG;
memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- modlen -= sizeof(ms);
+
+ ret = mod_check_sig(&ms, modlen, info->name);
+ if (ret)
+ return ret;
sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= modlen)
- return -EBADMSG;
- modlen -= sig_len;
+ modlen -= sig_len + sizeof(ms);
info->len = modlen;
- if (ms.id_type != PKEY_ID_PKCS7) {
- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
- info->name);
- return -ENOPKG;
- }
-
- if (ms.algo != 0 ||
- ms.hash != 0 ||
- ms.signer_len != 0 ||
- ms.key_id_len != 0 ||
- ms.__pad[0] != 0 ||
- ms.__pad[1] != 0 ||
- ms.__pad[2] != 0) {
- pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
- info->name);
- return -EBADMSG;
- }
-
return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
VERIFY_USE_SECONDARY_KEYRING,
VERIFYING_MODULE_SIGNATURE,
diff --git a/scripts/Makefile b/scripts/Makefile
index 9d442ee050bd..52098b080ab7 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,7 +17,7 @@ hostprogs-$(CONFIG_VT) += conmakehash
hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1) += asn1_compiler
-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
+hostprogs-$(CONFIG_MODULE_SIG_FORMAT) += sign-file
hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
^ permalink raw reply related
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Andy Lutomirski @ 2019-06-11 0:36 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <a329c4fa-adb0-09a4-7a8c-465f82e0e6c7@intel.com>
> On Jun 10, 2019, at 5:08 PM, Dave Hansen <dave.hansen@intel.com> wrote:
>
>> On 6/10/19 4:54 PM, Andy Lutomirski wrote:
>> Another benefit of kernel management: we could plausibly auto-clear
>> the bits corresponding to munmapped regions. Is this worth it?
>
> I did it for MPX. I think I even went to the trouble of zapping the
> whole pages that got unused.
>
> But, MPX tables took 80% of the address space, worst-case. This takes
> 0.003% :) The only case it would really matter would be a task was
> long-running, used legacy executables/JITs, and was mapping/unmapping
> text all over the address space. That seems rather unlikely.
Every wasted page still costs 4K plus page table overhead. The worst case is a JIT that doesn’t clean up and leaks legacy bitmap memory all over. We can blame the JIT, but the actual attribution could be complicated.
It also matters when you unmap one thing, map something else, and are sad when the legacy bits are still set.
Admittedly, it’s a bit hard to imagine the exploit that takes advantage of this.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-11 0:08 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <BBBF82D3-EE21-49E1-92A4-713C7729E6AD@amacapital.net>
On 6/10/19 4:54 PM, Andy Lutomirski wrote:
> Another benefit of kernel management: we could plausibly auto-clear
> the bits corresponding to munmapped regions. Is this worth it?
I did it for MPX. I think I even went to the trouble of zapping the
whole pages that got unused.
But, MPX tables took 80% of the address space, worst-case. This takes
0.003% :) The only case it would really matter would be a task was
long-running, used legacy executables/JITs, and was mapping/unmapping
text all over the address space. That seems rather unlikely.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Andy Lutomirski @ 2019-06-10 23:54 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <ea5e333f-8cd6-8396-635f-a9dc580d5364@intel.com>
> On Jun 10, 2019, at 3:59 PM, Dave Hansen <dave.hansen@intel.com> wrote:
>
>> On 6/10/19 3:40 PM, Yu-cheng Yu wrote:
>> Ok, we will go back to do_mmap() with MAP_PRIVATE, MAP_NORESERVE and
>> VM_DONTDUMP. The bitmap will cover only 48-bit address space.
>
> Could you make sure to discuss the downsides of only doing a 48-bit
> address space?
>
> What are the reasons behind and implications of VM_DONTDUMP?
>
>> We then create PR_MARK_CODE_AS_LEGACY. The kernel will set the bitmap, but it
>> is going to be slow.
>
> Slow compared to what? We're effectively adding one (quick) system call
> to a path that, today, has at *least* half a dozen syscalls and probably
> a bunch of page faults. Heck, we can probably avoid the actual page
> fault to populate the bitmap if we're careful. That alone would put a
> syscall on equal footing with any other approach. If the bit setting
> crossed a page boundary it would probably win.
>
>> Perhaps we still let the app fill the bitmap?
>
> I think I'd want to see some performance data on it first.
Trying to summarize:
If we manage the whole thing in user space, we are basically committing to only covering 48 bits — otherwise the whole model falls apart in quite a few ways. We gain some simplicity in the kernel.
If we do it in the kernel, we still have to decide how much address space to cover. We get to play games like allocating the bitmap above 2^48, but then we might have CRIU issues if we migrate to a system with fewer BA bits.
I doubt that the performance matters much one way or another. I just don’t expect any of this to be a bottleneck.
Another benefit of kernel management: we could plausibly auto-clear the bits corresponding to munmapped regions. Is this worth it?
And a maybe-silly benefit: if we manage it in the kernel, we could optimize the inevitable case where the bitmap contains pages that are all ones :). If it’s in userspace, KSM could do the, but that will be inefficient at best.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 23:37 UTC (permalink / raw)
To: H.J. Lu
Cc: Yu-cheng Yu, Andy Lutomirski, Peter Zijlstra,
the arch/x86 maintainers, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <CAMe9rOqLxNxE-gGX9ozX=emW9iQ+gOeUiS3ec5W4jmF6wk6cng@mail.gmail.com>
On 6/10/19 4:20 PM, H.J. Lu wrote:
>>> Perhaps we still let the app fill the bitmap?
>> I think I'd want to see some performance data on it first.
> Updating legacy bitmap in user space from kernel requires
>
> long q;
>
> get_user(q, ...);
> q |= mask;
> put_user(q, ...);
>
> instead of
>
> *p |= mask;
>
> get_user + put_user was quite slow when we tried before.
Numbers, please.
There are *lots* of ways to speed something like that up if you have
actual issues with it. For instance, you can skip the get_user() for
whole bytes. You can write bits with 0's for unallocated address space.
You can do user_access_begin/end() to avoid bunches of STAC/CLACs...
The list goes on and on. :)
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: H.J. Lu @ 2019-06-10 23:20 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, Andy Lutomirski, Peter Zijlstra,
the arch/x86 maintainers, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <ea5e333f-8cd6-8396-635f-a9dc580d5364@intel.com>
On Mon, Jun 10, 2019 at 3:59 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> > We then create PR_MARK_CODE_AS_LEGACY. The kernel will set the bitmap, but it
> > is going to be slow.
>
> Slow compared to what? We're effectively adding one (quick) system call
> to a path that, today, has at *least* half a dozen syscalls and probably
> a bunch of page faults. Heck, we can probably avoid the actual page
> fault to populate the bitmap if we're careful. That alone would put a
> syscall on equal footing with any other approach. If the bit setting
> crossed a page boundary it would probably win.
>
> > Perhaps we still let the app fill the bitmap?
>
> I think I'd want to see some performance data on it first.
Updating legacy bitmap in user space from kernel requires
long q;
get_user(q, ...);
q |= mask;
put_user(q, ...);
instead of
*p |= mask;
get_user + put_user was quite slow when we tried before.
--
H.J.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 22:59 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <1b961c71d30e31ecb22da2c5401b1a81cb802d86.camel@intel.com>
On 6/10/19 3:40 PM, Yu-cheng Yu wrote:
> Ok, we will go back to do_mmap() with MAP_PRIVATE, MAP_NORESERVE and
> VM_DONTDUMP. The bitmap will cover only 48-bit address space.
Could you make sure to discuss the downsides of only doing a 48-bit
address space?
What are the reasons behind and implications of VM_DONTDUMP?
> We then create PR_MARK_CODE_AS_LEGACY. The kernel will set the bitmap, but it
> is going to be slow.
Slow compared to what? We're effectively adding one (quick) system call
to a path that, today, has at *least* half a dozen syscalls and probably
a bunch of page faults. Heck, we can probably avoid the actual page
fault to populate the bitmap if we're careful. That alone would put a
syscall on equal footing with any other approach. If the bit setting
crossed a page boundary it would probably win.
> Perhaps we still let the app fill the bitmap?
I think I'd want to see some performance data on it first.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Yu-cheng Yu @ 2019-06-10 22:40 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <92e56b28-0cd4-e3f4-867b-639d9b98b86c@intel.com>
On Mon, 2019-06-10 at 15:02 -0700, Dave Hansen wrote:
> On 6/10/19 1:58 PM, Yu-cheng Yu wrote:
> > > > On each memory request, the kernel then must consider a percentage of
> > > > allocated space in its calculation, and on systems with less memory
> > > > this quickly becomes a problem.
> > >
> > > I'm not sure what you're referring to here? Are you referring to our
> > > overcommit limits?
> >
> > Yes.
>
> My assumption has always been that these large, potentially sparse
> hardware tables *must* be mmap()'d with MAP_NORESERVE specified. That
> should keep them from being problematic with respect to overcommit.
Ok, we will go back to do_mmap() with MAP_PRIVATE, MAP_NORESERVE and
VM_DONTDUMP. The bitmap will cover only 48-bit address space.
We then create PR_MARK_CODE_AS_LEGACY. The kernel will set the bitmap, but it
is going to be slow.
Perhaps we still let the app fill the bitmap?
Yu-cheng
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 22:02 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <328275c9b43c06809c9937c83d25126a6e3efcbd.camel@intel.com>
On 6/10/19 1:58 PM, Yu-cheng Yu wrote:
>>> On each memory request, the kernel then must consider a percentage of
>>> allocated space in its calculation, and on systems with less memory
>>> this quickly becomes a problem.
>> I'm not sure what you're referring to here? Are you referring to our
>> overcommit limits?
> Yes.
My assumption has always been that these large, potentially sparse
hardware tables *must* be mmap()'d with MAP_NORESERVE specified. That
should keep them from being problematic with respect to overcommit.
^ permalink raw reply
* Re: [PATCH v3 16/33] docs: locking: convert docs to ReST and rename to *.rst
From: Federico Vaga @ 2019-06-10 16:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Peter Zijlstra, Ingo Molnar, Will Deacon,
Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, dri-devel
In-Reply-To: <d5a915447d63fce96cbf463a512cce89423776c3.1560045490.git.mchehab+samsung@kernel.org>
In data Sunday, June 9, 2019 4:27:06 AM CEST, Mauro Carvalho Chehab ha
scritto:
> Convert the locking documents to ReST and add them to the
> kernel development book where it belongs.
>
> Most of the stuff here is just to make Sphinx to properly
> parse the text file, as they're already in good shape,
> not requiring massive changes in order to be parsed.
>
> The conversion is actually:
> - add blank lines and identation in order to identify paragraphs;
> - fix tables markups;
> - add some lists markups;
> - mark literal blocks;
> - adjust title markups.
>
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/kernel-hacking/locking.rst | 2 +-
> Documentation/locking/index.rst | 24 ++
> ...{lockdep-design.txt => lockdep-design.rst} | 51 ++--
> .../locking/{lockstat.txt => lockstat.rst} | 221 ++++++++++--------
> .../{locktorture.txt => locktorture.rst} | 105 +++++----
> .../{mutex-design.txt => mutex-design.rst} | 26 ++-
> ...t-mutex-design.txt => rt-mutex-design.rst} | 139 ++++++-----
> .../locking/{rt-mutex.txt => rt-mutex.rst} | 30 +--
> .../locking/{spinlocks.txt => spinlocks.rst} | 32 ++-
> ...w-mutex-design.txt => ww-mutex-design.rst} | 82 ++++---
> Documentation/pi-futex.txt | 2 +-
> .../it_IT/kernel-hacking/locking.rst | 2 +-
Limited to translations/it_IT
Acked-by: Federico Vaga <federico.vaga@vaga.pv.it>
> drivers/gpu/drm/drm_modeset_lock.c | 2 +-
> include/linux/lockdep.h | 2 +-
> include/linux/mutex.h | 2 +-
> include/linux/rwsem.h | 2 +-
> kernel/locking/mutex.c | 2 +-
> kernel/locking/rtmutex.c | 2 +-
> lib/Kconfig.debug | 4 +-
> 19 files changed, 428 insertions(+), 304 deletions(-)
> create mode 100644 Documentation/locking/index.rst
> rename Documentation/locking/{lockdep-design.txt => lockdep-design.rst}
> (93%) rename Documentation/locking/{lockstat.txt => lockstat.rst} (41%)
> rename Documentation/locking/{locktorture.txt => locktorture.rst} (57%)
> rename Documentation/locking/{mutex-design.txt => mutex-design.rst} (94%)
> rename Documentation/locking/{rt-mutex-design.txt => rt-mutex-design.rst}
> (91%) rename Documentation/locking/{rt-mutex.txt => rt-mutex.rst} (71%)
> rename Documentation/locking/{spinlocks.txt => spinlocks.rst} (89%) rename
> Documentation/locking/{ww-mutex-design.txt => ww-mutex-design.rst} (93%)
>
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Yu-cheng Yu @ 2019-06-10 20:58 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <ac9a20a6-170a-694e-beeb-605a17195034@intel.com>
On Mon, 2019-06-10 at 13:43 -0700, Dave Hansen wrote:
> On 6/10/19 1:27 PM, Yu-cheng Yu wrote:
> > > > If the loader cannot allocate a big bitmap to cover all 5-level
> > > > address space (the bitmap will be large), it can put all legacy lib's
> > > > at lower address. We cannot do these easily in the kernel.
> > >
> > > This is actually an argument to do it in the kernel. The kernel can
> > > always allocate the virtual space however it wants, no matter how large.
> > > If we hide the bitmap behind a kernel API then we can put it at high
> > > 5-level user addresses because we also don't have to worry about the
> > > high bits confusing userspace.
> >
> > We actually tried this. The kernel needs to reserve the bitmap space in the
> > beginning for every CET-enabled app, regardless of actual needs.
>
> I don't think this is a problem. In fact, I think reserving the space
> is actually the only sane behavior. If you don't reserve it, you
> fundamentally limit where future legacy instructions can go.
>
> One idea is that we always size the bitmap for the 48-bit addressing
> systems. Legacy code probably doesn't _need_ to go in the new address
> space, and if we do this we don't have to worry about the gigantic
> 57-bit address space bitmap.
>
> > On each memory request, the kernel then must consider a percentage of
> > allocated space in its calculation, and on systems with less memory
> > this quickly becomes a problem.
>
> I'm not sure what you're referring to here? Are you referring to our
> overcommit limits?
Yes.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 20:43 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <5c8727dde9653402eea97bfdd030c479d1e8dd99.camel@intel.com>
On 6/10/19 1:27 PM, Yu-cheng Yu wrote:
>>> If the loader cannot allocate a big bitmap to cover all 5-level
>>> address space (the bitmap will be large), it can put all legacy lib's
>>> at lower address. We cannot do these easily in the kernel.
>> This is actually an argument to do it in the kernel. The kernel can
>> always allocate the virtual space however it wants, no matter how large.
>> If we hide the bitmap behind a kernel API then we can put it at high
>> 5-level user addresses because we also don't have to worry about the
>> high bits confusing userspace.
> We actually tried this. The kernel needs to reserve the bitmap space in the
> beginning for every CET-enabled app, regardless of actual needs.
I don't think this is a problem. In fact, I think reserving the space
is actually the only sane behavior. If you don't reserve it, you
fundamentally limit where future legacy instructions can go.
One idea is that we always size the bitmap for the 48-bit addressing
systems. Legacy code probably doesn't _need_ to go in the new address
space, and if we do this we don't have to worry about the gigantic
57-bit address space bitmap.
> On each memory request, the kernel then must consider a percentage of
> allocated space in its calculation, and on systems with less memory
> this quickly becomes a problem.
I'm not sure what you're referring to here? Are you referring to our
overcommit limits?
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Yu-cheng Yu @ 2019-06-10 20:27 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <0665416d-9999-b394-df17-f2a5e1408130@intel.com>
On Mon, 2019-06-10 at 12:52 -0700, Dave Hansen wrote:
> On 6/10/19 12:38 PM, Yu-cheng Yu wrote:
> > > > When an application starts, its highest stack address is determined.
> > > > It uses that as the maximum the bitmap needs to cover.
> > >
> > > Huh, I didn't think we ran code from the stack. ;)
> > >
> > > Especially given the way that we implemented the new 5-level-paging
> > > address space, I don't think that expecting code to be below the stack
> > > is a good universal expectation.
> >
> > Yes, you make a good point. However, allowing the application manage the
> > bitmap
> > is the most efficient and flexible. If the loader finds a legacy lib is
> > beyond
> > the bitmap can cover, it can deal with the problem by moving the lib to a
> > lower
> > address; or re-allocate the bitmap.
>
> How could the loader reallocate the bitmap and coordinate with other
> users of the bitmap?
Assuming the loader actually chooses to re-allocate, it can copy the old bitmap
over to the new before doing the switch. But, I agree, the other choice is
easier; the loader can simply put the lib at lower address. AFAIK, the loader
does not request high address in mmap().
>
> > If the loader cannot allocate a big bitmap to cover all 5-level
> > address space (the bitmap will be large), it can put all legacy lib's
> > at lower address. We cannot do these easily in the kernel.
>
> This is actually an argument to do it in the kernel. The kernel can
> always allocate the virtual space however it wants, no matter how large.
> If we hide the bitmap behind a kernel API then we can put it at high
> 5-level user addresses because we also don't have to worry about the
> high bits confusing userspace.
We actually tried this. The kernel needs to reserve the bitmap space in the
beginning for every CET-enabled app, regardless of actual needs. On each memory
request, the kernel then must consider a percentage of allocated space in its
calculation, and on systems with less memory this quickly becomes a problem.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Andy Lutomirski @ 2019-06-10 19:55 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, Peter Zijlstra, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, LKML, open list:DOCUMENTATION,
Linux-MM, linux-arch, Linux API, Arnd Bergmann, Balbir Singh,
Borislav Petkov, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <0665416d-9999-b394-df17-f2a5e1408130@intel.com>
On Mon, Jun 10, 2019 at 12:52 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/10/19 12:38 PM, Yu-cheng Yu wrote:
> >>> When an application starts, its highest stack address is determined.
> >>> It uses that as the maximum the bitmap needs to cover.
> >> Huh, I didn't think we ran code from the stack. ;)
> >>
> >> Especially given the way that we implemented the new 5-level-paging
> >> address space, I don't think that expecting code to be below the stack
> >> is a good universal expectation.
> > Yes, you make a good point. However, allowing the application manage the bitmap
> > is the most efficient and flexible. If the loader finds a legacy lib is beyond
> > the bitmap can cover, it can deal with the problem by moving the lib to a lower
> > address; or re-allocate the bitmap.
>
> How could the loader reallocate the bitmap and coordinate with other
> users of the bitmap?
>
> > If the loader cannot allocate a big bitmap to cover all 5-level
> > address space (the bitmap will be large), it can put all legacy lib's
> > at lower address. We cannot do these easily in the kernel.
>
> This is actually an argument to do it in the kernel. The kernel can
> always allocate the virtual space however it wants, no matter how large.
> If we hide the bitmap behind a kernel API then we can put it at high
> 5-level user addresses because we also don't have to worry about the
> high bits confusing userspace.
>
That's a fairly compelling argument.
The bitmap is one bit per page, right? So it's smaller than the
address space by a factor of 8*2^12 == 2^15. This means that, if we
ever get full 64-bit linear addresses reserved entirely for userspace
(which could happen if my perennial request to Intel to split user and
kernel addresses completely happens), then we'll need 2^48 bytes for
the bitmap, which simply does not fit in the address space of a legacy
application.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 19:52 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <5aa98999b1343f34828414b74261201886ec4591.camel@intel.com>
On 6/10/19 12:38 PM, Yu-cheng Yu wrote:
>>> When an application starts, its highest stack address is determined.
>>> It uses that as the maximum the bitmap needs to cover.
>> Huh, I didn't think we ran code from the stack. ;)
>>
>> Especially given the way that we implemented the new 5-level-paging
>> address space, I don't think that expecting code to be below the stack
>> is a good universal expectation.
> Yes, you make a good point. However, allowing the application manage the bitmap
> is the most efficient and flexible. If the loader finds a legacy lib is beyond
> the bitmap can cover, it can deal with the problem by moving the lib to a lower
> address; or re-allocate the bitmap.
How could the loader reallocate the bitmap and coordinate with other
users of the bitmap?
> If the loader cannot allocate a big bitmap to cover all 5-level
> address space (the bitmap will be large), it can put all legacy lib's
> at lower address. We cannot do these easily in the kernel.
This is actually an argument to do it in the kernel. The kernel can
always allocate the virtual space however it wants, no matter how large.
If we hide the bitmap behind a kernel API then we can put it at high
5-level user addresses because we also don't have to worry about the
high bits confusing userspace.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Yu-cheng Yu @ 2019-06-10 19:38 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <3f19582d-78b1-5849-ffd0-53e8ca747c0d@intel.com>
On Mon, 2019-06-10 at 11:02 -0700, Dave Hansen wrote:
> On 6/10/19 8:22 AM, Yu-cheng Yu wrote:
> > > How does glibc know the linear address space size? We don’t want LA64 to
> > > break old binaries because the address calculation changed.
> >
> > When an application starts, its highest stack address is determined.
> > It uses that as the maximum the bitmap needs to cover.
>
> Huh, I didn't think we ran code from the stack. ;)
>
> Especially given the way that we implemented the new 5-level-paging
> address space, I don't think that expecting code to be below the stack
> is a good universal expectation.
Yes, you make a good point. However, allowing the application manage the bitmap
is the most efficient and flexible. If the loader finds a legacy lib is beyond
the bitmap can cover, it can deal with the problem by moving the lib to a lower
address; or re-allocate the bitmap. If the loader cannot allocate a big bitmap
to cover all 5-level address space (the bitmap will be large), it can put all
legacy lib's at lower address. We cannot do these easily in the kernel.
Yu-cheng
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 18:02 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <e26f7d09376740a5f7e8360fac4805488b2c0a4f.camel@intel.com>
On 6/10/19 8:22 AM, Yu-cheng Yu wrote:
>> How does glibc know the linear address space size? We don’t want LA64 to
>> break old binaries because the address calculation changed.
> When an application starts, its highest stack address is determined.
> It uses that as the maximum the bitmap needs to cover.
Huh, I didn't think we ran code from the stack. ;)
Especially given the way that we implemented the new 5-level-paging
address space, I don't think that expecting code to be below the stack
is a good universal expectation.
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Dave Hansen @ 2019-06-10 17:59 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: Peter Zijlstra, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <5dc357f5858f8036cad5847cfe214401bb9138bf.camel@intel.com>
On 6/10/19 9:05 AM, Yu-cheng Yu wrote:
> On Fri, 2019-06-07 at 14:09 -0700, Dave Hansen wrote:
>> On 6/7/19 1:06 PM, Yu-cheng Yu wrote:
>>>> Huh, how does glibc know about all possible past and future legacy code
>>>> in the application?
>>> When dlopen() gets a legacy binary and the policy allows that, it will
>>> manage
>>> the bitmap:
>>>
>>> If a bitmap has not been created, create one.
>>> Set bits for the legacy code being loaded.
>> I was thinking about code that doesn't go through GLIBC like JITs.
> If JIT manages the bitmap, it knows where it is.
> It can always read the bitmap again, right?
Let's just be clear:
The design proposed here is that all code mappers (anybody wanting to
get legacy non-CET code into the address space):
1. Know about CET
2. Know where the bitmap is, and identify the part that needs to be
changed
3. Be able to mprotect() the bitmap to be writable (undoing glibc's
PROT_READ)
4. Set the bits in the bitmap for the legacy code
5. mprotect() the bitmap back to PROT_READ
Do the non-glibc code mappers have glibc interfaces for this?
Otherwise, how could a bunch of JITs in a big multi-threaded application
possibly coordinate the mprotect()s? Won't they race with each other?
^ permalink raw reply
* Re: [PATCH v3 13/33] docs: infiniband: convert docs to ReST and rename to *.rst
From: Jonathan Corbet @ 2019-06-10 17:35 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Mauro Carvalho Chehab, Linux Doc Mailing List,
Mauro Carvalho Chehab, linux-kernel, Doug Ledford, linux-rdma
In-Reply-To: <20190610172712.GG18468@ziepe.ca>
On Mon, 10 Jun 2019 14:27:12 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:
> Looks OK to me, do you want to run these patches through the docs tree
> or through RDMA?
>
> Given that we've generally pushed doc updates through rdma, I think
> I'd prefer the latter? Jonathan?
Whichever works best for you is fine with me; go ahead and take them.
jon
^ permalink raw reply
* Re: [PATCH v7 03/14] x86/cet/ibt: Add IBT legacy code bitmap setup function
From: Florian Weimer @ 2019-06-10 17:28 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Dave Hansen, Andy Lutomirski, Peter Zijlstra, x86, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, linux-kernel, linux-doc, linux-mm,
linux-arch, linux-api, Arnd Bergmann, Balbir Singh,
Borislav Petkov, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, H.J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook, Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <5dc357f5858f8036cad5847cfe214401bb9138bf.camel@intel.com>
* Yu-cheng Yu:
> On Fri, 2019-06-07 at 14:09 -0700, Dave Hansen wrote:
>> On 6/7/19 1:06 PM, Yu-cheng Yu wrote:
>> > > Huh, how does glibc know about all possible past and future legacy code
>> > > in the application?
>> >
>> > When dlopen() gets a legacy binary and the policy allows that, it will
>> > manage
>> > the bitmap:
>> >
>> > If a bitmap has not been created, create one.
>> > Set bits for the legacy code being loaded.
>>
>> I was thinking about code that doesn't go through GLIBC like JITs.
>
> If JIT manages the bitmap, it knows where it is.
> It can always read the bitmap again, right?
The problem are JIT libraries without assembler code which can be marked
non-CET, such as liborc. Our builds (e.g., orc-0.4.29-2.fc30.x86_64)
currently carries the IBT and SHSTK flag, although the entry points into
the generated code do not start with ENDBR, so that a jump to them will
fault with the CET enabled.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v3 13/33] docs: infiniband: convert docs to ReST and rename to *.rst
From: Jason Gunthorpe @ 2019-06-10 17:27 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Doug Ledford, linux-rdma
In-Reply-To: <09036fdb89c4bec94cb92d25398c026afdb134e7.1560045490.git.mchehab+samsung@kernel.org>
On Sat, Jun 08, 2019 at 11:27:03PM -0300, Mauro Carvalho Chehab wrote:
> The InfiniBand docs are plain text with no markups.
> So, all we needed to do were to add the title markups and
> some markup sequences in order to properly parse tables,
> lists and literal blocks.
>
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> .../{core_locking.txt => core_locking.rst} | 64 ++++++-----
> Documentation/infiniband/index.rst | 23 ++++
> .../infiniband/{ipoib.txt => ipoib.rst} | 24 ++--
> .../infiniband/{opa_vnic.txt => opa_vnic.rst} | 108 +++++++++---------
> .../infiniband/{sysfs.txt => sysfs.rst} | 4 +-
> .../{tag_matching.txt => tag_matching.rst} | 5 +
> .../infiniband/{user_mad.txt => user_mad.rst} | 33 ++++--
> .../{user_verbs.txt => user_verbs.rst} | 12 +-
> drivers/infiniband/core/user_mad.c | 2 +-
> drivers/infiniband/ulp/ipoib/Kconfig | 2 +-
> 10 files changed, 174 insertions(+), 103 deletions(-)
> rename Documentation/infiniband/{core_locking.txt => core_locking.rst} (78%)
> create mode 100644 Documentation/infiniband/index.rst
> rename Documentation/infiniband/{ipoib.txt => ipoib.rst} (90%)
> rename Documentation/infiniband/{opa_vnic.txt => opa_vnic.rst} (63%)
> rename Documentation/infiniband/{sysfs.txt => sysfs.rst} (69%)
> rename Documentation/infiniband/{tag_matching.txt => tag_matching.rst} (98%)
> rename Documentation/infiniband/{user_mad.txt => user_mad.rst} (90%)
> rename Documentation/infiniband/{user_verbs.txt => user_verbs.rst} (93%)
Looks OK to me, do you want to run these patches through the docs tree
or through RDMA?
Given that we've generally pushed doc updates through rdma, I think
I'd prefer the latter? Jonathan?
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Florian Weimer @ 2019-06-10 17:24 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Dave Martin, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue
In-Reply-To: <94b9c55b3b874825fda485af40ab2a6bc3dad171.camel@intel.com>
* Yu-cheng Yu:
> To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> logical choice. And it breaks only a limited set of toolchains.
>
> I will simplify the parser and leave this patch as-is for anyone who wants to
> back-port. Are there any objections or concerns?
Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
the largest collection of CET-enabled binaries that exists today.
My hope was that we would backport the upstream kernel patches for CET,
port the glibc dynamic loader to the new kernel interface, and be ready
to run with CET enabled in principle (except that porting userspace
libraries such as OpenSSL has not really started upstream, so many
processes where CET is particularly desirable will still run without
it).
I'm not sure if it is a good idea to port the legacy support if it's not
part of the mainline kernel because it comes awfully close to creating
our own private ABI.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Dave Martin @ 2019-06-10 16:57 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue
In-Reply-To: <94b9c55b3b874825fda485af40ab2a6bc3dad171.camel@intel.com>
On Mon, Jun 10, 2019 at 09:29:04AM -0700, Yu-cheng Yu wrote:
> On Fri, 2019-06-07 at 19:01 +0100, Dave Martin wrote:
> > On Thu, Jun 06, 2019 at 01:06:41PM -0700, Yu-cheng Yu wrote:
> > > An ELF file's .note.gnu.property indicates features the executable file
> > > can support. For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> > >
> > > With this patch, if an arch needs to setup features from ELF properties,
> > > it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
> > > arch_setup_property().
> > >
> > > For example, for X86_64:
> > >
> > > int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> > > {
> > > int r;
> > > uint32_t property;
> > >
> > > r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> > > &property);
> > > ...
> > > }
> >
> > Although this code works for the simple case, I have some concerns about
> > some aspects of the implementation here. There appear to be some bounds
> > checking / buffer overrun issues, and the code seems quite complex.
> >
> > Maybe this patch tries too hard to be compatible with toolchains that do
> > silly things such as embedding huge notes in an executable, or mixing
> > NT_GNU_PROPERTY_TYPE_0 in a single PT_NOTE with a load of junk not
> > relevant to the loader. I wonder whether Linux can dictate what
> > interpretation(s) of the ELF specs it is prepared to support, rather than
> > trying to support absolutely anything.
>
> To me, looking at PT_GNU_PROPERTY and not trying to support anything is a
> logical choice. And it breaks only a limited set of toolchains.
>
> I will simplify the parser and leave this patch as-is for anyone who wants to
> back-port. Are there any objections or concerns?
No objection from me ;) But I'm biased.
Hopefully this change should allow substantial simplification. For one
thing, PT_GNU_PROPERTY tells its file offset and size directly in its
phdrs entry. That should save us a lot of effort on the kernel side.
Cheers
---Dave
^ 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