[PATCH v5 00/18] Appended signatures support for IMA appraisal

[PATCH v5 00/18] Appended signatures support for IMA appraisal

[Thiago Jung Bauermann]

Hello,

The main highlight in this version is that it fixes a bug where the modsig
wasn't being included in the measurement list if the appraised file was
already measured by another rule. The fix is in the last patch.

Another change is that the last patch in the v4 series ("ima: Support
module-style appended signatures for appraisal") has been broken up into
smaller patches. I may have overdone it...

Finally, I have added some patches removing superfluous parentheses from
expressions. IMO these patches make it easier (and more pleasant) to read
the code, and thus easier to understand it. Since I'm not sure how welcome
the changes are, I split them in 3 "levels" in increasing potential for
conflict with patches from other people (they can be squashed together when
applied):

1. patch 2 contains the bare minimum, changing only lines that are also
   touched by other patches in the series;

2. patch 3 cleans up all the files that are touched by this patch series;

3. patch 4 cleans up all other EVM and IMA files that weren't already fixed
   by the previous patches.

If unwanted, patches 3 and 4 can be simply skipped without affecting the
rest of the patches. I have already rebased them from v4.13-rc2 to
v4.14-rc3 and now to linux-integrity/next with very few easy to resolve
conflicts, so I think they are worth keeping.

These patches apply on top of today's linux-integrity/next.

Original cover letter:

On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.

This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.

Since modsig is only supported on some specific hooks which don't get
called often (cf. ima_hook_supports_modsig), it's possible to always check
for the presence of an appended modsig before looking for the xattr sig. In
that case, the policy doesn't need to be changed to support the modsig
keyword. Is that preferable than requiring the policy to explicitly allow a
modsig like this code does?

I tested these patches with EVM and I believe they don't break it and
things work as expected, but I'm not really familiar with EVM and its use
cases so this should be taken with a grain of salt.

I also verified that the code correctly recalculates the file hash if the
modsig verification fails and the file also has an xattr signature which
uses a different hash algorithm.

Changes since v4:
- Patch "ima: Remove redundant conditional operator"
  - New patch.

- Patch "ima: Remove some superfluous parentheses"
  - New patch.

- Patch "evm, ima: Remove superfluous parentheses"
  - New patch.

- Patch "evm, ima: Remove more superfluous parentheses"
  - New patch.

- Patch "ima: Simplify ima_eventsig_init"
  - New patch.

- Patch "ima: Improvements in ima_appraise_measurement"
  - New patch.

- Patch "ima: Don't pass xattr value to EVM xattr verification."
  - New patch.

- Patch "ima: Export func_tokens"
  - Split from patch "ima: Support module-style appended signatures for
    appraisal".

- Patch "ima: Add modsig appraise_type option for module-style appended
         signatures"
  - Split from patch "ima: Support module-style appended signatures for
    appraisal".
  - Mention modsig option in Documentation/ABI/testing/ima_policy
    (suggested by Mimi Zohar).

- Patch "ima: Add functions to read and verify a modsig signature"
  - Split from patch "ima: Support module-style appended signatures for
    appraisal".

- Patch "ima: Implement support for module-style appended signatures"
  - Split from patch "ima: Support module-style appended signatures for
    appraisal".
  - In ima_appraise_measurement, change the logic of dealing with xattr
    errors in case the modsig verification fails. With this,
    process_xattr_error isn't needed anymore.

- Patch "ima: Write modsig to the measurement list"
  - Split from patch "ima: Support module-style appended signatures for
    appraisal".
  - Added ima_current_template_has_sig function.
  - Removed hdr parameter from ima_modsig_serialize_data.
  - In ima_store_measurement, continue processing even if the given PCR
    is already measured if it's for a modsig.
  - In process_measurement, add exception to store measurement even if
    IMA_MEASURE is not set when appraising a modsig (suggested by
    Mimi Zohar).
  - Call is_ima_sig in ima_eventsig_init.

Changes since v3:
- Patch "integrity: Introduce struct evm_hmac_xattr"
  - Renamed new struct to evm_xattr.
  - Define struct evm_xattr using struct evm_ima_xattr_data, and moved it
    from evm.h to integrity.h (suggested by Mimi Zohar).
- Patch "PKCS#7: Introduce verify_pkcs7_message_sig"
  - Also introduce pkcs7_get_message_sig.
- Patch "ima: Support appended signatures for appraisal"
  - Moved check for buffer presence and size from ima_appraise_measurement
    to ima_read_modsig (suggested by Mimi Zohar).
  - Factored out handling of ima_read_xattr return value into
    process_xattr_error in ima_appraise_measurement so that it can be used
    if the modsig verification fails.
  - Pass NULL xattr_value to evm_verifyxattr even in the case of xattr
    signature in ima_appraise_measurement (suggested by Mimi Zohar).
  - Use switch statement provided by Mimi Zohar to check result of
    evm_verifyxattr.
  - If the modsig verification succeeds, copy the hash calculated during
    the verification to the iint cache (suggested by Mimi Zohar).
  - Substitute recursion in ima_appraise_measurement by a goto statement
    back to the main switch statement (suggested by Mimi Zohar).

Thiago Jung Bauermann (18):
  ima: Remove redundant conditional operator
  ima: Remove some superfluous parentheses
  evm, ima: Remove superfluous parentheses
  evm, ima: Remove more superfluous parentheses
  ima: Simplify ima_eventsig_init
  ima: Improvements in ima_appraise_measurement
  integrity: Introduce struct evm_xattr
  integrity: Select CONFIG_KEYS instead of depending on it
  ima: Don't pass xattr value to EVM xattr verification.
  ima: Store measurement after appraisal
  ima: Export func_tokens
  MODSIGN: Export module signature definitions
  PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig
  integrity: Introduce integrity_keyring_from_id
  ima: Add modsig appraise_type option for module-style appended
    signatures
  ima: Add functions to read and verify a modsig signature
  ima: Implement support for module-style appended signatures
  ima: Write modsig to the measurement list

 Documentation/ABI/testing/ima_policy      |   6 +-
 certs/system_keyring.c                    |  60 +++++++---
 crypto/asymmetric_keys/pkcs7_parser.c     |  12 ++
 include/crypto/pkcs7.h                    |   2 +
 include/linux/module.h                    |   3 -
 include/linux/module_signature.h          |  47 ++++++++
 include/linux/verification.h              |  10 ++
 init/Kconfig                              |   6 +-
 kernel/Makefile                           |   2 +-
 kernel/module.c                           |   1 +
 kernel/module_signing.c                   |  74 ++++++-------
 security/integrity/Kconfig                |   2 +-
 security/integrity/digsig.c               |  28 +++--
 security/integrity/evm/evm_crypto.c       |   6 +-
 security/integrity/evm/evm_main.c         |  23 ++--
 security/integrity/evm/evm_posix_acl.c    |   8 +-
 security/integrity/ima/Kconfig            |  13 +++
 security/integrity/ima/Makefile           |   1 +
 security/integrity/ima/ima.h              |  75 ++++++++++++-
 security/integrity/ima/ima_api.c          |  10 +-
 security/integrity/ima/ima_appraise.c     | 166 +++++++++++++++++++++++-----
 security/integrity/ima/ima_fs.c           |   6 +-
 security/integrity/ima/ima_main.c         |  50 +++++++--
 security/integrity/ima/ima_modsig.c       | 176 ++++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |  65 ++++++-----
 security/integrity/ima/ima_queue.c        |   6 +-
 security/integrity/ima/ima_template.c     |  37 ++++---
 security/integrity/ima/ima_template_lib.c |  30 +++--
 security/integrity/integrity.h            |  10 +-
 29 files changed, 739 insertions(+), 196 deletions(-)
 create mode 100644 include/linux/module_signature.h
 create mode 100644 security/integrity/ima/ima_modsig.c

[PATCH v5 01/18] ima: Remove redundant conditional operator

[Thiago Jung Bauermann]

A non-zero value is converted to 1 when assigned to a bool variable, so the
conditional operator in is_ima_appraise_enabled is redundant.

The value of a comparison operator is either 1 or 0 so the conditional
operator in ima_inode_setxattr is redundant as well.

Confirmed that the patch is correct by comparing the object file from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..ec7dfa02c051 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -40,7 +40,7 @@ __setup("ima_appraise=", default_appraise_setup);
  */
 bool is_ima_appraise_enabled(void)
 {
-	return (ima_appraise & IMA_APPRAISE_ENFORCE) ? 1 : 0;
+	return ima_appraise & IMA_APPRAISE_ENFORCE;
 }
 
 /*
@@ -405,7 +405,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
 		ima_reset_appraise_flags(d_backing_inode(dentry),
-			 (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
+			xvalue->type == EVM_IMA_XATTR_DIGSIG);
 		result = 0;
 	}
 	return result;

[PATCH v5 02/18] ima: Remove some superfluous parentheses

[Thiago Jung Bauermann]

Superfluous parentheses just add clutter to the code, making it harder to
read and to understand.

In order to avoid churn and minimize conflicts with other patches from the
community, this patch only removes superfluous parentheses from lines that
are modified by other patches in this series.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c     | 11 +++++------
 security/integrity/ima/ima_template_lib.c |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ec7dfa02c051..bce0b36778bd 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -229,9 +229,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
 
 	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
-		    || (status == INTEGRITY_NOXATTRS))
+	if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
+		if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS)
 			cause = "missing-HMAC";
 		else if (status == INTEGRITY_FAIL)
 			cause = "invalid-HMAC";
@@ -293,10 +292,10 @@ int ima_appraise_measurement(enum ima_hooks func,
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
-		} else if ((inode->i_size == 0) &&
+		} else if (inode->i_size == 0 &&
 			   (iint->flags & IMA_NEW_FILE) &&
-			   (xattr_value &&
-			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+			   xattr_value &&
+			   xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
 			status = INTEGRITY_PASS;
 		}
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 28af43f63572..8bebcbb61162 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -383,7 +383,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
 	int xattr_len = event_data->xattr_len;
 	int rc = 0;
 
-	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+	if (!xattr_value || xattr_value->type != EVM_IMA_XATTR_DIGSIG)
 		goto out;
 
 	rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,

[PATCH v5 03/18] evm, ima: Remove superfluous parentheses

[Thiago Jung Bauermann]

This patch removes unnecessary parentheses from all EVM and IMA files
touched by this patch series.

The difference from the previous patch is that it cleans up the files as a
whole, not just the lines that were already going to be modified by other
patches. It is separate from the previous one so that it can be easily
dropped if the churn and conflict potential is deemed not worth it.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/evm/evm_crypto.c       |  2 +-
 security/integrity/evm/evm_main.c         | 13 +++++-----
 security/integrity/ima/ima_api.c          |  2 +-
 security/integrity/ima/ima_appraise.c     |  2 +-
 security/integrity/ima/ima_main.c         | 11 +++++----
 security/integrity/ima/ima_policy.c       | 41 ++++++++++++++++---------------
 security/integrity/ima/ima_template.c     | 25 +++++++++----------
 security/integrity/ima/ima_template_lib.c |  6 ++---
 8 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index bcd64baf8788..9c2d88c80b9d 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -199,7 +199,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 
 	error = -ENODATA;
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
-		if ((req_xattr_name && req_xattr_value)
+		if (req_xattr_name && req_xattr_value
 		    && !strcmp(*xattrname, req_xattr_name)) {
 			error = 0;
 			crypto_shash_update(desc, (const u8 *)req_xattr_value,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9826c02e2db8..37f062d38d5f 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -188,7 +188,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	}
 
 	if (rc)
-		evm_status = (rc == -ENODATA) ?
+		evm_status = rc == -ENODATA ?
 				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
 out:
 	if (iint)
@@ -205,8 +205,8 @@ static int evm_protected_xattr(const char *req_xattr_name)
 
 	namelen = strlen(req_xattr_name);
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
-		if ((strlen(*xattrname) == namelen)
-		    && (strncmp(req_xattr_name, *xattrname, namelen) == 0)) {
+		if (strlen(*xattrname) == namelen
+		    && strncmp(req_xattr_name, *xattrname, namelen) == 0) {
 			found = 1;
 			break;
 		}
@@ -294,8 +294,8 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 		if (!posix_xattr_acl(xattr_name))
 			return 0;
 		evm_status = evm_verify_current_integrity(dentry);
-		if ((evm_status == INTEGRITY_PASS) ||
-		    (evm_status == INTEGRITY_NOXATTRS))
+		if (evm_status == INTEGRITY_PASS ||
+		    evm_status == INTEGRITY_NOXATTRS)
 			return 0;
 		goto out;
 	}
@@ -434,8 +434,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
 		return 0;
 	evm_status = evm_verify_current_integrity(dentry);
-	if ((evm_status == INTEGRITY_PASS) ||
-	    (evm_status == INTEGRITY_NOXATTRS))
+	if (evm_status == INTEGRITY_PASS || evm_status == INTEGRITY_NOXATTRS)
 		return 0;
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c7e8db0ea4c0..c6d346e9f708 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -54,7 +54,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
 		u32 len;
 
 		result = field->field_init(event_data,
-					   &((*entry)->template_data[i]));
+					   &(*entry)->template_data[i]);
 		if (result != 0)
 			goto out;
 
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index bce0b36778bd..58c6a60c7e83 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -401,7 +401,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
 	if (result == 1) {
-		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
+		if (!xattr_value_len || xvalue->type >= IMA_XATTR_LAST)
 			return -EINVAL;
 		ima_reset_appraise_flags(d_backing_inode(dentry),
 			xvalue->type == EVM_IMA_XATTR_DIGSIG);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e4ab8ef8016e..747a4fd9e2de 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -96,7 +96,7 @@ static void ima_rdwr_violation_check(struct file *file,
 				send_tomtou = true;
 		}
 	} else {
-		if ((atomic_read(&inode->i_writecount) > 0) && must_measure)
+		if (atomic_read(&inode->i_writecount) > 0 && must_measure)
 			send_writers = true;
 	}
 
@@ -123,7 +123,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 
 	inode_lock(inode);
 	if (atomic_read(&inode->i_writecount) == 1) {
-		if ((iint->version != inode->i_version) ||
+		if (iint->version != inode->i_version ||
 		    (iint->flags & IMA_NEW_FILE)) {
 			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
 			iint->measured_pcrs = 0;
@@ -179,8 +179,9 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(inode, mask, func, &pcr);
-	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
-			   (ima_policy_flag & IMA_MEASURE));
+
+	violation_check = (func == FILE_CHECK || func == MMAP_CHECK) &&
+			  (ima_policy_flag & IMA_MEASURE);
 	if (!action && !violation_check)
 		return 0;
 
@@ -260,7 +261,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		__putname(pathbuf);
 out:
 	inode_unlock(inode);
-	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
+	if (rc && must_appraise && (ima_appraise & IMA_APPRAISE_ENFORCE))
 		return -EACCES;
 	return 0;
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..efd8e1c60c10 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -41,8 +41,8 @@
 #define DONT_APPRAISE	0x0008
 #define AUDIT		0x0040
 
-#define INVALID_PCR(a) (((a) < 0) || \
-	(a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
+#define INVALID_PCR(a) ((a) < 0 || \
+	(a) >= FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8)
 
 int ima_policy_flag;
 static int temp_ima_appraise;
@@ -193,7 +193,7 @@ static int __init policy_setup(char *str)
 	while ((p = strsep(&str, " |\n")) != NULL) {
 		if (*p == ' ')
 			continue;
-		if ((strcmp(p, "tcb") == 0) && !ima_policy)
+		if (strcmp(p, "tcb") == 0 && !ima_policy)
 			ima_policy = DEFAULT_TCB;
 		else if (strcmp(p, "appraise_tcb") == 0)
 			ima_use_appraise_tcb = 1;
@@ -254,13 +254,13 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	int i;
 
 	if ((rule->flags & IMA_FUNC) &&
-	    (rule->func != func && func != POST_SETATTR))
+	    rule->func != func && func != POST_SETATTR)
 		return false;
 	if ((rule->flags & IMA_MASK) &&
-	    (rule->mask != mask && func != POST_SETATTR))
+	    rule->mask != mask && func != POST_SETATTR)
 		return false;
 	if ((rule->flags & IMA_INMASK) &&
-	    (!(rule->mask & mask) && func != POST_SETATTR))
+	    !(rule->mask & mask) && func != POST_SETATTR)
 		return false;
 	if ((rule->flags & IMA_FSMAGIC)
 	    && rule->fsmagic != inode->i_sb->s_magic)
@@ -314,7 +314,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		default:
 			break;
 		}
-		if ((rc < 0) && (!retried)) {
+		if (rc < 0 && !retried) {
 			retried = 1;
 			ima_lsm_update_rules();
 			goto retry;
@@ -388,7 +388,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		else
 			actmask &= ~(entry->action | entry->action >> 1);
 
-		if ((pcr) && (entry->flags & IMA_PCR))
+		if (pcr && (entry->flags & IMA_PCR))
 			*pcr = entry->pcr;
 
 		if (!actmask)
@@ -627,7 +627,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 
 		if (result < 0)
 			break;
-		if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
+		if (*p == '\0' || *p == ' ' || *p == '\t')
 			continue;
 		token = match_token(p, policy_tokens, args);
 		switch (token) {
@@ -686,8 +686,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = MODULE_CHECK;
 			else if (strcmp(args[0].from, "FIRMWARE_CHECK") == 0)
 				entry->func = FIRMWARE_CHECK;
-			else if ((strcmp(args[0].from, "FILE_MMAP") == 0)
-				|| (strcmp(args[0].from, "MMAP_CHECK") == 0))
+			else if (strcmp(args[0].from, "FILE_MMAP") == 0
+				|| strcmp(args[0].from, "MMAP_CHECK") == 0)
 				entry->func = MMAP_CHECK;
 			else if (strcmp(args[0].from, "BPRM_CHECK") == 0)
 				entry->func = BPRM_CHECK;
@@ -714,7 +714,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			if (*from == '^')
 				from++;
 
-			if ((strcmp(from, "MAY_EXEC")) == 0)
+			if (strcmp(from, "MAY_EXEC") == 0)
 				entry->mask = MAY_EXEC;
 			else if (strcmp(from, "MAY_WRITE") == 0)
 				entry->mask = MAY_WRITE;
@@ -757,13 +757,13 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			entry->uid_op = &uid_gt;
 		case Opt_uid_lt:
 		case Opt_euid_lt:
-			if ((token == Opt_uid_lt) || (token == Opt_euid_lt))
+			if (token == Opt_uid_lt || token == Opt_euid_lt)
 				entry->uid_op = &uid_lt;
 		case Opt_uid_eq:
 		case Opt_euid_eq:
-			uid_token = (token == Opt_uid_eq) ||
-				    (token == Opt_uid_gt) ||
-				    (token == Opt_uid_lt);
+			uid_token = token == Opt_uid_eq ||
+				    token == Opt_uid_gt ||
+				    token == Opt_uid_lt;
 
 			ima_log_string_op(ab, uid_token ? "uid" : "euid",
 					  args[0].from, entry->uid_op);
@@ -802,7 +802,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			result = kstrtoul(args[0].from, 10, &lnum);
 			if (!result) {
 				entry->fowner = make_kuid(current_user_ns(), (uid_t)lnum);
-				if (!uid_valid(entry->fowner) || (((uid_t)lnum) != lnum))
+				if (!uid_valid(entry->fowner) ||
+				    (uid_t) lnum != lnum)
 					result = -EINVAL;
 				else
 					entry->flags |= IMA_FOWNER;
@@ -851,7 +852,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			}
 
 			ima_log_string(ab, "appraise_type", args[0].from);
-			if ((strcmp(args[0].from, "imasig")) == 0)
+			if (strcmp(args[0].from, "imasig") == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED;
 			else
 				result = -EINVAL;
@@ -879,7 +880,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			break;
 		}
 	}
-	if (!result && (entry->action == UNKNOWN))
+	if (!result && entry->action == UNKNOWN)
 		result = -EINVAL;
 	else if (entry->func == MODULE_CHECK)
 		temp_ima_appraise |= IMA_APPRAISE_MODULES;
@@ -1001,7 +1002,7 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
 	rcu_read_unlock();
 	(*pos)++;
 
-	return (&entry->list == ima_rules) ? NULL : entry;
+	return &entry->list == ima_rules ? NULL : entry;
 }
 
 void ima_policy_stop(struct seq_file *m, void *v)
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 7412d0291ab9..3cc1d2763fd2 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -115,8 +115,8 @@ static struct ima_template_desc *lookup_template_desc(const char *name)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(template_desc, &defined_templates, list) {
-		if ((strcmp(template_desc->name, name) == 0) ||
-		    (strcmp(template_desc->fmt, name) == 0)) {
+		if (strcmp(template_desc->name, name) == 0 ||
+		    strcmp(template_desc->fmt, name) == 0) {
 			found = 1;
 			break;
 		}
@@ -233,13 +233,12 @@ int __init ima_init_template(void)
 	struct ima_template_desc *template = ima_template_desc_current();
 	int result;
 
-	result = template_desc_init_fields(template->fmt,
-					   &(template->fields),
-					   &(template->num_fields));
+	result = template_desc_init_fields(template->fmt, &template->fields,
+					   &template->num_fields);
 	if (result < 0)
 		pr_err("template %s init failed, result: %d\n",
-		       (strlen(template->name) ?
-		       template->name : template->fmt), result);
+		       strlen(template->name) ? template->name : template->fmt,
+		       result);
 
 	return result;
 }
@@ -367,10 +366,10 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 	 *	      template-data-size, template-data
 	 */
 	bufendp = buf + khdr->buffer_size;
-	while ((bufp < bufendp) && (count++ < khdr->count)) {
+	while (bufp < bufendp && count++ < khdr->count) {
 		int enforce_mask = ENFORCE_FIELDS;
 
-		enforce_mask |= (count == khdr->count) ? ENFORCE_BUFEND : 0;
+		enforce_mask |= count == khdr->count ? ENFORCE_BUFEND : 0;
 		ret = ima_parse_buf(bufp, bufendp, &bufp, HDR__LAST, hdr, NULL,
 				    hdr_mask, enforce_mask, "entry header");
 		if (ret < 0)
@@ -407,8 +406,8 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 		 * on boot.  As needed, initialize the other template formats.
 		 */
 		ret = template_desc_init_fields(template_desc->fmt,
-						&(template_desc->fields),
-						&(template_desc->num_fields));
+						&template_desc->fields,
+						&template_desc->num_fields);
 		if (ret < 0) {
 			pr_err("attempting to restore the template fmt \"%s\" \
 				failed\n", template_desc->fmt);
@@ -425,8 +424,8 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 
 		memcpy(entry->digest, hdr[HDR_DIGEST].data,
 		       hdr[HDR_DIGEST].len);
-		entry->pcr = !ima_canonical_fmt ? *(hdr[HDR_PCR].data) :
-			     le32_to_cpu(*(hdr[HDR_PCR].data));
+		entry->pcr = !ima_canonical_fmt ? *hdr[HDR_PCR].data :
+			     le32_to_cpu(*hdr[HDR_PCR].data);
 		ret = ima_restore_measurement_entry(entry);
 		if (ret < 0)
 			break;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 8bebcbb61162..d941260e979f 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -100,7 +100,7 @@ static void ima_show_template_data_binary(struct seq_file *m,
 					  enum data_formats datafmt,
 					  struct ima_field_data *field_data)
 {
-	u32 len = (show == IMA_SHOW_BINARY_OLD_STRING_FMT) ?
+	u32 len = show == IMA_SHOW_BINARY_OLD_STRING_FMT ?
 	    strlen(field_data->data) : field_data->len;
 
 	if (show != IMA_SHOW_BINARY_NO_FIELD_LEN) {
@@ -182,7 +182,7 @@ int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
 
 	for (i = 0; i < maxfields; i++) {
 		if (len_mask == NULL || !test_bit(i, len_mask)) {
-			if (bufp > (bufendp - sizeof(u32)))
+			if (bufp > bufendp - sizeof(u32))
 				break;
 
 			fields[i].len = *(u32 *)bufp;
@@ -192,7 +192,7 @@ int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp,
 			bufp += sizeof(u32);
 		}
 
-		if (bufp > (bufendp - fields[i].len))
+		if (bufp > bufendp - fields[i].len)
 			break;
 
 		fields[i].data = bufp;

[PATCH v5 04/18] evm, ima: Remove more superfluous parentheses

[Thiago Jung Bauermann]

This patch removes unnecessary parentheses from all EVM and IMA files
not yet cleaned up by the previous patches.

It is separate from the previous one so that it can be easily dropped if
the churn and conflict potential is deemed not worth it.

Confirmed that the patch is correct by comparing the object files from
before and after the patch. They are identical.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/evm/evm_posix_acl.c | 8 ++++----
 security/integrity/ima/ima_fs.c        | 6 +++---
 security/integrity/ima/ima_queue.c     | 6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_posix_acl.c b/security/integrity/evm/evm_posix_acl.c
index 46408b9e62e8..0a32798cfc96 100644
--- a/security/integrity/evm/evm_posix_acl.c
+++ b/security/integrity/evm/evm_posix_acl.c
@@ -17,11 +17,11 @@ int posix_xattr_acl(const char *xattr)
 {
 	int xattr_len = strlen(xattr);
 
-	if ((strlen(XATTR_NAME_POSIX_ACL_ACCESS) == xattr_len)
-	     && (strncmp(XATTR_NAME_POSIX_ACL_ACCESS, xattr, xattr_len) == 0))
+	if (strlen(XATTR_NAME_POSIX_ACL_ACCESS) == xattr_len
+	    && strncmp(XATTR_NAME_POSIX_ACL_ACCESS, xattr, xattr_len) == 0)
 		return 1;
-	if ((strlen(XATTR_NAME_POSIX_ACL_DEFAULT) == xattr_len)
-	     && (strncmp(XATTR_NAME_POSIX_ACL_DEFAULT, xattr, xattr_len) == 0))
+	if (strlen(XATTR_NAME_POSIX_ACL_DEFAULT) == xattr_len
+	    && strncmp(XATTR_NAME_POSIX_ACL_DEFAULT, xattr, xattr_len) == 0)
 		return 1;
 	return 0;
 }
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 4d50b982b453..552f79fc4291 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -105,7 +105,7 @@ static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos)
 	rcu_read_unlock();
 	(*pos)++;
 
-	return (&qe->later == &ima_measurements) ? NULL : qe;
+	return &qe->later == &ima_measurements ? NULL : qe;
 }
 
 static void ima_measurements_stop(struct seq_file *m, void *v)
@@ -141,7 +141,7 @@ int ima_measurements_show(struct seq_file *m, void *v)
 	if (e == NULL)
 		return -1;
 
-	template_name = (e->template_desc->name[0] != '\0') ?
+	template_name = e->template_desc->name[0] != '\0' ?
 	    e->template_desc->name : e->template_desc->fmt;
 
 	/*
@@ -228,7 +228,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 	if (e == NULL)
 		return -1;
 
-	template_name = (e->template_desc->name[0] != '\0') ?
+	template_name = e->template_desc->name[0] != '\0' ?
 	    e->template_desc->name : e->template_desc->fmt;
 
 	/* 1st: PCR used (config option) */
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index a02a86d51102..2ef7d33ba1fc 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -60,7 +60,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) {
 		rc = memcmp(qe->entry->digest, digest_value, TPM_DIGEST_SIZE);
-		if ((rc == 0) && (qe->entry->pcr == pcr)) {
+		if (rc == 0 && qe->entry->pcr == pcr) {
 			ret = qe;
 			break;
 		}
@@ -119,7 +119,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 		int size;
 
 		size = get_binary_runtime_size(entry);
-		binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
+		binary_runtime_size = binary_runtime_size < ULONG_MAX - size ?
 		     binary_runtime_size + size : ULONG_MAX;
 	}
 	return 0;
@@ -132,7 +132,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
  */
 unsigned long ima_get_binary_runtime_size(void)
 {
-	if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr)))
+	if (binary_runtime_size >= ULONG_MAX - sizeof(struct ima_kexec_hdr))
 		return ULONG_MAX;
 	else
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);

[PATCH v5 05/18] ima: Simplify ima_eventsig_init

[Thiago Jung Bauermann]

The "goto out" statement doesn't have any purpose since there's no cleanup
to be done when returning early, so remove it. This also makes the rc
variable unnecessary so remove it as well.

Also, the xattr_len and fmt variables are redundant so remove them as well.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template_lib.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index d941260e979f..e8ec783b6a8d 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -378,16 +378,11 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
 int ima_eventsig_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data)
 {
-	enum data_formats fmt = DATA_FMT_HEX;
 	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
-	int xattr_len = event_data->xattr_len;
-	int rc = 0;
 
 	if (!xattr_value || xattr_value->type != EVM_IMA_XATTR_DIGSIG)
-		goto out;
+		return 0;
 
-	rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,
-					   field_data);
-out:
-	return rc;
+	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+					     DATA_FMT_HEX, field_data);
 }

[PATCH v5 06/18] ima: Improvements in ima_appraise_measurement

[Thiago Jung Bauermann]

Replace nested ifs in the EVM xattr verification logic with a switch
statement, making the code easier to understand.

Also, add comments to the if statements in the out section.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h          |  5 +++++
 security/integrity/ima/ima_appraise.c | 33 ++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..4e61d3189b72 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -155,6 +155,11 @@ unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 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;
+}
+
 /*
  * used to protect h_table and sha_table
  */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 58c6a60c7e83..2c069f47eeec 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -204,7 +204,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			     int xattr_len, int opened)
 {
 	static const char op[] = "appraise_data";
-	char *cause = "unknown";
+	const char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
@@ -229,11 +229,16 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
 
 	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
-		if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS)
-			cause = "missing-HMAC";
-		else if (status == INTEGRITY_FAIL)
-			cause = "invalid-HMAC";
+	switch (status) {
+	case INTEGRITY_PASS:
+	case INTEGRITY_UNKNOWN:
+		break;
+	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
+	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
+		cause = "missing-HMAC";
+		goto out;
+	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
+		cause = "invalid-HMAC";
 		goto out;
 	}
 	switch (xattr_value->type) {
@@ -287,17 +292,19 @@ int ima_appraise_measurement(enum ima_hooks func,
 
 out:
 	if (status != INTEGRITY_PASS) {
+		/* Fix mode, but don't replace file signatures. */
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
-		    (!xattr_value ||
-		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+		    !is_ima_sig(xattr_value)) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
-		} else if (inode->i_size == 0 &&
-			   (iint->flags & IMA_NEW_FILE) &&
-			   xattr_value &&
-			   xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+		}
+
+		/* Permit new files with file signatures, but without data. */
+		if (inode->i_size == 0 && (iint->flags & IMA_NEW_FILE) &&
+		    is_ima_sig(xattr_value)) {
 			status = INTEGRITY_PASS;
 		}
+
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
 	} else {
@@ -404,7 +411,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		if (!xattr_value_len || xvalue->type >= IMA_XATTR_LAST)
 			return -EINVAL;
 		ima_reset_appraise_flags(d_backing_inode(dentry),
-			xvalue->type == EVM_IMA_XATTR_DIGSIG);
+					 is_ima_sig(xvalue));
 		result = 0;
 	}
 	return result;

[PATCH v5 07/18] integrity: Introduce struct evm_xattr

[Thiago Jung Bauermann]

Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/evm/evm_crypto.c   |  4 ++--
 security/integrity/evm/evm_main.c     | 10 +++++-----
 security/integrity/ima/ima_appraise.c |  7 ++++---
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 9c2d88c80b9d..5fddc28e8a0e 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,13 +252,13 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_xattr xattr_data;
 	int rc = 0;
 
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 			   xattr_value_len, xattr_data.digest);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		xattr_data.data.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
 					   &xattr_data,
 					   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 37f062d38d5f..7f31f65b8492 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -119,7 +119,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct evm_xattr calc;
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	int rc, xattr_len;
 
@@ -150,7 +150,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
-		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+		if (xattr_len != sizeof(struct evm_xattr)) {
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
@@ -158,7 +158,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				   xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
+		rc = crypto_memneq(xattr_data->data, calc.digest,
 			    sizeof(calc.digest));
 		if (rc)
 			rc = -EINVAL;
@@ -469,7 +469,7 @@ int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
 				 struct xattr *evm_xattr)
 {
-	struct evm_ima_xattr_data *xattr_data;
+	struct evm_xattr *xattr_data;
 	int rc;
 
 	if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
@@ -479,7 +479,7 @@ int evm_inode_init_security(struct inode *inode,
 	if (!xattr_data)
 		return -ENOMEM;
 
-	xattr_data->type = EVM_XATTR_HMAC;
+	xattr_data->data.type = EVM_XATTR_HMAC;
 	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
 	if (rc < 0)
 		goto out;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 2c069f47eeec..091977c8ec40 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		return sig->hash_algo;
 		break;
 	case IMA_XATTR_DIGEST_NG:
-		ret = xattr_value->digest[0];
+		/* first byte contains algorithm id */
+		ret = xattr_value->data[0];
 		if (ret < HASH_ALGO__LAST)
 			return ret;
 		break;
@@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		/* this is for backward compatibility */
 		if (xattr_len == 21) {
 			unsigned int zero = 0;
-			if (!memcmp(&xattr_value->digest[16], &zero, 4))
+			if (!memcmp(&xattr_value->data[16], &zero, 4))
 				return HASH_ALGO_MD5;
 			else
 				return HASH_ALGO_SHA1;
@@ -257,7 +258,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			/* xattr length may be longer. md5 hash in previous
 			   version occupied 20 bytes in xattr, instead of 16
 			 */
-			rc = memcmp(&xattr_value->digest[hash_start],
+			rc = memcmp(&xattr_value->data[hash_start],
 				    iint->ima_hash->digest,
 				    iint->ima_hash->length);
 		else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e1bf040fb110..dd7ac4bfc89a 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -63,6 +63,11 @@ enum evm_ima_xattr_type {
 
 struct evm_ima_xattr_data {
 	u8 type;
+	u8 data[];
+} __packed;
+
+struct evm_xattr {
+	struct evm_ima_xattr_data data;
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 

[PATCH v5 08/18] integrity: Select CONFIG_KEYS instead of depending on it

[Thiago Jung Bauermann]

This avoids a dependency cycle in CONFIG_IMA_APPRAISE_MODSIG (introduced by
a later patch in this series): it will select CONFIG_MODULE_SIG_FORMAT
which in turn selects CONFIG_KEYS. Kconfig then complains that
CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..0d642e0317c7 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
 
 config INTEGRITY_SIGNATURE
 	bool "Digital signature verification using multiple keyrings"
-	depends on KEYS
 	default n
+	select KEYS
 	select SIGNATURE
 	help
 	  This option enables digital signature verification support

[PATCH v5 09/18] ima: Don't pass xattr value to EVM xattr verification.

[Thiago Jung Bauermann]

The patch implementing modsig support will retry verifying the xattr
signature if the modsig verification fails, and if we have already passed
the modsig as the xattr_value we'll have problems if we pass the xattr sig
in the second call to evm_verifyxattr.

Since this is an optimization and not actually required, just don't do it.

Suggested-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 091977c8ec40..58e147049e98 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -229,7 +229,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		goto out;
 	}
 
-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, NULL, 0, iint);
 	switch (status) {
 	case INTEGRITY_PASS:
 	case INTEGRITY_UNKNOWN:

[PATCH v5 10/18] ima: Store measurement after appraisal

[Thiago Jung Bauermann]

When module-style signatures appended at the end of files are supported for
IMA appraisal, the code will fallback to the xattr signature if the
appended one fails to verify.

The problem is that we don't know whether we need to fallback to the xattr
signature until the appraise step, and by then the measure step was already
completed and would need to be done again in case the template includes the
signature.

To avoid this problem, do the appraisal first so that the correct signature
is stored by the template in the measure step.

Suggested-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 747a4fd9e2de..8e96450e27f5 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -242,12 +242,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
-	if (action & IMA_MEASURE)
-		ima_store_measurement(iint, file, pathname,
-				      xattr_value, xattr_len, pcr);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK))
 		rc = ima_appraise_measurement(func, iint, file, pathname,
 					      xattr_value, xattr_len, opened);
+	if (action & IMA_MEASURE)
+		ima_store_measurement(iint, file, pathname,
+				      xattr_value, xattr_len, pcr);
 	if (action & IMA_AUDIT)
 		ima_audit_measurement(iint, pathname);
 

[PATCH v5 11/18] ima: Export func_tokens

[Thiago Jung Bauermann]

ima_read_modsig will need it so that it can show an error message.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h        |  2 ++
 security/integrity/ima/ima_policy.c | 12 ++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4e61d3189b72..c9ba4362760b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -195,6 +195,8 @@ enum ima_hooks {
 	__ima_hooks(__ima_hook_enumify)
 };
 
+extern const char *const func_tokens[];
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, int mask,
 		   enum ima_hooks func, int *pcr);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index efd8e1c60c10..7355d4ab7bf4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -959,6 +959,12 @@ void ima_delete_rules(void)
 	}
 }
 
+#define __ima_hook_stringify(str)	(#str),
+
+const char *const func_tokens[] = {
+	__ima_hooks(__ima_hook_stringify)
+};
+
 #ifdef	CONFIG_IMA_READ_POLICY
 enum {
 	mask_exec = 0, mask_write, mask_read, mask_append
@@ -971,12 +977,6 @@ static const char *const mask_tokens[] = {
 	"MAY_APPEND"
 };
 
-#define __ima_hook_stringify(str)	(#str),
-
-static const char *const func_tokens[] = {
-	__ima_hooks(__ima_hook_stringify)
-};
-
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t l = *pos;

[PATCH v5 12/18] MODSIGN: Export module signature definitions

[Thiago Jung Bauermann]

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 validate_module_signature without having to depend on
CONFIG_MODULE_SIG.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/module.h           |  3 --
 include/linux/module_signature.h | 47 +++++++++++++++++++++++++
 init/Kconfig                     |  6 +++-
 kernel/Makefile                  |  2 +-
 kernel/module.c                  |  1 +
 kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
 6 files changed, 85 insertions(+), 48 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index fe5aa3736707..108874af9838 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -23,9 +23,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..e80728e5b86c
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,47 @@
+/* Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#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 validate_module_sig(const struct module_signature *ms, size_t file_len);
+int mod_verify_sig(const void *mod, unsigned long *_modlen);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 78cb2461012e..230e9f3aedaf 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1748,7 +1748,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
@@ -1763,6 +1763,10 @@ config MODULE_SIG
 	  debuginfo strip done by some packagers (such as rpmbuild) and
 	  inclusion into an initramfs that wants the module size reduced.
 
+config MODULE_SIG_FORMAT
+	def_bool n
+	select SYSTEM_DATA_VERIFICATION
+
 config MODULE_SIG_FORCE
 	bool "Require modules to be validly signed"
 	depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index ed470aac53da..20fd6d76232e 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -57,7 +57,7 @@ obj-y += up.o
 endif
 obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.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 de66ec825992..a259e9df570d 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_signing.c b/kernel/module_signing.c
index 937c844bee4a..204c60d4cc9f 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,38 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.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:
+/**
+ * validate_module_sig - validate that the given signature is sane
  *
- *	- Signer's name
- *	- Key identifier
- *	- Signature data
- *	- Information block
+ * @ms:		Signature to validate.
+ * @file_len:	Size of the file to which @ms is appended.
  */
-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 validate_module_sig(const struct module_signature *ms, size_t file_len)
+{
+	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+		return -EBADMSG;
+	else if (ms->id_type != PKEY_ID_PKCS7) {
+		pr_err("Module is not signed with expected PKCS#7 message\n");
+		return -ENOPKG;
+	} else 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("PKCS#7 signature info has unexpected non-zero params\n");
+		return -EBADMSG;
+	}
+
+	return 0;
+}
 
 /*
  * Verify the signature on a module.
@@ -49,6 +51,7 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 {
 	struct module_signature ms;
 	size_t modlen = *_modlen, sig_len;
+	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
@@ -56,30 +59,15 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 		return -EBADMSG;
 
 	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
-	modlen -= sizeof(ms);
+
+	ret = validate_module_sig(&ms, modlen);
+	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);
 	*_modlen = modlen;
 
-	if (ms.id_type != PKEY_ID_PKCS7) {
-		pr_err("Module is not signed with expected PKCS#7 message\n");
-		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("PKCS#7 signature info has unexpected non-zero params\n");
-		return -EBADMSG;
-	}
-
 	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
 				      NULL, VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);

[PATCH v5 13/18] PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig

[Thiago Jung Bauermann]

IMA will need to access the digest used in the signature so that it can
verify files containing module-style appended signatures. For this purpose,
add function pkcs7_get_message_sig.

It will also need to verify an already parsed PKCS#7 message. For this
purpose, add function verify_pkcs7_message_signature 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>
---
 certs/system_keyring.c                | 60 +++++++++++++++++++++++++----------
 crypto/asymmetric_keys/pkcs7_parser.c | 12 +++++++
 include/crypto/pkcs7.h                |  2 ++
 include/linux/verification.h          | 10 ++++++
 4 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b27f0c..6a8684959780 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -190,33 +190,26 @@ 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 +251,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 af4cd8649117..e41beda297a8 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
 		return -ENOMEM;
 	return 0;
 }
+
+/**
+ * pkcs7_get_message_sig - get signature in @pkcs7
+ *
+ * This function doesn't meaningfully support messages with more than one
+ * signature. It will always return the first signature.
+ */
+const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7)
+{
+	return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL;
+}
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,

[PATCH v5 14/18] integrity: Introduce integrity_keyring_from_id

[Thiago Jung Bauermann]

IMA will need to obtain the keyring used to verify file signatures so that
it can verify the module-style signature appended to files.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/digsig.c    | 28 +++++++++++++++++++---------
 security/integrity/integrity.h |  1 +
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 6f9e4ce568cd..935f1b3258e7 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -48,11 +48,10 @@ static bool init_keyring __initdata;
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-			    const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
 {
-	if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
-		return -EINVAL;
+	if (id >= INTEGRITY_KEYRING_MAX)
+		return ERR_PTR(-EINVAL);
 
 	if (!keyring[id]) {
 		keyring[id] =
@@ -61,18 +60,29 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			int err = PTR_ERR(keyring[id]);
 			pr_err("no %s keyring: %d\n", keyring_name[id], err);
 			keyring[id] = NULL;
-			return err;
+			return ERR_PTR(err);
 		}
 	}
 
+	return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+			    const char *digest, int digestlen)
+{
+	struct key *keyring = integrity_keyring_from_id(id);
+
+	if (IS_ERR(keyring) || siglen < 2)
+		return PTR_ERR(keyring);
+
 	switch (sig[1]) {
 	case 1:
 		/* v1 API expect signature without xattr type */
-		return digsig_verify(keyring[id], sig + 1, siglen - 1,
-				     digest, digestlen);
+		return digsig_verify(keyring, sig + 1, siglen - 1, digest,
+				     digestlen);
 	case 2:
-		return asymmetric_verify(keyring[id], sig, siglen,
-					 digest, digestlen);
+		return asymmetric_verify(keyring, sig, siglen, digest,
+					 digestlen);
 	}
 
 	return -EOPNOTSUPP;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index dd7ac4bfc89a..657ce4558984 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -132,6 +132,7 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
 
+struct key *integrity_keyring_from_id(const unsigned int id);
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			    const char *digest, int digestlen);
 

[PATCH v5 15/18] ima: Add modsig appraise_type option for module-style appended signatures

[Thiago Jung Bauermann]

This patch introduces the modsig keyword to the IMA policy syntax to
specify that a given hook should expect the file to have the IMA signature
appended to it. Here is how it can be used in a rule:

appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig

With this rule, IMA will accept either an appended signature or a signature
stored in the extended attribute. In that case, it will first check whether
there is an appended signature, and if not it will read it from the
extended attribute.

For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified because the actual modsig implementation
will be introduced in a separate patch.

Suggested-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/ima_policy |  6 +++++-
 security/integrity/ima/Kconfig       | 10 +++++++++
 security/integrity/ima/Makefile      |  1 +
 security/integrity/ima/ima.h         | 11 ++++++++++
 security/integrity/ima/ima_modsig.c  | 39 ++++++++++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c  | 12 +++++++++--
 security/integrity/integrity.h       |  3 ++-
 7 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index e76432b9954d..ced1e1835a30 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -36,7 +36,7 @@ Description:
 			euid:= decimal value
 			fowner:= decimal value
 		lsm:  	are LSM specific
-		option:	appraise_type:= [imasig]
+		option:	appraise_type:= [imasig] [modsig|imasig]
 			pcr:= decimal value
 
 		default policy:
@@ -102,3 +102,7 @@ Description:
 
 			measure func=KEXEC_KERNEL_CHECK pcr=4
 			measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+		Example of appraise rule allowing modsig appended signatures:
+
+			appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 35ef69312811..40c6618d00e6 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -163,6 +163,16 @@ config IMA_APPRAISE_BOOTPARAM
 	  This option enables the different "ima_appraise=" modes
 	  (eg. fix, log) from the boot command line.
 
+config IMA_APPRAISE_MODSIG
+	bool "Support module-style signatures for appraisal"
+	depends on IMA_APPRAISE
+	default n
+	help
+	   Adds support for signatures appended to files. The format of the
+	   appended signature is the same used for signed kernel modules.
+	   The modsig keyword can be used in the IMA policy to allow a hook
+	   to accept such signatures.
+
 config IMA_TRUSTED_KEYRING
 	bool "Require all keys on the .ima keyring be signed (deprecated)"
 	depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 29f198bde02b..c72026acecc3 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 	 ima_policy.o ima_template.o ima_template_lib.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c9ba4362760b..156ba218e0b6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -255,6 +255,10 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#endif
+
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
@@ -298,6 +302,13 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #endif /* CONFIG_IMA_APPRAISE */
 
+#ifndef CONFIG_IMA_APPRAISE_MODSIG
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+	return false;
+}
+#endif
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
new file mode 100644
index 000000000000..452ce6048a7e
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,39 @@
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2017  IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation (version 2 of the License).
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include "ima.h"
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file, because only they
+ * preload the contents of the file in a buffer. FILE_CHECK does that in some
+ * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but
+ * it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+	switch (func) {
+	case FIRMWARE_CHECK:
+	case KEXEC_KERNEL_CHECK:
+	case KEXEC_INITRAMFS_CHECK:
+		return true;
+	default:
+		return false;
+	}
+}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 7355d4ab7bf4..c5f98c54e59d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -854,6 +854,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			ima_log_string(ab, "appraise_type", args[0].from);
 			if (strcmp(args[0].from, "imasig") == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED;
+			else if (ima_hook_supports_modsig(entry->func) &&
+				 strcmp(args[0].from, "modsig|imasig") == 0)
+				entry->flags |= IMA_DIGSIG_REQUIRED
+						| IMA_MODSIG_ALLOWED;
 			else
 				result = -EINVAL;
 			break;
@@ -1139,8 +1143,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 			}
 		}
 	}
-	if (entry->flags & IMA_DIGSIG_REQUIRED)
-		seq_puts(m, "appraise_type=imasig ");
+	if (entry->flags & IMA_DIGSIG_REQUIRED) {
+		if (entry->flags & IMA_MODSIG_ALLOWED)
+			seq_puts(m, "appraise_type=modsig|imasig ");
+		else
+			seq_puts(m, "appraise_type=imasig ");
+	}
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
 	rcu_read_unlock();
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 657ce4558984..da709558ec8b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -28,11 +28,12 @@
 
 /* iint cache flags */
 #define IMA_ACTION_FLAGS	0xff000000
-#define IMA_ACTION_RULE_FLAGS	0x06000000
+#define IMA_ACTION_RULE_FLAGS	0x16000000
 #define IMA_DIGSIG		0x01000000
 #define IMA_DIGSIG_REQUIRED	0x02000000
 #define IMA_PERMIT_DIRECTIO	0x04000000
 #define IMA_NEW_FILE		0x08000000
+#define IMA_MODSIG_ALLOWED	0x10000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)

[PATCH v5 16/18] ima: Add functions to read and verify a modsig signature

[Thiago Jung Bauermann]

This is the code needed by IMA-appraise to work with modsig signatures.
It will be used by the next patch.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/Kconfig      |   3 +
 security/integrity/ima/ima.h        |  34 +++++++++++
 security/integrity/ima/ima_modsig.c | 119 ++++++++++++++++++++++++++++++++++++
 security/integrity/integrity.h      |   1 +
 4 files changed, 157 insertions(+)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 40c6618d00e6..55f734a6124b 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -166,6 +166,9 @@ config IMA_APPRAISE_BOOTPARAM
 config IMA_APPRAISE_MODSIG
 	bool "Support module-style signatures for appraisal"
 	depends on IMA_APPRAISE
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	select PKCS7_MESSAGE_PARSER
+	select MODULE_SIG_FORMAT
 	default n
 	help
 	   Adds support for signatures appended to files. The format of the
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 156ba218e0b6..eb58af06566f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -257,6 +257,14 @@ int ima_read_xattr(struct dentry *dentry,
 
 #ifdef CONFIG_IMA_APPRAISE_MODSIG
 bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+		    struct evm_ima_xattr_data **xattr_value,
+		    int *xattr_len);
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+			void const **hash, u8 *len);
+int ima_modsig_verify(const unsigned int keyring_id,
+		      struct evm_ima_xattr_data *hdr);
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
 #endif
 
 #else
@@ -307,6 +315,32 @@ static inline bool ima_hook_supports_modsig(enum ima_hooks func)
 {
 	return false;
 }
+
+static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
+				  loff_t buf_len,
+				  struct evm_ima_xattr_data **xattr_value,
+				  int *xattr_len)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
+				      enum hash_algo *algo, void const **hash,
+				      u8 *len)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ima_modsig_verify(const unsigned int keyring_id,
+				    struct evm_ima_xattr_data *hdr)
+{
+	return -ENOTSUPP;
+}
+
+static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+	kfree(hdr);
+}
 #endif
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index 452ce6048a7e..2786aa97060e 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -16,8 +16,19 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/types.h>
+#include <linux/module_signature.h>
+#include <crypto/pkcs7.h>
+
 #include "ima.h"
 
+struct modsig_hdr {
+	uint8_t type;		/* Should be IMA_MODSIG. */
+	const void *data;	/* Pointer to data covered by pkcs7_msg. */
+	size_t data_len;
+	struct pkcs7_message *pkcs7_msg;
+};
+
 /**
  * ima_hook_supports_modsig - can the policy allow modsig for this hook?
  *
@@ -37,3 +48,111 @@ bool ima_hook_supports_modsig(enum ima_hooks func)
 		return false;
 	}
 }
+
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+		    struct evm_ima_xattr_data **xattr_value,
+		    int *xattr_len)
+{
+	const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	const struct module_signature *sig;
+	struct modsig_hdr *hdr;
+	size_t sig_len;
+	const void *p;
+	int rc;
+
+	/*
+	 * Not supposed to happen. Hooks that support modsig are
+	 * whitelisted when parsing the policy using
+	 * ima_hooks_supports_modsig.
+	 */
+	if (!buf || !buf_len) {
+		WARN_ONCE(true, "%s doesn't support modsig\n",
+			  func_tokens[func]);
+		return -ENOENT;
+	} else if (buf_len <= marker_len + sizeof(*sig))
+		return -ENOENT;
+
+	p = buf + buf_len - marker_len;
+	if (memcmp(p, MODULE_SIG_STRING, marker_len))
+		return -ENOENT;
+
+	buf_len -= marker_len;
+	sig = (const struct module_signature *) (p - sizeof(*sig));
+
+	rc = validate_module_sig(sig, buf_len);
+	if (rc)
+		return rc;
+
+	sig_len = be32_to_cpu(sig->sig_len);
+	buf_len -= sig_len + sizeof(*sig);
+
+	hdr = kmalloc(sizeof(*hdr), GFP_KERNEL);
+	if (!hdr)
+		return -ENOMEM;
+
+	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
+	if (IS_ERR(hdr->pkcs7_msg)) {
+		rc = PTR_ERR(hdr->pkcs7_msg);
+		kfree(hdr);
+		return rc;
+	}
+
+	hdr->type = IMA_MODSIG;
+	hdr->data = buf;
+	hdr->data_len = buf_len;
+
+	*xattr_value = (typeof(*xattr_value)) hdr;
+	*xattr_len = sizeof(*hdr);
+
+	return 0;
+}
+
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+			void const **hash, u8 *len)
+{
+	const struct public_key_signature *pks;
+	struct modsig_hdr *modsig = (typeof(modsig)) hdr;
+	int i;
+
+	pks = pkcs7_get_message_sig(modsig->pkcs7_msg);
+	if (!pks)
+		return -EBADMSG;
+
+	for (i = 0; i < HASH_ALGO__LAST; i++)
+		if (!strcmp(hash_algo_name[i], pks->hash_algo))
+			break;
+
+	*algo = i;
+	*hash = pks->digest;
+	*len = pks->digest_size;
+
+	return 0;
+}
+
+int ima_modsig_verify(const unsigned int keyring_id,
+		      struct evm_ima_xattr_data *hdr)
+{
+	struct modsig_hdr *modsig = (struct modsig_hdr *) hdr;
+	struct key *trusted_keys = integrity_keyring_from_id(keyring_id);
+
+	if (IS_ERR(trusted_keys))
+		return -EINVAL;
+
+	return verify_pkcs7_message_sig(modsig->data, modsig->data_len,
+					modsig->pkcs7_msg, trusted_keys,
+					VERIFYING_MODULE_SIGNATURE, NULL, NULL);
+}
+
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+	if (!hdr)
+		return;
+
+	if (hdr->type == IMA_MODSIG) {
+		struct modsig_hdr *modsig = (struct modsig_hdr *) hdr;
+
+		pkcs7_free_message(modsig->pkcs7_msg);
+	}
+
+	kfree(hdr);
+}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index da709558ec8b..ce2bd9d5afa5 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -59,6 +59,7 @@ enum evm_ima_xattr_type {
 	EVM_XATTR_HMAC,
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
+	IMA_MODSIG,
 	IMA_XATTR_LAST
 };
 

[PATCH v5 17/18] ima: Implement support for module-style appended signatures

[Thiago Jung Bauermann]

This patch actually implements the appraise_type=modsig option, allowing
IMA to read and verify modsig signatures

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h          |  17 +++--
 security/integrity/ima/ima_appraise.c | 119 ++++++++++++++++++++++++++++++++--
 security/integrity/ima/ima_main.c     |   7 +-
 3 files changed, 128 insertions(+), 15 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index eb58af06566f..b082138461b3 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);
 }
 
 /*
@@ -243,9 +244,10 @@ int ima_policy_show(struct seq_file *m, void *v);
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
-			     struct file *file, const unsigned char *filename,
-			     struct evm_ima_xattr_data *xattr_value,
-			     int xattr_len, int opened);
+			     struct file *file, const void *buf, loff_t size,
+			     const unsigned char *filename,
+			     struct evm_ima_xattr_data **xattr_value,
+			     int *xattr_len, int opened);
 int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func);
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
 enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -270,10 +272,11 @@ void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
-					   struct file *file,
+					   struct file *file, const void *buf,
+					   loff_t size,
 					   const unsigned char *filename,
-					   struct evm_ima_xattr_data *xattr_value,
-					   int xattr_len, int opened)
+					   struct evm_ima_xattr_data **xattr_value,
+					   int *xattr_len, int opened)
 {
 	return INTEGRITY_UNKNOWN;
 }
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 58e147049e98..108690741c1a 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -190,6 +190,45 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+static int appraise_modsig(struct integrity_iint_cache *iint,
+			   struct evm_ima_xattr_data *xattr_value,
+			   int xattr_len)
+{
+	enum hash_algo algo;
+	const void *digest;
+	void *buf;
+	int rc, len;
+	u8 dig_len;
+
+	rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
+	if (rc)
+		return rc;
+
+	/*
+	 * The signature is good. Now let's put the sig hash
+	 * into the iint cache so that it gets stored in the
+	 * measurement list.
+	 */
+
+	rc = ima_get_modsig_hash(xattr_value, &algo, &digest, &dig_len);
+	if (rc)
+		return rc;
+
+	len = sizeof(iint->ima_hash) + dig_len;
+	buf = krealloc(iint->ima_hash, len, GFP_NOFS);
+	if (!buf)
+		return -ENOMEM;
+
+	iint->ima_hash = buf;
+	iint->flags |= IMA_DIGSIG;
+	iint->ima_hash->algo = algo;
+	iint->ima_hash->length = dig_len;
+
+	memcpy(iint->ima_hash->digest, digest, dig_len);
+
+	return 0;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -200,18 +239,28 @@ int ima_read_xattr(struct dentry *dentry,
  */
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
-			     struct file *file, const unsigned char *filename,
-			     struct evm_ima_xattr_data *xattr_value,
-			     int xattr_len, int opened)
+			     struct file *file, const void *buf, loff_t size,
+			     const unsigned char *filename,
+			     struct evm_ima_xattr_data **xattr_value_,
+			     int *xattr_len_, int opened)
 {
 	static const char op[] = "appraise_data";
 	const char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
-	int rc = xattr_len, hash_start = 0;
+	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
+	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
+	bool appraising_modsig = false;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED &&
+	    !ima_read_modsig(func, buf, size, &xattr_value, &xattr_len)) {
+		appraising_modsig = true;
+		rc = xattr_len;
+	}
 
-	if (!(inode->i_opflags & IOP_XATTR))
+	/* If not appraising a modsig, we need an xattr. */
+	if (!appraising_modsig && !(inode->i_opflags & IOP_XATTR))
 		return INTEGRITY_UNKNOWN;
 
 	if (rc <= 0) {
@@ -235,6 +284,9 @@ int ima_appraise_measurement(enum ima_hooks func,
 	case INTEGRITY_UNKNOWN:
 		break;
 	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
+		/* It's fine not to have xattrs when using a modsig. */
+		if (appraising_modsig)
+			break;
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
 		cause = "missing-HMAC";
 		goto out;
@@ -242,6 +294,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 		cause = "invalid-HMAC";
 		goto out;
 	}
+
+ retry:
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST_NG:
 		/* first byte contains algorithm id */
@@ -285,6 +339,61 @@ int ima_appraise_measurement(enum ima_hooks func,
 			status = INTEGRITY_PASS;
 		}
 		break;
+	case IMA_MODSIG:
+		/*
+		 * To avoid being tricked into an infinite loop, we don't allow
+		 * a modsig stored in the xattr.
+		 */
+		if (!appraising_modsig) {
+			status = INTEGRITY_UNKNOWN;
+			cause = "unknown-ima-data";
+			break;
+		}
+
+		rc = appraise_modsig(iint, xattr_value, xattr_len);
+		if (!rc) {
+			kfree(*xattr_value_);
+			*xattr_value_ = xattr_value;
+			*xattr_len_ = xattr_len;
+
+			status = INTEGRITY_PASS;
+			break;
+		}
+
+		ima_free_xattr_data(xattr_value);
+
+		/*
+		 * The appended signature failed verification. If there's a
+		 * signature in the extended attribute, let's fall back to it.
+		 */
+		if (inode->i_opflags & IOP_XATTR && *xattr_len_ != 0 &&
+		    *xattr_len_ != -ENODATA) {
+			const char *modsig_cause = rc == -EOPNOTSUPP ?
+				"unknown" : "invalid-signature";
+
+			/* First, log that the modsig verification failed. */
+			integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
+					    filename, op, modsig_cause, rc, 0);
+
+			xattr_len = rc = *xattr_len_;
+			xattr_value = *xattr_value_;
+			appraising_modsig = false;
+
+			if (rc > 0)
+				/* Process xattr contents. */
+				goto retry;
+
+			/* Unexpected error reading xattr. */
+			status = INTEGRITY_UNKNOWN;
+		} else {
+			if (rc == -EOPNOTSUPP)
+				status = INTEGRITY_UNKNOWN;
+			else {
+				cause = "invalid-signature";
+				status = INTEGRITY_FAIL;
+			}
+		}
+		break;
 	default:
 		status = INTEGRITY_UNKNOWN;
 		cause = "unknown-ima-data";
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8e96450e27f5..6a2d960fbd92 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -243,8 +243,9 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK))
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, opened);
+		rc = ima_appraise_measurement(func, iint, file, buf, size,
+					      pathname, &xattr_value,
+					      &xattr_len, opened);
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, pcr);
@@ -255,7 +256,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
 	     !(iint->flags & IMA_NEW_FILE))
 		rc = -EACCES;
-	kfree(xattr_value);
+	ima_free_xattr_data(xattr_value);
 out_free:
 	if (pathbuf)
 		__putname(pathbuf);

[PATCH v5 18/18] ima: Write modsig to the measurement list

[Thiago Jung Bauermann]

Add modsig support for templates which require the contents of the file
signature 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>
---
 security/integrity/ima/ima.h              |  8 ++++++++
 security/integrity/ima/ima_api.c          |  8 +++++++-
 security/integrity/ima/ima_main.c         | 30 +++++++++++++++++++++++++++++-
 security/integrity/ima/ima_modsig.c       | 20 +++++++++++++++++++-
 security/integrity/ima/ima_template.c     | 12 ++++++++++++
 security/integrity/ima/ima_template_lib.c | 17 +++++++++++++++--
 6 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b082138461b3..68f471666151 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -148,6 +148,7 @@ int ima_init_crypto(void);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
 struct ima_template_desc *ima_template_desc_current(void);
+bool ima_current_template_has_sig(void);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
@@ -264,6 +265,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 		    int *xattr_len);
 int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
 			void const **hash, u8 *len);
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len);
 int ima_modsig_verify(const unsigned int keyring_id,
 		      struct evm_ima_xattr_data *hdr);
 void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
@@ -334,6 +336,12 @@ static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
 	return -ENOTSUPP;
 }
 
+static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data **data,
+					    int *data_len)
+{
+	return -ENOTSUPP;
+}
+
 static inline int ima_modsig_verify(const unsigned int keyring_id,
 				    struct evm_ima_xattr_data *hdr)
 {
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c6d346e9f708..59a5b044b48b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -284,7 +284,13 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
 					    xattr_len, NULL};
 	int violation = 0;
 
-	if (iint->measured_pcrs & (0x1 << pcr))
+	/*
+	 * We still need to store the measurement in the case of MODSIG because
+	 * we only have its contents to put in the list at the time of
+	 * appraisal. See comment in process_measurement for more details.
+	 */
+	if ((iint->measured_pcrs & (0x1 << pcr)) &&
+	    (!xattr_value || xattr_value->type != IMA_MODSIG))
 		return;
 
 	result = ima_alloc_init_template(&event_data, &entry);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6a2d960fbd92..0d3390de7432 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,7 +246,35 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		rc = ima_appraise_measurement(func, iint, file, buf, size,
 					      pathname, &xattr_value,
 					      &xattr_len, opened);
-	if (action & IMA_MEASURE)
+
+	/*
+	 * MODSIG has one corner case we need to deal with here:
+	 *
+	 * Suppose the policy has one measure rule for one hook and an appraise
+	 * rule for a different hook. Suppose also that the template requires
+	 * the signature to be stored in the measurement list.
+	 *
+	 * If a file containing a MODSIG is measured by the first hook before
+	 * being appraised by the second one, the corresponding entry in the
+	 * measurement list will not contain the MODSIG because we only fetch it
+	 * for IMA_APPRAISAL. We don't fetch it earlier because if the file has
+	 * both a DIGSIG and a MODSIG it's not possible to know which one will
+	 * be valid without actually doing the appraisal.
+	 *
+	 * Therefore, after appraisal of a MODSIG signature we need to store the
+	 * measurement again if the current template requires storing the
+	 * signature.
+	 *
+	 * With the opposite ordering (the appraise rule triggering before the
+	 * measurement rule) there is the same problem but it's not possible to
+	 * do anything about it because at the time we are appraising the
+	 * signature it's impossible to know whether a measurement will ever
+	 * need to be stored for this file.
+	 */
+	if ((action & IMA_MEASURE) || ((iint->flags & IMA_MEASURE) &&
+				       xattr_value &&
+				       xattr_value->type == IMA_MODSIG &&
+				       ima_current_template_has_sig()))
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, pcr);
 	if (action & IMA_AUDIT)
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index 2786aa97060e..82c3e2693982 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -27,6 +27,10 @@ struct modsig_hdr {
 	const void *data;	/* Pointer to data covered by pkcs7_msg. */
 	size_t data_len;
 	struct pkcs7_message *pkcs7_msg;
+	int raw_pkcs7_len;
+
+	/* This will be in the measurement list if required by the template. */
+	struct evm_ima_xattr_data raw_pkcs7;
 };
 
 /**
@@ -86,7 +90,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 	sig_len = be32_to_cpu(sig->sig_len);
 	buf_len -= sig_len + sizeof(*sig);
 
-	hdr = kmalloc(sizeof(*hdr), GFP_KERNEL);
+	hdr = kmalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
 	if (!hdr)
 		return -ENOMEM;
 
@@ -97,6 +101,10 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 		return rc;
 	}
 
+	memcpy(hdr->raw_pkcs7.data, buf + buf_len, sig_len);
+	hdr->raw_pkcs7_len = sig_len + 1;
+	hdr->raw_pkcs7.type = IMA_MODSIG;
+
 	hdr->type = IMA_MODSIG;
 	hdr->data = buf;
 	hdr->data_len = buf_len;
@@ -129,6 +137,16 @@ int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
 	return 0;
 }
 
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len)
+{
+	struct modsig_hdr *modsig = (struct modsig_hdr *) *data;
+
+	*data = &modsig->raw_pkcs7;
+	*data_len = modsig->raw_pkcs7_len;
+
+	return 0;
+}
+
 int ima_modsig_verify(const unsigned int keyring_id,
 		      struct evm_ima_xattr_data *hdr)
 {
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 3cc1d2763fd2..a5bad6996334 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -228,6 +228,18 @@ struct ima_template_desc *ima_template_desc_current(void)
 	return ima_template;
 }
 
+bool ima_current_template_has_sig(void)
+{
+	struct ima_template_desc *template = ima_template_desc_current();
+	int i;
+
+	for (i = 0; i < template->num_fields; i++)
+		if (!strcmp(template->fields[i]->field_id, "sig"))
+			return true;
+
+	return false;
+}
+
 int __init ima_init_template(void)
 {
 	struct ima_template_desc *template = ima_template_desc_current();
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index e8ec783b6a8d..d0e85def0ae9 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -379,10 +379,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 (!xattr_value || xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+	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);
 }

Re: [PATCH v5 18/18] ima: Write modsig to the measurement list

[Mimi Zohar]

On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6a2d960fbd92..0d3390de7432 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -246,7 +246,35 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  		rc = ima_appraise_measurement(func, iint, file, buf, size,
>  					      pathname, &xattr_value,
>  					      &xattr_len, opened);
> -	if (action & IMA_MEASURE)
> +
> +	/*
> +	 * MODSIG has one corner case we need to deal with here:
> +	 *
> +	 * Suppose the policy has one measure rule for one hook and an appraise
> +	 * rule for a different hook. Suppose also that the template requires
> +	 * the signature to be stored in the measurement list.
> +	 *
> +	 * If a file containing a MODSIG is measured by the first hook before
> +	 * being appraised by the second one, the corresponding entry in the
> +	 * measurement list will not contain the MODSIG because we only fetch it
> +	 * for IMA_APPRAISAL. We don't fetch it earlier because if the file has
> +	 * both a DIGSIG and a MODSIG it's not possible to know which one will
> +	 * be valid without actually doing the appraisal.
> +	 *
> +	 * Therefore, after appraisal of a MODSIG signature we need to store the
> +	 * measurement again if the current template requires storing the
> +	 * signature.

Yes, all true, but this long comment doesn't belong here in the middle
of process_measurement(). 

> +	 * With the opposite ordering (the appraise rule triggering before the
> +	 * measurement rule) there is the same problem but it's not possible to
> +	 * do anything about it because at the time we are appraising the
> +	 * signature it's impossible to know whether a measurement will ever
> +	 * need to be stored for this file.
> +	 */

With the template format "ima-sig", the verified file signature needs
to be included in the measurement list.  Based on this file signature,
the attestation server can validate the signature.

In this case, where the appraisal comes first followed by the
measurement, the appraised file signature is included in the
measurement list.  I don't see the problem here.

> +	if ((action & IMA_MEASURE) || ((iint->flags & IMA_MEASURE) &&
> +				       xattr_value &&
> +				       xattr_value->type == IMA_MODSIG &&
> +				       ima_current_template_has_sig()))

Like the clean up you did elsewhere, this new set of tests should be
made into a function.  The comment could placed along with the new
function.

Mimi

>  		ima_store_measurement(iint, file, pathname,
>  				      xattr_value, xattr_len, pcr);
>  	if (action & IMA_AUDIT)
> 

Re: [PATCH v5 12/18] MODSIGN: Export module signature definitions

[Mimi Zohar]

On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
> 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 validate_module_signature without having to depend on
> CONFIG_MODULE_SIG.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>

Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

One minor comment below...

> ---
>  include/linux/module.h           |  3 --
>  include/linux/module_signature.h | 47 +++++++++++++++++++++++++
>  init/Kconfig                     |  6 +++-
>  kernel/Makefile                  |  2 +-
>  kernel/module.c                  |  1 +
>  kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
>  6 files changed, 85 insertions(+), 48 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index fe5aa3736707..108874af9838 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -23,9 +23,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..e80728e5b86c
> --- /dev/null
> +++ b/include/linux/module_signature.h
> @@ -0,0 +1,47 @@
> +/* Module signature handling.
> + *
> + * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#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 validate_module_sig(const struct module_signature *ms, size_t file_len);
> +int mod_verify_sig(const void *mod, unsigned long *_modlen);
> +
> +#endif /* _LINUX_MODULE_SIGNATURE_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 78cb2461012e..230e9f3aedaf 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1748,7 +1748,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
> @@ -1763,6 +1763,10 @@ config MODULE_SIG
>  	  debuginfo strip done by some packagers (such as rpmbuild) and
>  	  inclusion into an initramfs that wants the module size reduced.
> 
> +config MODULE_SIG_FORMAT
> +	def_bool n
> +	select SYSTEM_DATA_VERIFICATION
> +
>  config MODULE_SIG_FORCE
>  	bool "Require modules to be validly signed"
>  	depends on MODULE_SIG
> diff --git a/kernel/Makefile b/kernel/Makefile
> index ed470aac53da..20fd6d76232e 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -57,7 +57,7 @@ obj-y += up.o
>  endif
>  obj-$(CONFIG_UID16) += uid16.o
>  obj-$(CONFIG_MODULES) += module.o
> -obj-$(CONFIG_MODULE_SIG) += module_signing.o
> +obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.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 de66ec825992..a259e9df570d 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_signing.c b/kernel/module_signing.c
> index 937c844bee4a..204c60d4cc9f 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -11,36 +11,38 @@
> 
>  #include <linux/kernel.h>
>  #include <linux/errno.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:
> +/**
> + * validate_module_sig - validate that the given signature is sane
>   *
> - *	- Signer's name
> - *	- Key identifier
> - *	- Signature data
> - *	- Information block
> + * @ms:		Signature to validate.
> + * @file_len:	Size of the file to which @ms is appended.
>   */
> -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 validate_module_sig(const struct module_signature *ms, size_t file_len)
> +{
> +	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
> +		return -EBADMSG;
> +	else if (ms->id_type != PKEY_ID_PKCS7) {
> +		pr_err("Module is not signed with expected PKCS#7 message\n");
> +		return -ENOPKG;
> +	} else 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("PKCS#7 signature info has unexpected non-zero params\n");
> +		return -EBADMSG;
> +	}
> +

When moving code from one place to another, it's easier to review when
there aren't code changes as well.  In this case, the original code
doesn't have "else clauses".  Here some of the if/then/else clauses
have braces others don't.  There shouldn't be a mixture.

Mimi

> +	return 0;
> +}
> 
>  /*
>   * Verify the signature on a module.
> @@ -49,6 +51,7 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
>  {
>  	struct module_signature ms;
>  	size_t modlen = *_modlen, sig_len;
> +	int ret;
> 
>  	pr_devel("==>%s(,%zu)\n", __func__, modlen);
> 
> @@ -56,30 +59,15 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
>  		return -EBADMSG;
> 
>  	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
> -	modlen -= sizeof(ms);
> +
> +	ret = validate_module_sig(&ms, modlen);
> +	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);
>  	*_modlen = modlen;
> 
> -	if (ms.id_type != PKEY_ID_PKCS7) {
> -		pr_err("Module is not signed with expected PKCS#7 message\n");
> -		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("PKCS#7 signature info has unexpected non-zero params\n");
> -		return -EBADMSG;
> -	}
> -
>  	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
>  				      NULL, VERIFYING_MODULE_SIGNATURE,
>  				      NULL, NULL);
> 

Re: [PATCH v5 13/18] PKCS#7: Introduce pkcs7_get_message_sig and verify_pkcs7_message_sig

[Mimi Zohar]

On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
> IMA will need to access the digest used in the signature so that it can
> verify files containing module-style appended signatures. For this purpose,
> add function pkcs7_get_message_sig.
> 
> It will also need to verify an already parsed PKCS#7 message. For this
> purpose, add function verify_pkcs7_message_signature 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>

Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

> ---
>  certs/system_keyring.c                | 60 +++++++++++++++++++++++++----------
>  crypto/asymmetric_keys/pkcs7_parser.c | 12 +++++++
>  include/crypto/pkcs7.h                |  2 ++
>  include/linux/verification.h          | 10 ++++++
>  4 files changed, 68 insertions(+), 16 deletions(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b27f0c..6a8684959780 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -190,33 +190,26 @@ 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 +251,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 af4cd8649117..e41beda297a8 100644
> --- a/crypto/asymmetric_keys/pkcs7_parser.c
> +++ b/crypto/asymmetric_keys/pkcs7_parser.c
> @@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
>  		return -ENOMEM;
>  	return 0;
>  }
> +
> +/**
> + * pkcs7_get_message_sig - get signature in @pkcs7
> + *
> + * This function doesn't meaningfully support messages with more than one
> + * signature. It will always return the first signature.
> + */
> +const struct public_key_signature *pkcs7_get_message_sig(
> +					const struct pkcs7_message *pkcs7)
> +{
> +	return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL;
> +}
> 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,
> 

Re: [PATCH v5 00/18] Appended signatures support for IMA appraisal

[Mimi Zohar]

On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
> Hello,
> 
> The main highlight in this version is that it fixes a bug where the modsig
> wasn't being included in the measurement list if the appraised file was
> already measured by another rule. The fix is in the last patch.
> 
> Another change is that the last patch in the v4 series ("ima: Support
> module-style appended signatures for appraisal") has been broken up into
> smaller patches. I may have overdone it...
> 
> Finally, I have added some patches removing superfluous parentheses from
> expressions. IMO these patches make it easier (and more pleasant) to read
> the code, and thus easier to understand it. Since I'm not sure how welcome
> the changes are, I split them in 3 "levels" in increasing potential for
> conflict with patches from other people (they can be squashed together when
> applied):
> 
> 1. patch 2 contains the bare minimum, changing only lines that are also
>    touched by other patches in the series;
> 
> 2. patch 3 cleans up all the files that are touched by this patch series;
> 
> 3. patch 4 cleans up all other EVM and IMA files that weren't already fixed
>    by the previous patches.
> 
> If unwanted, patches 3 and 4 can be simply skipped without affecting the
> rest of the patches. I have already rebased them from v4.13-rc2 to
> v4.14-rc3 and now to linux-integrity/next with very few easy to resolve
> conflicts, so I think they are worth keeping.
> 
> These patches apply on top of today's linux-integrity/next.

This cover letter and the patch descriptions are well written,
explaining what and why you're making this change.  The problem is
that I don't agree that fewer parentheses makes the code more
readable.  When you repost the patches (for other reasons), please
don't include these changes.

thanks,

Mimi

Re: [PATCH v5 18/18] ima: Write modsig to the measurement list

[Thiago Jung Bauermann]

Hello Mimi,

Thanks for your review.

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 6a2d960fbd92..0d3390de7432 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -246,7 +246,35 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>  		rc = ima_appraise_measurement(func, iint, file, buf, size,
>>  					      pathname, &xattr_value,
>>  					      &xattr_len, opened);
>> -	if (action & IMA_MEASURE)
>> +
>> +	/*
>> +	 * MODSIG has one corner case we need to deal with here:
>> +	 *
>> +	 * Suppose the policy has one measure rule for one hook and an appraise
>> +	 * rule for a different hook. Suppose also that the template requires
>> +	 * the signature to be stored in the measurement list.
>> +	 *
>> +	 * If a file containing a MODSIG is measured by the first hook before
>> +	 * being appraised by the second one, the corresponding entry in the
>> +	 * measurement list will not contain the MODSIG because we only fetch it
>> +	 * for IMA_APPRAISAL. We don't fetch it earlier because if the file has
>> +	 * both a DIGSIG and a MODSIG it's not possible to know which one will
>> +	 * be valid without actually doing the appraisal.
>> +	 *
>> +	 * Therefore, after appraisal of a MODSIG signature we need to store the
>> +	 * measurement again if the current template requires storing the
>> +	 * signature.
>
> Yes, all true, but this long comment doesn't belong here in the middle
> of process_measurement().
>
>> +	 * With the opposite ordering (the appraise rule triggering before the
>> +	 * measurement rule) there is the same problem but it's not possible to
>> +	 * do anything about it because at the time we are appraising the
>> +	 * signature it's impossible to know whether a measurement will ever
>> +	 * need to be stored for this file.
>> +	 */
>
> With the template format "ima-sig", the verified file signature needs
> to be included in the measurement list. Based on this file signature,
> the attestation server can validate the signature.
>
> In this case, where the appraisal comes first followed by the
> measurement, the appraised file signature is included in the
> measurement list. I don't see the problem here.

I think I forgot that during appraisal the modsig is copied into the
iint cache and that it will be used when the measure rule is trigerred.
I'll drop that last paragraph.

>
>> +	if ((action & IMA_MEASURE) || ((iint->flags & IMA_MEASURE) &&
>> +				       xattr_value &&
>> +				       xattr_value->type == IMA_MODSIG &&
>> +				       ima_current_template_has_sig()))
>
> Like the clean up you did elsewhere, this new set of tests should be
> made into a function. The comment could placed along with the new
> function.

Ok. I didn't create a function because these tests are only done here,
but I agree that it will make the code clearer, and be a better place
for the big comment as well. Will do in the next version.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v5 12/18] MODSIGN: Export module signature definitions

[Thiago Jung Bauermann]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
>> 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 validate_module_signature without having to depend on
>> CONFIG_MODULE_SIG.
>> 
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
>
> Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>
> One minor comment below...

Thanks!

>> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
>> index 937c844bee4a..204c60d4cc9f 100644
>> --- a/kernel/module_signing.c
>> +++ b/kernel/module_signing.c
>> @@ -11,36 +11,38 @@
>> 
>>  #include <linux/kernel.h>
>>  #include <linux/errno.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:
>> +/**
>> + * validate_module_sig - validate that the given signature is sane
>>   *
>> - *	- Signer's name
>> - *	- Key identifier
>> - *	- Signature data
>> - *	- Information block
>> + * @ms:		Signature to validate.
>> + * @file_len:	Size of the file to which @ms is appended.
>>   */
>> -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 validate_module_sig(const struct module_signature *ms, size_t file_len)
>> +{
>> +	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
>> +		return -EBADMSG;
>> +	else if (ms->id_type != PKEY_ID_PKCS7) {
>> +		pr_err("Module is not signed with expected PKCS#7 message\n");
>> +		return -ENOPKG;
>> +	} else 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("PKCS#7 signature info has unexpected non-zero params\n");
>> +		return -EBADMSG;
>> +	}
>> +
>
> When moving code from one place to another, it's easier to review when
> there aren't code changes as well. In this case, the original code
> doesn't have "else clauses".

Indeed. I changed the code back to using separate if clauses, making
only the changes that are required for the refactoring.

> Here some of the if/then/else clauses
> have braces others don't. There shouldn't be a mixture.

Does this still apply when the if clauses are separate as in the
original code? Should the first if still have braces?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v5 12/18] MODSIGN: Export module signature definitions

[Mimi Zohar]

On Thu, 2017-10-26 at 20:47 -0200, Thiago Jung Bauermann wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:
> >> 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 validate_module_signature without having to depend on
> >> CONFIG_MODULE_SIG.
> >> 
> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> >
> > Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >
> > One minor comment below...
> 
> Thanks!
> 
> >> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> >> index 937c844bee4a..204c60d4cc9f 100644
> >> --- a/kernel/module_signing.c
> >> +++ b/kernel/module_signing.c
> >> @@ -11,36 +11,38 @@
> >> 
> >>  #include <linux/kernel.h>
> >>  #include <linux/errno.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:
> >> +/**
> >> + * validate_module_sig - validate that the given signature is sane
> >>   *
> >> - *	- Signer's name
> >> - *	- Key identifier
> >> - *	- Signature data
> >> - *	- Information block
> >> + * @ms:		Signature to validate.
> >> + * @file_len:	Size of the file to which @ms is appended.
> >>   */
> >> -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 validate_module_sig(const struct module_signature *ms, size_t file_len)
> >> +{
> >> +	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
> >> +		return -EBADMSG;
> >> +	else if (ms->id_type != PKEY_ID_PKCS7) {
> >> +		pr_err("Module is not signed with expected PKCS#7 message\n");
> >> +		return -ENOPKG;
> >> +	} else 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("PKCS#7 signature info has unexpected non-zero params\n");
> >> +		return -EBADMSG;
> >> +	}
> >> +
> >
> > When moving code from one place to another, it's easier to review when
> > there aren't code changes as well. In this case, the original code
> > doesn't have "else clauses".
> 
> Indeed. I changed the code back to using separate if clauses, making
> only the changes that are required for the refactoring.
> 
> > Here some of the if/then/else clauses
> > have braces others don't. There shouldn't be a mixture.
> 
> Does this still apply when the if clauses are separate as in the
> original code? Should the first if still have braces?

No, the original code was fine. 

Re: [PATCH v5 17/18] ima: Implement support for module-style appended signatures

[Mimi Zohar]

On Tue, 2017-10-17 at 22:53 -0200, Thiago Jung Bauermann wrote:

Below are a few additional comments.

> @@ -200,18 +239,28 @@ int ima_read_xattr(struct dentry *dentry,
>   */
>  int ima_appraise_measurement(enum ima_hooks func,
>  			     struct integrity_iint_cache *iint,
> -			     struct file *file, const unsigned char *filename,
> -			     struct evm_ima_xattr_data *xattr_value,
> -			     int xattr_len, int opened)
> +			     struct file *file, const void *buf, loff_t size,
> +			     const unsigned char *filename,
> +			     struct evm_ima_xattr_data **xattr_value_,
> +			     int *xattr_len_, int opened)
>  {
>  	static const char op[] = "appraise_data";
>  	const char *cause = "unknown";
>  	struct dentry *dentry = file_dentry(file);
>  	struct inode *inode = d_backing_inode(dentry);
>  	enum integrity_status status = INTEGRITY_UNKNOWN;
> -	int rc = xattr_len, hash_start = 0;
> +	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
> +	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
> +	bool appraising_modsig = false;
> +
> +	if (iint->flags & IMA_MODSIG_ALLOWED &&
> +	    !ima_read_modsig(func, buf, size, &xattr_value, &xattr_len)) {
> +		appraising_modsig = true;
> +		rc = xattr_len;
> +	}
> 
> -	if (!(inode->i_opflags & IOP_XATTR))
> +	/* If not appraising a modsig, we need an xattr. */
> +	if (!appraising_modsig && !(inode->i_opflags & IOP_XATTR))
>  		return INTEGRITY_UNKNOWN;
> 
>  	if (rc <= 0) {
> @@ -235,6 +284,9 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	case INTEGRITY_UNKNOWN:
>  		break;
>  	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
> +		/* It's fine not to have xattrs when using a modsig. */
> +		if (appraising_modsig)
> +			break;
>  	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
>  		cause = "missing-HMAC";
>  		goto out;
> @@ -242,6 +294,8 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		cause = "invalid-HMAC";
>  		goto out;
>  	}
> +
> + retry:
>  	switch (xattr_value->type) {
>  	case IMA_XATTR_DIGEST_NG:
>  		/* first byte contains algorithm id */
> @@ -285,6 +339,61 @@ int ima_appraise_measurement(enum ima_hooks func,
>  			status = INTEGRITY_PASS;
>  		}
>  		break;
> +	case IMA_MODSIG:
> +		/*
> +		 * To avoid being tricked into an infinite loop, we don't allow
> +		 * a modsig stored in the xattr.
> +		 */
> +		if (!appraising_modsig) {
> +			status = INTEGRITY_UNKNOWN;
> +			cause = "unknown-ima-data";
> +			break;
> +		}
> +		rc = appraise_modsig(iint, xattr_value, xattr_len);
> +		if (!rc) {
> +			kfree(*xattr_value_);
> +			*xattr_value_ = xattr_value;
> +			*xattr_len_ = xattr_len;
> +
> +			status = INTEGRITY_PASS;
> +			break;
> +		}
> +
> +		ima_free_xattr_data(xattr_value);
> +
> +		/*
> +		 * The appended signature failed verification. If there's a
> +		 * signature in the extended attribute, let's fall back to it.
> +		 */
> +		if (inode->i_opflags & IOP_XATTR && *xattr_len_ != 0 &&
> +		    *xattr_len_ != -ENODATA) {

At this point, there was an appended signature verification failure.
 If there isn't an xattr, for whatever reason, shouldn't we be
returning "invalid_signature" and "INTEGRITY_FAIL".  If so, then the
above test could be simplified to check whether there is any data,
like this:

	if ((inode->i_opflags & IOP_XATTR) && (*xattr_len_ > 0)) {

> +			const char *modsig_cause = rc == -EOPNOTSUPP ?
> +				"unknown" : "invalid-signature";

This can then be cleaned up as well.

> +
> +			/* First, log that the modsig verification failed. */
> +			integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> +					    filename, op, modsig_cause, rc, 0);

I'm not sure that we want to audit intermediary signature verification
failures.  Perhaps this audit message should be considered
"additional", meaning it is only emitted if the "integrity_audit" boot
command line option is enabled.  Change the last field to 1 to
indicate it is an "additional" audit message.

> +
> +			xattr_len = rc = *xattr_len_;
> +			xattr_value = *xattr_value_;
> +			appraising_modsig = false;
> +
> +			if (rc > 0)

This test becomes redundant.

> +				/* Process xattr contents. */
> +				goto retry;
> +
> +			/* Unexpected error reading xattr. */
> +			status = INTEGRITY_UNKNOWN;
> +		} else {
> +			if (rc == -EOPNOTSUPP)
> +				status = INTEGRITY_UNKNOWN;
> +			else {
> +				cause = "invalid-signature";
> +				status = INTEGRITY_FAIL;
> +			}
> +		}
> +		break;

I think the rest can be simplified to:
	cause = "invalid-signature";
	status = INTEGRITY_FAIL;

Mimi