fsverity: rework fsverity_get_digest() again

Commit Message

Eric Biggers June 12, 2023, 7 p.m. UTC

From: Eric Biggers <[e-mail]>

Address several issues with the calling convention and documentation of
fsverity_get_digest():

- Make it provide the hash algorithm as either a FS_VERITY_HASH_ALG_*
  value or HASH_ALGO_* value, at the caller's choice, rather than only a
  HASH_ALGO_* value as it did before.  This allows callers to work with
  the fsverity native algorithm numbers if they want to.  HASH_ALGO_* is
  what IMA uses, but other users (e.g. overlayfs) should use
  FS_VERITY_HASH_ALG_* to match fsverity-utils and the fsverity UAPI.

- Make it return the digest size so that it doesn't need to be looked up
  separately.  Use the return value for this, since 0 works nicely for
  the "file doesn't have fsverity enabled" case.  This also makes it
  clear that no other errors are possible.

- Rename the 'digest' parameter to 'raw_digest' and clearly document
  that it is only useful in combination with the algorithm ID.  This
  hopefully clears up a point of confusion.

- Export it to modules, since overlayfs will need it for checking the
  fsverity digests of lowerdata files
  (https://lore.kernel.org/r/[e-mail]).

Signed-off-by: Eric Biggers <[e-mail]>
---
 fs/verity/measure.c              | 37 ++++++++++++++++++++++----------
 include/linux/fsverity.h         | 14 +++++++-----
 security/integrity/ima/ima_api.c | 31 +++++++++++---------------
 3 files changed, 47 insertions(+), 35 deletions(-)


base-commit: 13e2408d02dd12a3b46bf8a29b3ae4f6119fc520

Comments

Mimi Zohar June 14, 2023, 1:30 p.m. UTC | #1

On Mon, 2023-06-12 at 12:00 -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> Address several issues with the calling convention and documentation of
> fsverity_get_digest():
> 
> - Make it provide the hash algorithm as either a FS_VERITY_HASH_ALG_*
>   value or HASH_ALGO_* value, at the caller's choice, rather than only a
>   HASH_ALGO_* value as it did before.  This allows callers to work with
>   the fsverity native algorithm numbers if they want to.  HASH_ALGO_* is
>   what IMA uses, but other users (e.g. overlayfs) should use
>   FS_VERITY_HASH_ALG_* to match fsverity-utils and the fsverity UAPI.
> 
> - Make it return the digest size so that it doesn't need to be looked up
>   separately.  Use the return value for this, since 0 works nicely for
>   the "file doesn't have fsverity enabled" case.  This also makes it
>   clear that no other errors are possible.
> 
> - Rename the 'digest' parameter to 'raw_digest' and clearly document
>   that it is only useful in combination with the algorithm ID.  This
>   hopefully clears up a point of confusion.
> 
> - Export it to modules, since overlayfs will need it for checking the
>   fsverity digests of lowerdata files
>   (https://lore.kernel.org/r/[e-mail]).
> 
> Signed-off-by: Eric Biggers <[e-mail]>

Acked-by: Mimi Zohar <[e-mail]> for the IMA piece.

Eric Biggers June 14, 2023, 5:48 p.m. UTC | #2

On Mon, Jun 12, 2023 at 12:00:47PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> Address several issues with the calling convention and documentation of
> fsverity_get_digest():
> 
> - Make it provide the hash algorithm as either a FS_VERITY_HASH_ALG_*
>   value or HASH_ALGO_* value, at the caller's choice, rather than only a
>   HASH_ALGO_* value as it did before.  This allows callers to work with
>   the fsverity native algorithm numbers if they want to.  HASH_ALGO_* is
>   what IMA uses, but other users (e.g. overlayfs) should use
>   FS_VERITY_HASH_ALG_* to match fsverity-utils and the fsverity UAPI.
> 
> - Make it return the digest size so that it doesn't need to be looked up
>   separately.  Use the return value for this, since 0 works nicely for
>   the "file doesn't have fsverity enabled" case.  This also makes it
>   clear that no other errors are possible.
> 
> - Rename the 'digest' parameter to 'raw_digest' and clearly document
>   that it is only useful in combination with the algorithm ID.  This
>   hopefully clears up a point of confusion.
> 
> - Export it to modules, since overlayfs will need it for checking the
>   fsverity digests of lowerdata files
>   (https://lore.kernel.org/r/[e-mail]).
> 
> Signed-off-by: Eric Biggers <[e-mail]>
> ---
>  fs/verity/measure.c              | 37 ++++++++++++++++++++++----------
>  include/linux/fsverity.h         | 14 +++++++-----
>  security/integrity/ima/ima_api.c | 31 +++++++++++---------------
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 

Applied to https://git.kernel.org/pub/scm/fs/fsverity/linux.git/log/?h=for-next

- Eric

Patch

diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index 5c79ea1b24687..eec5956141da2 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -61,27 +61,42 @@  EXPORT_SYMBOL_GPL(fsverity_ioctl_measure);
 /**
  * fsverity_get_digest() - get a verity file's digest
  * @inode: inode to get digest of
- * @digest: (out) pointer to the digest
- * @alg: (out) pointer to the hash algorithm enumeration
+ * @raw_digest: (out) the raw file digest
+ * @alg: (out) the digest's algorithm, as a FS_VERITY_HASH_ALG_* value
+ * @halg: (out) the digest's algorithm, as a HASH_ALGO_* value
  *
- * Return the file hash algorithm and digest of an fsverity protected file.
- * Assumption: before calling this, the file must have been opened.
+ * Retrieves the fsverity digest of the given file.  The file must have been
+ * opened at least once since the inode was last loaded into the inode cache;
+ * otherwise this function will not recognize when fsverity is enabled.
  *
- * Return: 0 on success, -errno on failure
+ * The file's fsverity digest consists of @raw_digest in combination with either
+ * @alg or @halg.  (The caller can choose which one of @alg or @halg to use.)
+ *
+ * IMPORTANT: Callers *must* make use of one of the two algorithm IDs, since
+ * @raw_digest is meaningless without knowing which algorithm it uses!  fsverity
+ * provides no security guarantee for users who ignore the algorithm ID, even if
+ * they use the digest size (since algorithms can share the same digest size).
+ *
+ * Return: The size of the raw digest in bytes, or 0 if the file doesn't have
+ *	   fsverity enabled.
  */
 int fsverity_get_digest(struct inode *inode,
-			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
-			enum hash_algo *alg)
+			u8 raw_digest[FS_VERITY_MAX_DIGEST_SIZE],
+			u8 *alg, enum hash_algo *halg)
 {
 	const struct fsverity_info *vi;
 	const struct fsverity_hash_alg *hash_alg;
 
 	vi = fsverity_get_info(inode);
 	if (!vi)
-		return -ENODATA; /* not a verity file */
+		return 0; /* not a verity file */
 
 	hash_alg = vi->tree_params.hash_alg;
-	memcpy(digest, vi->file_digest, hash_alg->digest_size);
-	*alg = hash_alg->algo_id;
-	return 0;
+	memcpy(raw_digest, vi->file_digest, hash_alg->digest_size);
+	if (alg)
+		*alg = hash_alg - fsverity_hash_algs;
+	if (halg)
+		*halg = hash_alg->algo_id;
+	return hash_alg->digest_size;
 }
+EXPORT_SYMBOL_GPL(fsverity_get_digest);
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index e76605d5b36ee..1eb7eae580be7 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -143,8 +143,8 @@  int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
 
 int fsverity_ioctl_measure(struct file *filp, void __user *arg);
 int fsverity_get_digest(struct inode *inode,
-			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
-			enum hash_algo *alg);
+			u8 raw_digest[FS_VERITY_MAX_DIGEST_SIZE],
+			u8 *alg, enum hash_algo *halg);
 
 /* open.c */
 
@@ -197,10 +197,14 @@  static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
 }
 
 static inline int fsverity_get_digest(struct inode *inode,
-				      u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
-				      enum hash_algo *alg)
+				      u8 raw_digest[FS_VERITY_MAX_DIGEST_SIZE],
+				      u8 *alg, enum hash_algo *halg)
 {
-	return -EOPNOTSUPP;
+	/*
+	 * fsverity is not enabled in the kernel configuration, so always report
+	 * that the file doesn't have fsverity enabled (digest size 0).
+	 */
+	return 0;
 }
 
 /* open.c */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index d3662f4acadc1..ce541b0ee1d30 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -202,19 +202,19 @@  int ima_get_action(struct mnt_idmap *idmap, struct inode *inode,
 				allowed_algos);
 }
 
-static int ima_get_verity_digest(struct integrity_iint_cache *iint,
-				 struct ima_max_digest_data *hash)
+static bool ima_get_verity_digest(struct integrity_iint_cache *iint,
+				  struct ima_max_digest_data *hash)
 {
-	enum hash_algo verity_alg;
-	int ret;
+	enum hash_algo alg;
+	int digest_len;
 
 	/*
 	 * On failure, 'measure' policy rules will result in a file data
 	 * hash containing 0's.
 	 */
-	ret = fsverity_get_digest(iint->inode, hash->digest, &verity_alg);
-	if (ret)
-		return ret;
+	digest_len = fsverity_get_digest(iint->inode, hash->digest, NULL, &alg);
+	if (digest_len == 0)
+		return false;
 
 	/*
 	 * Unlike in the case of actually calculating the file hash, in
@@ -223,9 +223,9 @@  static int ima_get_verity_digest(struct integrity_iint_cache *iint,
 	 * mismatch between the verity algorithm and the xattr signature
 	 * algorithm, if one exists, will be detected later.
 	 */
-	hash->hdr.algo = verity_alg;
-	hash->hdr.length = hash_digest_size[verity_alg];
-	return 0;
+	hash->hdr.algo = alg;
+	hash->hdr.length = digest_len;
+	return true;
 }
 
 /*
@@ -276,16 +276,9 @@  int ima_collect_measurement(struct integrity_iint_cache *iint,
 	memset(&hash.digest, 0, sizeof(hash.digest));
 
 	if (iint->flags & IMA_VERITY_REQUIRED) {
-		result = ima_get_verity_digest(iint, &hash);
-		switch (result) {
-		case 0:
-			break;
-		case -ENODATA:
+		if (!ima_get_verity_digest(iint, &hash)) {
 			audit_cause = "no-verity-digest";
-			break;
-		default:
-			audit_cause = "invalid-verity-digest";
-			break;
+			result = -ENODATA;
 		}
 	} else if (buf) {
 		result = ima_calc_buffer_hash(buf, size, &hash.hdr);