On Oct  4, 2023 Fan Wu [off-list ref] wrote:

Allows author of IPE policy to indicate trust for a singular dm-verity
volume, identified by roothash, through "dmverity_roothash" and all
signed dm-verity volumes, through "dmverity_signature".

Signed-off-by: Deven Bowers <redacted>
Signed-off-by: Fan Wu <redacted>
---
v2:
  + No Changes

v3:
  + No changes

v4:
  + No changes

v5:
  + No changes

v6:
  + Fix an improper cleanup that can result in
    a leak

v7:
  + Squash patch 08/12, 10/12 to [11/16]

v8:
  + Undo squash of 08/12, 10/12 - separating drivers/md/ from security/
    & block/
  + Use common-audit function for dmverity_signature.
  + Change implementation for storing the dm-verity digest to use the
    newly introduced dm_verity_digest structure introduced in patch
    14/20.

v9:
  + Adapt to the new parser

v10:
  + Select the Kconfig when all dependencies are enabled

v11:
  + No changes
---
 security/ipe/Kconfig         |  18 +++++
 security/ipe/Makefile        |   1 +
 security/ipe/audit.c         |  31 +++++++-
 security/ipe/digest.c        | 142 +++++++++++++++++++++++++++++++++++
 security/ipe/digest.h        |  26 +++++++
 security/ipe/eval.c          | 101 ++++++++++++++++++++++++-
 security/ipe/eval.h          |  13 ++++
 security/ipe/hooks.c         |  51 +++++++++++++
 security/ipe/hooks.h         |   8 ++
 security/ipe/ipe.c           |  15 ++++
 security/ipe/ipe.h           |   4 +
 security/ipe/policy.h        |   3 +
 security/ipe/policy_parser.c |  24 +++++-
 13 files changed, 433 insertions(+), 4 deletions(-)
 create mode 100644 security/ipe/digest.c
 create mode 100644 security/ipe/digest.h

...

Since the "dmverity_roothash=" field name is going to be written to
the audit record regardless of CONFIG_IPE_PROP_DM_VERITY we should
ensure that it has a value of "?" instead of nothing.  To fix that
I would suggest something like this:

  #else
  static void audit_dmv_roothash(struct audit_buffer *ab, const void *rh)
  {
    audit_log_format(ab, "?");
  }
  #endif /* CONFIG_IPE_PROP_DM_VERITY */

If you wanted to avoid the roothash change above, you could change the
code here so it didn't always write "dmverity_roothash=" into the audit
record.

Why is @value void?  You should make it a digest_info type or simply
skip the second parameter and return a digest_info pointer.

+{
+	char *sep, *raw_digest;
+	size_t raw_digest_len;
+	int rc = 0;
+	u8 *digest = NULL;
+	struct digest_info *info = NULL;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	sep = strchr(valstr, ':');
+	if (!sep) {
+		rc = -EBADMSG;
+		goto err;
+	}
+
+	info->alg = kstrndup(valstr, sep - valstr, GFP_KERNEL);
+	if (!info->alg) {
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	raw_digest = sep + 1;
+	raw_digest_len = strlen(raw_digest);
+	info->raw_digest = kstrndup(raw_digest, raw_digest_len, GFP_KERNEL);

Since you're running a strlen() over @raw_digest, you might as well
just use kstrdup() instead of kstrndup().

+	if (!info->raw_digest) {
+		rc = -ENOMEM;
+		goto err_free_alg;
+	}
+
+	info->digest_len = (raw_digest_len + 1) / 2;
+	digest = kzalloc(info->digest_len, GFP_KERNEL);
+	if (!digest) {
+		rc = -ENOMEM;
+		goto err_free_raw;
+	}
+
+	rc = hex2bin(digest, raw_digest, info->digest_len);
+	if (rc < 0) {
+		rc = -EINVAL;
+		goto err_free_raw;
+	}
+
+	info->digest = digest;
+	*value = info;
+	return 0;
+
+err_free_raw:
+	kfree(info->raw_digest);
+err_free_alg:
+	kfree(info->alg);
+err:
+	kfree(digest);
+	kfree(info);
+	return rc;
+}
+
+/**
+ * ipe_digest_eval - evaluate an IPE digest against another digest.
+ * @expect: Supplies the policy-provided digest value.
+ * @digest: Supplies the digest to compare against the policy digest value.
+ * @digest_len: The length of @digest.
+ * @alg: Supplies the name of the algorithm used to calculated @digest.
+ *
+ * Return:
+ * * true	- digests match
+ * * false	- digests do not match
+ */
+bool ipe_digest_eval(const void *expect, const u8 *digest, size_t digest_len,
+		     const char *alg)

Similar to the above, why not make the @expect parameter a digest_info
type?  Also, why not pass a second digest_info parameter instead of
a separate @digest, @digest_len, and @alg?

+{
+	const struct digest_info *info = (struct digest_info *)expect;
+
+	return (digest_len == info->digest_len) && !strcmp(alg, info->alg) &&
+	       (!memcmp(info->digest, digest, info->digest_len));
+}
+
+/**
+ * ipe_digest_free - free an IPE digest.
+ * @value: Supplies a pointer the policy-provided digest value to free.
+ */
+void ipe_digest_free(void **value)

Another digest_info parameter/type issue.

+{
+	struct digest_info *info = (struct digest_info *)(*value);
+
+	if (IS_ERR_OR_NULL(info))
+		return;
+
+	kfree(info->alg);
+	kfree(info->raw_digest);
+	kfree(info->digest);
+	kfree(info);
+}
+
+/**
+ * ipe_digest_audit - audit a digest that was sourced from IPE's policy.
+ * @ab: Supplies the audit_buffer to append the formatted result.
+ * @val: Supplies a pointer to source the audit record from.
+ *
+ * Digests in IPE are defined in a standard way:
+ *	<alg_name>:<hex>
+ *
+ * Use this function to create a property to audit the digest
+ * consistently.
+ *
+ * Return:
+ * 0 - OK
+ * !0 - Error
+ */
+void ipe_digest_audit(struct audit_buffer *ab, const void *val)

Another digest_info parameter/type issue.

+{
+	const struct digest_info *info = (struct digest_info *)val;
+
+	audit_log_untrustedstring(ab, info->alg);
+	audit_log_format(ab, ":");
+	audit_log_untrustedstring(ab, info->raw_digest);
+}

...

You don't need @ino.

  build_ipe_bdev_ctx(ctx, d_real_inode(file->f_path.dentry));

+	}
+}
+
+#ifdef CONFIG_IPE_PROP_DM_VERITY
+/**
+ * evaluate_dmv_roothash - Evaluate @ctx against a dmv roothash property.
+ * @ctx: Supplies a pointer to the context being evaluated.
+ * @p: Supplies a pointer to the property being evaluated.
+ *
+ * Return:
+ * * true	- The current @ctx match the @p
+ * * false	- The current @ctx doesn't match the @p
+ */
+static bool evaluate_dmv_roothash(const struct ipe_eval_ctx *const ctx,
+				  struct ipe_prop *p)
+{
+	return !!ctx->ipe_bdev &&
+	       ipe_digest_eval(p->value,
+			       ctx->ipe_bdev->digest,
+			       ctx->ipe_bdev->digest_len,
+			       ctx->ipe_bdev->digest_algo);

Building on my comments above in ipe_digest_eval(), if you convert it
to use digest_info structs this is simplified:

  ipe_digest_eval(p->vlaue, ctx->ipe_bdev)

+}
+
+/**
+ * evaluate_dmv_sig_false: Analyze @ctx against a dmv sig false property.
+ * @ctx: Supplies a pointer to the context being evaluated.
+ * @p: Supplies a pointer to the property being evaluated.
+ *
+ * Return:
+ * * true	- The current @ctx match the @p
+ * * false	- The current @ctx doesn't match the @p
+ */
+static bool evaluate_dmv_sig_false(const struct ipe_eval_ctx *const ctx,
+				   struct ipe_prop *p)
+{
+	return !ctx->ipe_bdev || (!ctx->ipe_bdev->dm_verity_signed);
+}
+
+/**
+ * evaluate_dmv_sig_true: Analyze @ctx against a dmv sig true property.
+ * @ctx: Supplies a pointer to the context being evaluated.
+ * @p: Supplies a pointer to the property being evaluated.
+ *
+ * Return:
+ * * true	- The current @ctx match the @p
+ * * false	- The current @ctx doesn't match the @p
+ */
+static bool evaluate_dmv_sig_true(const struct ipe_eval_ctx *const ctx,
+				  struct ipe_prop *p)
+{
+	return ctx->ipe_bdev && (!!ctx->ipe_bdev->dm_verity_signed);
+}

Do you need both evaluate_dmv_sig_true() and evaluate_dmv_sig_false()?
If yes, you should make one call the other and return the inverse to
help ensure there are no oddities.

You need to cleanup @blob on error so you don't leak ipe_bdev::digest.

+		blob->digest_len = digest->digest_len;
+		return 0;
+	} else if (!strcmp(key, DM_VERITY_SIGNATURE_SEC_NAME)) {
+		blob->dm_verity_signed = true;
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_IPE_PROP_DM_VERITY */

--
paul-moore.com