* [PATCH v6 11/12] ima: Implement support for module-style appended signatures
From: Thiago Jung Bauermann @ 2018-03-16 20:38 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-kernel, Mimi Zohar, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, AKASHI, Takahiro,
Thiago Jung Bauermann
In-Reply-To: <20180316203837.10174-1-bauerman@linux.vnet.ibm.com>
This patch actually implements the appraise_type=imasig|modsig option,
allowing IMA to read and verify modsig signatures.
In case both are present in the same file, IMA will first check whether the
key used by the xattr signature is present in the kernel keyring. If not,
it will try the appended signature.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
security/integrity/ima/ima.h | 11 +++++++-
security/integrity/ima/ima_appraise.c | 53 +++++++++++++++++++++++++++++++----
security/integrity/ima/ima_main.c | 21 +++++++++++---
3 files changed, 74 insertions(+), 11 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 49aef56dc96d..c11ccb7c5bfb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -157,7 +157,8 @@ void ima_init_template_list(void);
static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
{
- return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+ return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+ xattr_value->type == IMA_MODSIG);
}
/*
@@ -253,6 +254,8 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
enum ima_hooks func);
enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
int xattr_len);
+bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
+ int xattr_len);
int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value);
@@ -291,6 +294,12 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
return ima_hash_algo;
}
+static inline bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data
+ *xattr_value, int xattr_len)
+{
+ return false;
+}
+
static inline int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value)
{
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 01172eab297b..84e0fd5a19c8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -189,6 +189,22 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
return ima_hash_algo;
}
+bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
+ int xattr_len)
+{
+ struct key *keyring;
+
+ if (xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ return false;
+
+ keyring = integrity_keyring_from_id(INTEGRITY_KEYRING_IMA);
+ if (IS_ERR(keyring))
+ return false;
+
+ return asymmetric_sig_has_known_key(keyring, (const char *) xattr_value,
+ xattr_len);
+}
+
int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value)
{
@@ -221,8 +237,12 @@ 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, hash_start = 0;
+ size_t xattr_contents_len;
+ void *xattr_contents;
- if (!(inode->i_opflags & IOP_XATTR))
+ /* If not appraising a modsig, we need an xattr. */
+ if ((xattr_value == NULL || xattr_value->type != IMA_MODSIG) &&
+ !(inode->i_opflags & IOP_XATTR))
return INTEGRITY_UNKNOWN;
if (rc <= 0) {
@@ -241,13 +261,29 @@ int ima_appraise_measurement(enum ima_hooks func,
goto out;
}
- status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+ /*
+ * If it's a modsig, we don't have the xattr contents to pass to
+ * evm_verifyxattr().
+ */
+ if (xattr_value->type == IMA_MODSIG) {
+ xattr_contents = NULL;
+ xattr_contents_len = 0;
+ } else {
+ xattr_contents = xattr_value;
+ xattr_contents_len = xattr_len;
+ }
+
+ status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_contents,
+ xattr_contents_len, iint);
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_PASS_IMMUTABLE:
case INTEGRITY_UNKNOWN:
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
+ /* It's fine not to have xattrs when using a modsig. */
+ if (xattr_value->type == IMA_MODSIG)
+ break;
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
cause = "missing-HMAC";
goto out;
@@ -288,11 +324,16 @@ int ima_appraise_measurement(enum ima_hooks func,
status = INTEGRITY_PASS;
break;
case EVM_IMA_XATTR_DIGSIG:
+ case IMA_MODSIG:
set_bit(IMA_DIGSIG, &iint->atomic_flags);
- rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
- (const char *)xattr_value, rc,
- iint->ima_hash->digest,
- iint->ima_hash->length);
+ if (xattr_value->type == EVM_IMA_XATTR_DIGSIG)
+ rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+ (const char *)xattr_value,
+ rc, iint->ima_hash->digest,
+ iint->ima_hash->length);
+ else
+ rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA,
+ xattr_value);
if (rc == -EOPNOTSUPP) {
status = INTEGRITY_UNKNOWN;
} else if (rc) {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5d122daf5c8a..1b11c10f09df 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -183,7 +183,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
struct evm_ima_xattr_data *xattr_value = NULL;
int xattr_len = 0;
bool violation_check;
- enum hash_algo hash_algo;
+ enum hash_algo hash_algo = HASH_ALGO__LAST;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -277,11 +277,24 @@ 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);
+ if (iint->flags & IMA_MODSIG_ALLOWED &&
+ (xattr_len <= 0 || !ima_xattr_sig_known_key(xattr_value,
+ xattr_len))) {
+ /*
+ * Even if we end up using a modsig, hash_algo should
+ * come from the xattr (or even the default hash algo).
+ */
+ hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
+ ima_read_modsig(func, buf, size, &xattr_value,
+ &xattr_len);
+ }
+ }
- hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
+ if (hash_algo == HASH_ALGO__LAST)
+ hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
if (rc != 0 && rc != -EBADF && rc != -EINVAL)
@@ -309,7 +322,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
!(iint->flags & IMA_NEW_FILE))
rc = -EACCES;
mutex_unlock(&iint->mutex);
- kfree(xattr_value);
+ ima_free_xattr_data(xattr_value);
out:
if (pathbuf)
__putname(pathbuf);
^ permalink raw reply related
* [PATCH v6 12/12] ima: Write modsig to the measurement list
From: Thiago Jung Bauermann @ 2018-03-16 20:38 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-kernel, Mimi Zohar, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, AKASHI, Takahiro,
Thiago Jung Bauermann
In-Reply-To: <20180316203837.10174-1-bauerman@linux.vnet.ibm.com>
Define new "d-sig" template field which holds the digest that is expected
to match the one contained in the modsig.
Also add modsig support to the "sig" template field, allowing the the
contents of the modsig to be included in the measurement list.
Suggested-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
Documentation/security/IMA-templates.rst | 5 ++++
security/integrity/ima/ima_template.c | 4 ++-
security/integrity/ima/ima_template_lib.c | 47 +++++++++++++++++++++++++++++--
security/integrity/ima/ima_template_lib.h | 2 ++
4 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..f2a0f4225857 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -68,6 +68,11 @@ 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-sig': the digest of the event for files that have an appended modsig. This
+ field is calculated without including the modsig and thus will differ from
+ the total digest of the file, but it is what should match the digest
+ contained in the modsig (if it doesn't, the signature is invalid). It is
+ shown in the same format as 'd-ng';
- 'n-ng': the name of the event, without size limitations;
- 'sig': the file signature.
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 30db39b23804..36fc32f538b5 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -43,8 +43,10 @@ static 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-sig", .field_init = ima_eventdigest_sig_init,
+ .field_show = ima_show_template_digest_ng},
};
-#define MAX_TEMPLATE_NAME_LEN 15
+#define MAX_TEMPLATE_NAME_LEN 24
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 afb52a90e532..1dca082cce43 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -220,7 +220,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)
{
/*
@@ -323,6 +324,35 @@ 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_sig_init(struct ima_event_data *event_data,
+ struct ima_field_data *field_data)
+{
+ struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
+ enum hash_algo hash_algo = HASH_ALGO_SHA1;
+ const u8 *cur_digest = NULL;
+ u8 cur_digestsize = 0;
+ int ret;
+
+ if (!xattr_value || xattr_value->type != IMA_MODSIG)
+ return 0;
+
+ if (event_data->violation) /* recording a violation. */
+ goto out;
+
+ ret = ima_get_modsig_hash(xattr_value, &hash_algo, &cur_digest,
+ &cur_digestsize);
+ if (ret)
+ return ret;
+
+ out:
+ 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)
@@ -379,10 +409,23 @@ int ima_eventsig_init(struct ima_event_data *event_data,
struct ima_field_data *field_data)
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
+ int xattr_len = event_data->xattr_len;
if (!is_ima_sig(xattr_value))
return 0;
- return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+ /*
+ * The xattr_value for IMA_MODSIG is a runtime structure containing
+ * pointers. Get its raw data instead.
+ */
+ if (xattr_value->type == IMA_MODSIG) {
+ int rc;
+
+ rc = ima_modsig_serialize_data(&xattr_value, &xattr_len);
+ if (rc)
+ return rc;
+ }
+
+ return ima_write_template_field_data(xattr_value, xattr_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..3cd353e83f73 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -38,6 +38,8 @@ 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_sig_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,
^ permalink raw reply related
* [PATCH v6 02/12] PKCS#7: Introduce pkcs7_get_message_sig() and verify_pkcs7_message_sig()
From: Thiago Jung Bauermann @ 2018-03-16 20:38 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-kernel, Mimi Zohar, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, AKASHI, Takahiro,
Thiago Jung Bauermann
In-Reply-To: <20180316203837.10174-1-bauerman@linux.vnet.ibm.com>
IMA will need to know the key that signed a given PKCS#7 message, so add
pkcs7_get_message_sig().
It will also need to verify an already parsed PKCS#7 message. For this
purpose, add verify_pkcs7_message_sig() which takes a struct pkcs7_message
for verification instead of the raw bytes that verify_pkcs7_signature()
takes.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.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 ++++++++++++++++++++++++++---------
crypto/asymmetric_keys/pkcs7_parser.c | 16 +++++++++
include/crypto/pkcs7.h | 2 ++
include/linux/verification.h | 10 ++++++
4 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b27f0c..7ddc8b7a3062 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -190,33 +190,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");
@@ -258,6 +252,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/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index a6dcaa659aa8..456b803972b5 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -683,3 +683,19 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
return -ENOMEM;
return 0;
}
+
+/**
+ * pkcs7_get_message_sig - get signature in @pkcs7
+ */
+const struct public_key_signature *pkcs7_get_message_sig(
+ const struct pkcs7_message *pkcs7)
+{
+ /*
+ * This function doesn't support messages with more than one signature,
+ * so don't return anything in that case.
+ */
+ if (pkcs7->signed_infos == NULL || pkcs7->signed_infos->next != NULL)
+ return NULL;
+
+ return pkcs7->signed_infos->sig;
+}
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..6f51d0cb6d12 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -28,6 +28,8 @@ extern void pkcs7_free_message(struct pkcs7_message *pkcs7);
extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
const void **_data, size_t *_datalen,
size_t *_headerlen);
+extern const struct public_key_signature *pkcs7_get_message_sig(
+ const struct pkcs7_message *pkcs7);
/*
* pkcs7_trust.c
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6c7cd..f04dac2728ec 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -29,6 +29,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,
@@ -38,6 +39,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
* Re: dtc warnings
From: Stephen Rothwell @ 2018-03-16 21:25 UTC (permalink / raw)
To: Michael Ellerman; +Cc: ppc-dev
In-Reply-To: <87r2okglbj.fsf@concordia.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]
Hi Michael,
On Sat, 17 Mar 2018 01:13:36 +1100 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
>
> > I get the following from a powerpc_ppc44x defconfig build in current
> > linux-next:
> >
> > arch/powerpc/boot/ebony.dtb: Warning (pci_bridge): /plb/pci@20ec00000: missing bus-range for PCI bridge
> > arch/powerpc/boot/ebony.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/ebony.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/sequoia.dtb: Warning (pci_bridge): /plb/pci@1ec000000: missing bus-range for PCI bridge
> > arch/powerpc/boot/sequoia.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/sequoia.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/sam440ep.dtb: Warning (pci_bridge): /plb/pci@ec000000: missing bus-range for PCI bridge
> > arch/powerpc/boot/sam440ep.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/sam440ep.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/rainier.dtb: Warning (pci_bridge): /plb/pci@1ec000000: missing bus-range for PCI bridge
> > arch/powerpc/boot/rainier.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/rainier.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/taishan.dtb: Warning (pci_bridge): /plb/pci@20ec00000: missing bus-range for PCI bridge
> > arch/powerpc/boot/taishan.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/taishan.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/bamboo.dtb: Warning (pci_bridge): /plb/pci@ec000000: missing bus-range for PCI bridge
> > arch/powerpc/boot/bamboo.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/bamboo.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/katmai.dtb: Warning (pci_bridge): /plb/pciex@d00000000: node name is not "pci" or "pcie"
> > arch/powerpc/boot/katmai.dtb: Warning (pci_bridge): /plb/pciex@d20000000: node name is not "pci" or "pcie"
> > arch/powerpc/boot/katmai.dtb: Warning (pci_bridge): /plb/pciex@d40000000: node name is not "pci" or "pcie"
> > arch/powerpc/boot/katmai.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/katmai.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/warp.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> > arch/powerpc/boot/yosemite.dtb: Warning (pci_bridge): /plb/pci@ec000000: missing bus-range for PCI bridge
> > arch/powerpc/boot/yosemite.dtb: Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge'
> > arch/powerpc/boot/yosemite.dtb: Warning (chosen_node_stdout_path): /chosen:linux,stdout-path: Use 'stdout-path' instead
> >
> > I though someone might like to do something about them ...
>
> Feel free!
>
> Oh that's not what you meant :P
:-)
> Rob sent a patch to fix most of them:
> http://patchwork.ozlabs.org/patch/879475/
>
> I thought he was going to merge it, but actually I'm not sure why I
> thought that. So I should probably merge it.
excellent, thanks
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v6 00/12] Appended signatures support for IMA appraisal
From: Thiago Jung Bauermann @ 2018-03-16 21:38 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-kernel, Mimi Zohar, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, AKASHI, Takahiro
In-Reply-To: <20180316203837.10174-1-bauerman@linux.vnet.ibm.com>
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> Now the modsig is only ignored if it references a signature that is not
> present in IMA's keyring (or if there's a parsing error, obviously). If the
The above should read "Now the modsig is only ignored if it references a
*key* that is not present in IMA's keyring".
Sorry, I only noticed the mistake after sending the emails.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* [PATCH] powerpc/boot: Remove duplicate typedefs from libfdt_env.h
From: Mark Greer @ 2018-03-16 21:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, Mark Greer, David Gibson, Oliver O'Halloran
When building a uImage or zImage using ppc6xx_defconfig and some other
defconfigs, the following error occurs:
BOOTCC arch/powerpc/boot/fdt.o
In file included from arch/powerpc/boot/fdt.c:51:0:
../arch/powerpc/boot/libfdt_env.h:10:13: error: redefinition of typedef 'uint32_t'
../arch/powerpc/boot/types.h:21:13: note: previous declaration of 'uint32_t' was here
../arch/powerpc/boot/libfdt_env.h:11:13: error: redefinition of typedef 'uint64_t'
../arch/powerpc/boot/types.h:22:13: note: previous declaration of 'uint64_t' was here
../arch/powerpc/boot/Makefile:210: recipe for target 'arch/powerpc/boot/fdt.o' failed
make[2]: *** [arch/powerpc/boot/fdt.o] Error 1
The problem is that commit 656ad58ef19e (powerpc/boot: Add OPAL console
to epapr wrappers) adds typedefs for uint32_t and uint64_t to type.h but
doesn't remove the pre-existing (and now duplicate) typedefs from
libfdt_env.h. Fix the error by removing the duplicat typedefs from
libfdt_env.h
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Mark Greer <mgreer@animalcreek.com>
---
Having said all of that, commit 656ad58ef19e (powerpc/boot: Add OPAL
console to epapr wrappers) went into mainline back in 2016 so, AFAICT,
this has been broken since then. That seems unlikely so I must be
missing something... Any ideas what that is?
I built all of the defconfigs that I had toolchains handy for (really
just old-fashioned ppc32, non-booke) so I didn't test everything.
Compile tested only as I no longer have any relevant hardware.
Based on git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux fixes
currently at e4b79900222b (powerpc/64s: Fix NULL AT_BASE_PLATFORM when
using DT CPU features).
arch/powerpc/boot/libfdt_env.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/boot/libfdt_env.h b/arch/powerpc/boot/libfdt_env.h
index f52c31b1f48f..2a0c8b1bf147 100644
--- a/arch/powerpc/boot/libfdt_env.h
+++ b/arch/powerpc/boot/libfdt_env.h
@@ -7,8 +7,6 @@
#include "of.h"
-typedef u32 uint32_t;
-typedef u64 uint64_t;
typedef unsigned long uintptr_t;
typedef __be16 fdt16_t;
--
2.16.1
^ permalink raw reply related
* Re: [PATCH v12 02/22] selftests/vm: rename all references to pkru to a generic name
From: Dave Hansen @ 2018-03-16 21:55 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-3-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> int pkey_set(int pkey, unsigned long rights, unsigned long flags)
> {
> u32 mask = (PKEY_DISABLE_ACCESS|PKEY_DISABLE_WRITE);
> - u32 old_pkru = __rdpkru();
> - u32 new_pkru;
> + u32 old_pkey_reg = __rdpkey_reg();
> + u32 new_pkey_reg;
If we're not using the _actual_ instruction names ("rdpkru"), I think
I'd rather this be something more readable, like: __read_pkey_reg().
But, it's OK-ish the way it is.
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v12 04/22] selftests/vm: typecast the pkey register
From: Dave Hansen @ 2018-03-16 21:58 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-5-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> -static inline unsigned int _rdpkey_reg(int line)
> +static inline pkey_reg_t _rdpkey_reg(int line)
> {
> - unsigned int pkey_reg = __rdpkey_reg();
> + pkey_reg_t pkey_reg = __rdpkey_reg();
>
> - dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
> + dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
> line, pkey_reg, shadow_pkey_reg);
> assert(pkey_reg == shadow_pkey_reg);
Hmm. So we're using %lx for an int? Doesn't the compiler complain
about this?
^ permalink raw reply
* Re: [PATCH v12 05/22] selftests/vm: generic function to handle shadow key register
From: Dave Hansen @ 2018-03-16 22:05 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-6-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> +static inline u32 pkey_to_shift(int pkey)
> +{
> + return pkey * PKEY_BITS_PER_PKEY;
> +}
pkey_bit_position(), perhaps?
> +static inline pkey_reg_t reset_bits(int pkey, pkey_reg_t bits)
> +{
> + u32 shift = pkey_to_shift(pkey);
> +
> + return ~(bits << shift);
> +}
I'd prefer clear_pkey_flags() or maybe clear_pkey_bits(). "reset" can
mean "reset to 0" or "reset to 1".
Also, why the u32 here? Isn't an int more appropriate?
> +static inline pkey_reg_t left_shift_bits(int pkey, pkey_reg_t bits)
> +{
> + u32 shift = pkey_to_shift(pkey);
> +
> + return (bits << shift);
> +}
> +
> +static inline pkey_reg_t right_shift_bits(int pkey, pkey_reg_t bits)
> +{
> + u32 shift = pkey_to_shift(pkey);
> +
> + return (bits >> shift);
> +}
Some comments on these would be handy. Basically that this takes a
per-key flags value and puts it at the right position so it can be
shoved in the register.
^ permalink raw reply
* Re: [PATCH v12 06/22] selftests/vm: fix the wrong assert in pkey_disable_set()
From: Dave Hansen @ 2018-03-16 22:06 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-7-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> If the flag is 0, no bits will be set. Hence we cant expect
> the resulting bitmap to have a higher value than what it
> was earlier.
>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> tools/testing/selftests/vm/protection_keys.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index 83216c5..0109388 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -443,7 +443,7 @@ void pkey_disable_set(int pkey, int flags)
> dprintf1("%s(%d) pkey_reg: 0x%lx\n",
> __func__, pkey, rdpkey_reg());
> if (flags)
> - pkey_assert(rdpkey_reg() > orig_pkey_reg);
> + pkey_assert(rdpkey_reg() >= orig_pkey_reg);
> dprintf1("END<---%s(%d, 0x%x)\n", __func__,
> pkey, flags);
> }
I'm not sure about this one. Did this cause a problem for you?
Why would you call this and ask no bits to be set?
^ permalink raw reply
* Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
From: Dave Hansen @ 2018-03-16 22:08 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-8-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
> pkey, pkey, pkey_rights);
> pkey_assert(pkey_rights >= 0);
>
> - pkey_rights |= flags;
> + pkey_rights &= ~flags;
>
> ret = pkey_set(pkey, pkey_rights, 0);
> /* pkey_reg and flags have the same format */
> @@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
> dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
> pkey, rdpkey_reg());
> if (flags)
> - assert(rdpkey_reg() > orig_pkey_reg);
> + assert(rdpkey_reg() < orig_pkey_reg);
> }
>
> void pkey_write_allow(int pkey)
This seems so horribly wrong that I wonder how it worked in the first
place. Any idea?
^ permalink raw reply
* Re: [PATCH v12 08/22] selftests/vm: clear the bits in shadow reg when a pkey is freed.
From: Dave Hansen @ 2018-03-16 22:10 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-9-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> When a key is freed, the key is no more effective.
> Clear the bits corresponding to the pkey in the shadow
> register. Otherwise it will carry some spurious bits
> which can trigger false-positive asserts.
...
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index ca54a95..aaf9f09 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -582,6 +582,9 @@ int alloc_pkey(void)
> int sys_pkey_free(unsigned long pkey)
> {
> int ret = syscall(SYS_pkey_free, pkey);
> +
> + if (!ret)
> + shadow_pkey_reg &= reset_bits(pkey, PKEY_DISABLE_ACCESS);
> dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
> return ret;
> }
Did this cause problems for you in practice?
On x86, sys_pkey_free() does not affect PKRU, so this isn't quite right.
I'd much rather have the actual tests explicitly clear the PKRU bits
and also in the process clear the shadow bits.
^ permalink raw reply
* Re: [PATCH v12 09/22] selftests/vm: fix alloc_random_pkey() to make it really random
From: Dave Hansen @ 2018-03-16 22:13 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-10-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> alloc_random_pkey() was allocating the same pkey every time.
> Not all pkeys were geting tested. fixed it.
...
> @@ -602,13 +603,15 @@ int alloc_random_pkey(void)
> int alloced_pkeys[NR_PKEYS];
> int nr_alloced = 0;
> int random_index;
> +
> memset(alloced_pkeys, 0, sizeof(alloced_pkeys));
> + srand((unsigned int)time(NULL));
>
> /* allocate every possible key and make a note of which ones we got */
> max_nr_pkey_allocs = NR_PKEYS;
> - max_nr_pkey_allocs = 1;
> for (i = 0; i < max_nr_pkey_allocs; i++) {
> int new_pkey = alloc_pkey();
The srand() is probably useful, but won't this always just do a single
alloc_pkey() now? That seems like it will mean we always use the first
one the kernel gives us, which isn't random.
> - dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%x\n", __func__,
> - __LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
> + dprintf1("%s()::%d, ret: %d pkey_reg: 0x%x shadow: 0x%016lx\n",
> + __func__, __LINE__, ret, __rdpkey_reg(), shadow_pkey_reg);
> return ret;
> }
This belonged in the pkey_reg_t patch, I think.
^ permalink raw reply
* Re: [PATCH v12 10/22] selftests/vm: introduce two arch independent abstraction
From: Dave Hansen @ 2018-03-16 22:15 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-11-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> open_hugepage_file() <- opens the huge page file
> get_start_key() <-- provides the first non-reserved key.
>
Looks reasonable.
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v12 11/22] selftests/vm: pkey register should match shadow pkey
From: Dave Hansen @ 2018-03-16 22:19 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-12-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> expected_pkey_fault() is comparing the contents of pkey
> register with 0. This may not be true all the time. There
> could be bits set by default by the architecture
> which can never be changed. Hence compare the value against
> shadow pkey register, which is supposed to track the bits
> accurately all throughout
>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> tools/testing/selftests/vm/protection_keys.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index 254b66d..6054093 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -926,10 +926,10 @@ void expected_pkey_fault(int pkey)
> pkey_assert(last_pkey_faults + 1 == pkey_faults);
> pkey_assert(last_si_pkey == pkey);
> /*
> - * The signal handler shold have cleared out PKEY register to let the
> + * The signal handler shold have cleared out pkey-register to let the
Heh, you randomly changed the formatting and didn't bother with my awful
typo. :)
> * test program continue. We now have to restore it.
> */
> - if (__rdpkey_reg() != 0)
> + if (__rdpkey_reg() != shadow_pkey_reg)
> pkey_assert(0);
>
> __wrpkey_reg(shadow_pkey_reg);
>
I don't think this should be "shadow_pkey_reg". This was just trying to
double-check that the signal handler messed around with PKRU the way we
expected.
We could also just check that the disable bits for 'pkey' are clear at
this point. That would be almost as good.
^ permalink raw reply
* Re: [PATCH v12 12/22] selftests/vm: generic cleanup
From: Dave Hansen @ 2018-03-16 22:22 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-13-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> cleanup the code to satisfy coding styles.
>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> tools/testing/selftests/vm/protection_keys.c | 81 ++++++++++++++------------
> 1 files changed, 43 insertions(+), 38 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index 6054093..6fdd8f5 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -4,7 +4,7 @@
> *
> * There are examples in here of:
> * * how to set protection keys on memory
> - * * how to set/clear bits in pkey registers (the rights register)
> + * * how to set/clear bits in Protection Key registers (the rights register)
I don't think CodingStyle says to do this. :)
> * * how to handle SEGV_PKUERR signals and extract pkey-relevant
> * information from the siginfo
> *
> @@ -13,13 +13,18 @@
> * prefault pages in at malloc, or not
> * protect MPX bounds tables with protection keys?
> * make sure VMA splitting/merging is working correctly
> - * OOMs can destroy mm->mmap (see exit_mmap()), so make sure it is immune to pkeys
> - * look for pkey "leaks" where it is still set on a VMA but "freed" back to the kernel
> - * do a plain mprotect() to a mprotect_pkey() area and make sure the pkey sticks
> + * OOMs can destroy mm->mmap (see exit_mmap()),
> + * so make sure it is immune to pkeys
> + * look for pkey "leaks" where it is still set on a VMA
> + * but "freed" back to the kernel
> + * do a plain mprotect() to a mprotect_pkey() area and make
> + * sure the pkey sticks
Ram, I'm not sure where this came from, but this looks horrid. Please
don't do this to the file
> * Compile like this:
> - * gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm
> - * gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm
> + * gcc -o protection_keys -O2 -g -std=gnu99
> + * -pthread -Wall protection_keys.c -lrt -ldl -lm
> + * gcc -m32 -o protection_keys_32 -O2 -g -std=gnu99
> + * -pthread -Wall protection_keys.c -lrt -ldl -lm
> */
Please just leave this, or remove it from the file. It was a long line
so it could be copied and pasted, this ruins that.
> #define _GNU_SOURCE
> #include <errno.h>
> @@ -251,26 +256,11 @@ void signal_handler(int signum, siginfo_t *si, void *vucontext)
> dprintf1("signal pkey_reg from pkey_reg: %016lx\n", __rdpkey_reg());
> dprintf1("pkey from siginfo: %jx\n", siginfo_pkey);
> *(u64 *)pkey_reg_ptr = 0x00000000;
> - dprintf1("WARNING: set PRKU=0 to allow faulting instruction to continue\n");
> + dprintf1("WARNING: set PKEY_REG=0 to allow faulting instruction "
> + "to continue\n");
> pkey_faults++;
> dprintf1("<<<<==================================================\n");
> return;
> - if (trapno == 14) {
> - fprintf(stderr,
> - "ERROR: In signal handler, page fault, trapno = %d, ip = %016lx\n",
> - trapno, ip);
> - fprintf(stderr, "si_addr %p\n", si->si_addr);
> - fprintf(stderr, "REG_ERR: %lx\n",
> - (unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);
> - exit(1);
> - } else {
> - fprintf(stderr, "unexpected trap %d! at 0x%lx\n", trapno, ip);
> - fprintf(stderr, "si_addr %p\n", si->si_addr);
> - fprintf(stderr, "REG_ERR: %lx\n",
> - (unsigned long)uctxt->uc_mcontext.gregs[REG_ERR]);
> - exit(2);
> - }
> - dprint_in_signal = 0;
> }
I think this is just randomly removing code now.
I think you should probably just drop this patch. It's not really
brining anything useful.
^ permalink raw reply
* Re: [PATCH v12 13/22] selftests/vm: powerpc implementation for generic abstraction
From: Dave Hansen @ 2018-03-16 22:23 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-14-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> static inline u32 pkey_to_shift(int pkey)
> {
> +#if defined(__i386__) || defined(__x86_64__) /* arch */
> return pkey * PKEY_BITS_PER_PKEY;
> +#elif __powerpc64__ /* arch */
> + return (NR_PKEYS - pkey - 1) * PKEY_BITS_PER_PKEY;
> +#endif /* arch */
> }
I really detest the #if #else style. Can't we just have a pkey_ppc.h
and a pkey_x86.h or something?
^ permalink raw reply
* Re: [PATCH v12 14/22] selftests/vm: clear the bits in shadow reg when a pkey is freed.
From: Dave Hansen @ 2018-03-16 22:24 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-15-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
> index c4c73e6..e82bd88 100644
> --- a/tools/testing/selftests/vm/protection_keys.c
> +++ b/tools/testing/selftests/vm/protection_keys.c
> @@ -586,7 +586,8 @@ int sys_pkey_free(unsigned long pkey)
> int ret = syscall(SYS_pkey_free, pkey);
>
> if (!ret)
> - shadow_pkey_reg &= reset_bits(pkey, PKEY_DISABLE_ACCESS);
> + shadow_pkey_reg &= reset_bits(pkey,
> + PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE);
> dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret);
> return ret;
> }
What about your EXEC bit?
^ permalink raw reply
* Re: [PATCH v12 15/22] selftests/vm: powerpc implementation to check support for pkey
From: Dave Hansen @ 2018-03-16 22:26 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-16-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> #define PAGE_SIZE (0x1UL << 16)
> -static inline int cpu_has_pku(void)
> +static inline bool is_pkey_supported(void)
> {
> - return 1;
> + /*
> + * No simple way to determine this.
> + * lets try allocating a key and see if it succeeds.
> + */
> + int ret = sys_pkey_alloc(0, 0);
> +
> + if (ret > 0) {
> + sys_pkey_free(ret);
> + return true;
> + }
> + return false;
> }
The point of doing this was to have a test for the CPU that way separate
from the syscalls.
Can you leave cpu_has_pkeys() in place?
^ permalink raw reply
* Re: [PATCH v12 16/22] selftests/vm: fix an assertion in test_pkey_alloc_exhaust()
From: Dave Hansen @ 2018-03-16 22:28 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-17-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> +static inline int arch_reserved_keys(void)
> +{
> +#if defined(__i386__) || defined(__x86_64__) /* arch */
> + return NR_RESERVED_PKEYS;
> +#elif __powerpc64__ /* arch */
> + if (sysconf(_SC_PAGESIZE) == 4096)
> + return NR_RESERVED_PKEYS_4K;
> + else
> + return NR_RESERVED_PKEYS_64K;
> +#else /* arch */
> + NOT SUPPORTED
> +#endif /* arch */
> +}
Yeah, this is hideous.
Please either do it in one header:
#ifdef x86..
static inline int arch_reserved_keys(void)
{
}
...
#elif ppc
static inline int arch_reserved_keys(void)
{
}
...
#else
#error
#endif
Or in multiple:
#ifdef x86..
#include <pkey_x86.h>
#elif ppc
#include <pkey_ppc.h>
#else
#error
#endif
^ permalink raw reply
* Re: [PATCH v12 17/22] selftests/vm: associate key on a mapped page and detect access violation
From: Dave Hansen @ 2018-03-16 22:30 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-18-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> detect access-violation on a page to which access-disabled
> key is associated much after the page is mapped.
Looks fine to me. Did this actually find a bug for you?
Acked-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v12 18/22] selftests/vm: associate key on a mapped page and detect write violation
From: Dave Hansen @ 2018-03-16 22:30 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-19-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> detect write-violation on a page to which write-disabled
> key is associated much after the page is mapped.
The more tests the merrier.
Acked-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v12 19/22] selftests/vm: detect write violation on a mapped access-denied-key page
From: Dave Hansen @ 2018-03-16 22:31 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-20-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> detect write-violation on a page to which access-disabled
> key is associated much after the page is mapped.
Acked-by: Dave Hansen <dave.hansen@intel.com>
^ permalink raw reply
* Re: [PATCH v12 20/22] selftests/vm: testcases must restore pkey-permissions
From: Dave Hansen @ 2018-03-16 22:32 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-21-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
> Generally the signal handler restores the state of the pkey register
> before returning. However there are times when the read/write operation
> can legitamely fail without invoking the signal handler. Eg: A
> sys_read() operaton to a write-protected page should be disallowed. In
> such a case the state of the pkey register is not restored to its
> original state. The test case is responsible for restoring the key
> register state to its original value.
Oh, that's a good point.
Could we just do this in a common place, though? Like reset the
register after each test? Seems more foolproof.
^ permalink raw reply
* Re: [PATCH v12 21/22] selftests/vm: sub-page allocator
From: Dave Hansen @ 2018-03-16 22:33 UTC (permalink / raw)
To: Ram Pai, shuahkh, linux-kselftest
Cc: mpe, linuxppc-dev, linux-mm, x86, linux-arch, linux-doc,
linux-kernel, mingo, akpm, benh, paulus, khandual, aneesh.kumar,
bsingharora, hbabu, mhocko, bauerman, ebiederm, arnd
In-Reply-To: <1519264541-7621-22-git-send-email-linuxram@us.ibm.com>
On 02/21/2018 05:55 PM, Ram Pai wrote:
...
> @@ -888,6 +917,7 @@ void setup_hugetlbfs(void)
> void *(*pkey_malloc[])(long size, int prot, u16 pkey) = {
>
> malloc_pkey_with_mprotect,
> + malloc_pkey_with_mprotect_subpage,
> malloc_pkey_anon_huge,
> malloc_pkey_hugetlb
> /* can not do direct with the pkey_mprotect() API:
I think I'd rather have an #ifdef on the array entries than have the
malloc entry do nothing on x86. Maybe have a ppc-specific section at
the end?
^ 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