linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] fs-verity: add an ioctl to read verity metadata
@ 2021-01-15 18:18 Eric Biggers
  2021-01-15 18:18 ` [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor() Eric Biggers
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

[This patchset applies to v5.11-rc3]

Add an ioctl FS_IOC_READ_VERITY_METADATA which allows reading verity
metadata from a file that has fs-verity enabled, including:

- The Merkle tree
- The fsverity_descriptor (not including the signature if present)
- The built-in signature, if present

This ioctl has similar semantics to pread().  It is passed the type of
metadata to read (one of the above three), and a buffer, offset, and
size.  It returns the number of bytes read or an error.

This ioctl doesn't make any assumption about where the metadata is
stored on-disk.  It does assume the metadata is in a stable format, but
that's basically already the case:

- The Merkle tree and fsverity_descriptor are defined by how fs-verity
  file digests are computed; see the "File digest computation" section
  of Documentation/filesystems/fsverity.rst.  Technically, the way in
  which the levels of the tree are ordered relative to each other wasn't
  previously specified, but it's logical to put the root level first.

- The built-in signature is the value passed to FS_IOC_ENABLE_VERITY.

This ioctl is useful because it allows writing a server program that
takes a verity file and serves it to a client program, such that the
client can do its own fs-verity compatible verification of the file.
This only makes sense if the client doesn't trust the server and if the
server needs to provide the storage for the client.

More concretely, there is interest in using this ability in Android to
export APK files (which are protected by fs-verity) to "protected VMs".
This would use Protected KVM (https://lwn.net/Articles/836693), which
provides an isolated execution environment without having to trust the
traditional "host".  A "guest" VM can boot from a signed image and
perform specific tasks in a minimum trusted environment using files that
have fs-verity enabled on the host, without trusting the host or
requiring that the guest has its own trusted storage.

Technically, it would be possible to duplicate the metadata and store it
in separate files for serving.  However, that would be less efficient
and would require extra care in userspace to maintain file consistency.

In addition to the above, the ability to read the built-in signatures is
useful because it allows a system that is using the in-kernel signature
verification to migrate to userspace signature verification.

This patchset has been tested by new xfstests which call this new ioctl
via a new subcommand for the 'fsverity' program from fsverity-utils.

Eric Biggers (6):
  fs-verity: factor out fsverity_get_descriptor()
  fs-verity: don't pass whole descriptor to fsverity_verify_signature()
  fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  fs-verity: support reading Merkle tree with ioctl
  fs-verity: support reading descriptor with ioctl
  fs-verity: support reading signature with ioctl

 Documentation/filesystems/fsverity.rst |  76 ++++++++++
 fs/ext4/ioctl.c                        |   7 +
 fs/f2fs/file.c                         |  11 ++
 fs/verity/Makefile                     |   1 +
 fs/verity/fsverity_private.h           |  13 +-
 fs/verity/open.c                       | 133 +++++++++++------
 fs/verity/read_metadata.c              | 195 +++++++++++++++++++++++++
 fs/verity/signature.c                  |  20 +--
 include/linux/fsverity.h               |  12 ++
 include/uapi/linux/fsverity.h          |  14 ++
 10 files changed, 417 insertions(+), 65 deletions(-)
 create mode 100644 fs/verity/read_metadata.c


base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
-- 
2.30.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor()
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:04   ` Jaegeuk Kim
  2021-01-15 18:18 ` [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature() Eric Biggers
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

The FS_IOC_READ_VERITY_METADATA ioctl will need to return the fs-verity
descriptor (and signature) to userspace.

There are a few ways we could implement this:

- Save a copy of the descriptor (and signature) in the fsverity_info
  struct that hangs off of the in-memory inode.  However, this would
  waste memory since most of the time it wouldn't be needed.

- Regenerate the descriptor from the merkle_tree_params in the
  fsverity_info.  However, this wouldn't work for the signature, nor for
  the salt which the merkle_tree_params only contains indirectly as part
  of the 'hashstate'.  It would also be error-prone.

- Just get them from the filesystem again.  The disadvantage is that in
  general we can't trust that they haven't been maliciously changed
  since the file has opened.  However, the use cases for
  FS_IOC_READ_VERITY_METADATA don't require that it verifies the chain
  of trust.  So this is okay as long as we do some basic validation.

In preparation for implementing the third option, factor out a helper
function fsverity_get_descriptor() which gets the descriptor (and
appended signature) from the filesystem and does some basic validation.

As part of this, start checking the sig_size field for overflow.
Currently fsverity_verify_signature() does this.  But the new ioctl will
need this too, so do it earlier.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/fsverity_private.h |   7 +-
 fs/verity/open.c             | 130 +++++++++++++++++++++++------------
 2 files changed, 91 insertions(+), 46 deletions(-)

diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index 6413d28664d6d..6c9caccc06021 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -122,12 +122,17 @@ int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
 				     const u8 *salt, size_t salt_size);
 
 struct fsverity_info *fsverity_create_info(const struct inode *inode,
-					   void *desc, size_t desc_size);
+					   struct fsverity_descriptor *desc,
+					   size_t desc_size);
 
 void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
 
 void fsverity_free_info(struct fsverity_info *vi);
 
+int fsverity_get_descriptor(struct inode *inode,
+			    struct fsverity_descriptor **desc_ret,
+			    size_t *desc_size_ret);
+
 int __init fsverity_init_info_cache(void);
 void __init fsverity_exit_info_cache(void);
 
diff --git a/fs/verity/open.c b/fs/verity/open.c
index 228d0eca3e2e5..a987bb785e9b0 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -142,45 +142,17 @@ static int compute_file_digest(struct fsverity_hash_alg *hash_alg,
 }
 
 /*
- * Validate the given fsverity_descriptor and create a new fsverity_info from
- * it.  The signature (if present) is also checked.
+ * Create a new fsverity_info from the given fsverity_descriptor (with optional
+ * appended signature), and check the signature if present.  The
+ * fsverity_descriptor must have already undergone basic validation.
  */
 struct fsverity_info *fsverity_create_info(const struct inode *inode,
-					   void *_desc, size_t desc_size)
+					   struct fsverity_descriptor *desc,
+					   size_t desc_size)
 {
-	struct fsverity_descriptor *desc = _desc;
 	struct fsverity_info *vi;
 	int err;
 
-	if (desc_size < sizeof(*desc)) {
-		fsverity_err(inode, "Unrecognized descriptor size: %zu bytes",
-			     desc_size);
-		return ERR_PTR(-EINVAL);
-	}
-
-	if (desc->version != 1) {
-		fsverity_err(inode, "Unrecognized descriptor version: %u",
-			     desc->version);
-		return ERR_PTR(-EINVAL);
-	}
-
-	if (memchr_inv(desc->__reserved, 0, sizeof(desc->__reserved))) {
-		fsverity_err(inode, "Reserved bits set in descriptor");
-		return ERR_PTR(-EINVAL);
-	}
-
-	if (desc->salt_size > sizeof(desc->salt)) {
-		fsverity_err(inode, "Invalid salt_size: %u", desc->salt_size);
-		return ERR_PTR(-EINVAL);
-	}
-
-	if (le64_to_cpu(desc->data_size) != inode->i_size) {
-		fsverity_err(inode,
-			     "Wrong data_size: %llu (desc) != %lld (inode)",
-			     le64_to_cpu(desc->data_size), inode->i_size);
-		return ERR_PTR(-EINVAL);
-	}
-
 	vi = kmem_cache_zalloc(fsverity_info_cachep, GFP_KERNEL);
 	if (!vi)
 		return ERR_PTR(-ENOMEM);
@@ -245,15 +217,57 @@ void fsverity_free_info(struct fsverity_info *vi)
 	kmem_cache_free(fsverity_info_cachep, vi);
 }
 
-/* Ensure the inode has an ->i_verity_info */
-static int ensure_verity_info(struct inode *inode)
+static bool validate_fsverity_descriptor(struct inode *inode,
+					 const struct fsverity_descriptor *desc,
+					 size_t desc_size)
 {
-	struct fsverity_info *vi = fsverity_get_info(inode);
-	struct fsverity_descriptor *desc;
-	int res;
+	if (desc_size < sizeof(*desc)) {
+		fsverity_err(inode, "Unrecognized descriptor size: %zu bytes",
+			     desc_size);
+		return false;
+	}
 
-	if (vi)
-		return 0;
+	if (desc->version != 1) {
+		fsverity_err(inode, "Unrecognized descriptor version: %u",
+			     desc->version);
+		return false;
+	}
+
+	if (memchr_inv(desc->__reserved, 0, sizeof(desc->__reserved))) {
+		fsverity_err(inode, "Reserved bits set in descriptor");
+		return false;
+	}
+
+	if (desc->salt_size > sizeof(desc->salt)) {
+		fsverity_err(inode, "Invalid salt_size: %u", desc->salt_size);
+		return false;
+	}
+
+	if (le64_to_cpu(desc->data_size) != inode->i_size) {
+		fsverity_err(inode,
+			     "Wrong data_size: %llu (desc) != %lld (inode)",
+			     le64_to_cpu(desc->data_size), inode->i_size);
+		return false;
+	}
+
+	if (le32_to_cpu(desc->sig_size) > desc_size - sizeof(*desc)) {
+		fsverity_err(inode, "Signature overflows verity descriptor");
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Read the inode's fsverity_descriptor (with optional appended signature) from
+ * the filesystem, and do basic validation of it.
+ */
+int fsverity_get_descriptor(struct inode *inode,
+			    struct fsverity_descriptor **desc_ret,
+			    size_t *desc_size_ret)
+{
+	int res;
+	struct fsverity_descriptor *desc;
 
 	res = inode->i_sb->s_vop->get_verity_descriptor(inode, NULL, 0);
 	if (res < 0) {
@@ -272,20 +286,46 @@ static int ensure_verity_info(struct inode *inode)
 	res = inode->i_sb->s_vop->get_verity_descriptor(inode, desc, res);
 	if (res < 0) {
 		fsverity_err(inode, "Error %d reading verity descriptor", res);
-		goto out_free_desc;
+		kfree(desc);
+		return res;
+	}
+
+	if (!validate_fsverity_descriptor(inode, desc, res)) {
+		kfree(desc);
+		return -EINVAL;
 	}
 
-	vi = fsverity_create_info(inode, desc, res);
+	*desc_ret = desc;
+	*desc_size_ret = res;
+	return 0;
+}
+
+/* Ensure the inode has an ->i_verity_info */
+static int ensure_verity_info(struct inode *inode)
+{
+	struct fsverity_info *vi = fsverity_get_info(inode);
+	struct fsverity_descriptor *desc;
+	size_t desc_size;
+	int err;
+
+	if (vi)
+		return 0;
+
+	err = fsverity_get_descriptor(inode, &desc, &desc_size);
+	if (err)
+		return err;
+
+	vi = fsverity_create_info(inode, desc, desc_size);
 	if (IS_ERR(vi)) {
-		res = PTR_ERR(vi);
+		err = PTR_ERR(vi);
 		goto out_free_desc;
 	}
 
 	fsverity_set_info(inode, vi);
-	res = 0;
+	err = 0;
 out_free_desc:
 	kfree(desc);
-	return res;
+	return err;
 }
 
 /**
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature()
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
  2021-01-15 18:18 ` [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor() Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:04   ` Jaegeuk Kim
  2021-01-28  3:24   ` Amy Parker
  2021-01-15 18:18 ` [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl Eric Biggers
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

Now that fsverity_get_descriptor() validates the sig_size field,
fsverity_verify_signature() doesn't need to do it.

Just change the prototype of fsverity_verify_signature() to take the
signature directly rather than take a fsverity_descriptor.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/fsverity_private.h |  6 ++----
 fs/verity/open.c             |  3 ++-
 fs/verity/signature.c        | 20 ++++++--------------
 3 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index 6c9caccc06021..a7920434bae50 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -140,15 +140,13 @@ void __init fsverity_exit_info_cache(void);
 
 #ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
 int fsverity_verify_signature(const struct fsverity_info *vi,
-			      const struct fsverity_descriptor *desc,
-			      size_t desc_size);
+			      const u8 *signature, size_t sig_size);
 
 int __init fsverity_init_signature(void);
 #else /* !CONFIG_FS_VERITY_BUILTIN_SIGNATURES */
 static inline int
 fsverity_verify_signature(const struct fsverity_info *vi,
-			  const struct fsverity_descriptor *desc,
-			  size_t desc_size)
+			  const u8 *signature, size_t sig_size)
 {
 	return 0;
 }
diff --git a/fs/verity/open.c b/fs/verity/open.c
index a987bb785e9b0..60ff8af7219fe 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -181,7 +181,8 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
 		 vi->tree_params.hash_alg->name,
 		 vi->tree_params.digest_size, vi->file_digest);
 
-	err = fsverity_verify_signature(vi, desc, desc_size);
+	err = fsverity_verify_signature(vi, desc->signature,
+					le32_to_cpu(desc->sig_size));
 out:
 	if (err) {
 		fsverity_free_info(vi);
diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index 012468eda2a78..143a530a80088 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -29,21 +29,19 @@ static struct key *fsverity_keyring;
 /**
  * fsverity_verify_signature() - check a verity file's signature
  * @vi: the file's fsverity_info
- * @desc: the file's fsverity_descriptor
- * @desc_size: size of @desc
+ * @signature: the file's built-in signature
+ * @sig_size: size of signature in bytes, or 0 if no signature
  *
- * If the file's fs-verity descriptor includes a signature of the file digest,
- * verify it against the certificates in the fs-verity keyring.
+ * If the file includes a signature of its fs-verity file digest, verify it
+ * against the certificates in the fs-verity keyring.
  *
  * Return: 0 on success (signature valid or not required); -errno on failure
  */
 int fsverity_verify_signature(const struct fsverity_info *vi,
-			      const struct fsverity_descriptor *desc,
-			      size_t desc_size)
+			      const u8 *signature, size_t sig_size)
 {
 	const struct inode *inode = vi->inode;
 	const struct fsverity_hash_alg *hash_alg = vi->tree_params.hash_alg;
-	const u32 sig_size = le32_to_cpu(desc->sig_size);
 	struct fsverity_formatted_digest *d;
 	int err;
 
@@ -56,11 +54,6 @@ int fsverity_verify_signature(const struct fsverity_info *vi,
 		return 0;
 	}
 
-	if (sig_size > desc_size - sizeof(*desc)) {
-		fsverity_err(inode, "Signature overflows verity descriptor");
-		return -EBADMSG;
-	}
-
 	d = kzalloc(sizeof(*d) + hash_alg->digest_size, GFP_KERNEL);
 	if (!d)
 		return -ENOMEM;
@@ -70,8 +63,7 @@ int fsverity_verify_signature(const struct fsverity_info *vi,
 	memcpy(d->digest, vi->file_digest, hash_alg->digest_size);
 
 	err = verify_pkcs7_signature(d, sizeof(*d) + hash_alg->digest_size,
-				     desc->signature, sig_size,
-				     fsverity_keyring,
+				     signature, sig_size, fsverity_keyring,
 				     VERIFYING_UNSPECIFIED_SIGNATURE,
 				     NULL, NULL);
 	kfree(d);
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
  2021-01-15 18:18 ` [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor() Eric Biggers
  2021-01-15 18:18 ` [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature() Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:03   ` Jaegeuk Kim
  2021-02-07  7:46   ` [f2fs-dev] " Chao Yu
  2021-01-15 18:18 ` [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl Eric Biggers
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

Add an ioctl FS_IOC_READ_VERITY_METADATA which will allow reading verity
metadata from a file that has fs-verity enabled, including:

- The Merkle tree
- The fsverity_descriptor (not including the signature if present)
- The built-in signature, if present

This ioctl has similar semantics to pread().  It is passed the type of
metadata to read (one of the above three), and a buffer, offset, and
size.  It returns the number of bytes read or an error.

Separate patches will add support for each of the above metadata types.
This patch just adds the ioctl itself.

This ioctl doesn't make any assumption about where the metadata is
stored on-disk.  It does assume the metadata is in a stable format, but
that's basically already the case:

- The Merkle tree and fsverity_descriptor are defined by how fs-verity
  file digests are computed; see the "File digest computation" section
  of Documentation/filesystems/fsverity.rst.  Technically, the way in
  which the levels of the tree are ordered relative to each other wasn't
  previously specified, but it's logical to put the root level first.

- The built-in signature is the value passed to FS_IOC_ENABLE_VERITY.

This ioctl is useful because it allows writing a server program that
takes a verity file and serves it to a client program, such that the
client can do its own fs-verity compatible verification of the file.
This only makes sense if the client doesn't trust the server and if the
server needs to provide the storage for the client.

More concretely, there is interest in using this ability in Android to
export APK files (which are protected by fs-verity) to "protected VMs".
This would use Protected KVM (https://lwn.net/Articles/836693), which
provides an isolated execution environment without having to trust the
traditional "host".  A "guest" VM can boot from a signed image and
perform specific tasks in a minimum trusted environment using files that
have fs-verity enabled on the host, without trusting the host or
requiring that the guest has its own trusted storage.

Technically, it would be possible to duplicate the metadata and store it
in separate files for serving.  However, that would be less efficient
and would require extra care in userspace to maintain file consistency.

In addition to the above, the ability to read the built-in signatures is
useful because it allows a system that is using the in-kernel signature
verification to migrate to userspace signature verification.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fsverity.rst | 57 ++++++++++++++++++++++++++
 fs/ext4/ioctl.c                        |  7 ++++
 fs/f2fs/file.c                         | 11 +++++
 fs/verity/Makefile                     |  1 +
 fs/verity/read_metadata.c              | 55 +++++++++++++++++++++++++
 include/linux/fsverity.h               | 12 ++++++
 include/uapi/linux/fsverity.h          | 10 +++++
 7 files changed, 153 insertions(+)
 create mode 100644 fs/verity/read_metadata.c

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index e0204a23e997e..9ef7a7de60085 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -217,6 +217,63 @@ FS_IOC_MEASURE_VERITY can fail with the following errors:
 - ``EOVERFLOW``: the digest is longer than the specified
   ``digest_size`` bytes.  Try providing a larger buffer.
 
+FS_IOC_READ_VERITY_METADATA
+---------------------------
+
+The FS_IOC_READ_VERITY_METADATA ioctl reads verity metadata from a
+verity file.  This ioctl is available since Linux v5.12.
+
+This ioctl allows writing a server program that takes a verity file
+and serves it to a client program, such that the client can do its own
+fs-verity compatible verification of the file.  This only makes sense
+if the client doesn't trust the server and if the server needs to
+provide the storage for the client.
+
+This is a fairly specialized use case, and most fs-verity users won't
+need this ioctl.
+
+This ioctl takes in a pointer to the following structure::
+
+   struct fsverity_read_metadata_arg {
+           __u64 metadata_type;
+           __u64 offset;
+           __u64 length;
+           __u64 buf_ptr;
+           __u64 __reserved;
+   };
+
+``metadata_type`` specifies the type of metadata to read.
+
+The semantics are similar to those of ``pread()``.  ``offset``
+specifies the offset in bytes into the metadata item to read from, and
+``length`` specifies the maximum number of bytes to read from the
+metadata item.  ``buf_ptr`` is the pointer to the buffer to read into,
+cast to a 64-bit integer.  ``__reserved`` must be 0.  On success, the
+number of bytes read is returned.  0 is returned at the end of the
+metadata item.  The returned length may be less than ``length``, for
+example if the ioctl is interrupted.
+
+The metadata returned by FS_IOC_READ_VERITY_METADATA isn't guaranteed
+to be authenticated against the file digest that would be returned by
+`FS_IOC_MEASURE_VERITY`_, as the metadata is expected to be used to
+implement fs-verity compatible verification anyway (though absent a
+malicious disk, the metadata will indeed match).  E.g. to implement
+this ioctl, the filesystem is allowed to just read the Merkle tree
+blocks from disk without actually verifying the path to the root node.
+
+FS_IOC_READ_VERITY_METADATA can fail with the following errors:
+
+- ``EFAULT``: the caller provided inaccessible memory
+- ``EINTR``: the ioctl was interrupted before any data was read
+- ``EINVAL``: reserved fields were set, or ``offset + length``
+  overflowed
+- ``ENODATA``: the file is not a verity file
+- ``ENOTTY``: this type of filesystem does not implement fs-verity, or
+  this ioctl is not yet implemented on it
+- ``EOPNOTSUPP``: the kernel was not configured with fs-verity
+  support, or the filesystem superblock has not had the 'verity'
+  feature enabled on it.  (See `Filesystem support`_.)
+
 FS_IOC_GETFLAGS
 ---------------
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 524e134324475..56dc58adba8ac 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1306,6 +1306,12 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			return -EOPNOTSUPP;
 		return fsverity_ioctl_measure(filp, (void __user *)arg);
 
+	case FS_IOC_READ_VERITY_METADATA:
+		if (!ext4_has_feature_verity(sb))
+			return -EOPNOTSUPP;
+		return fsverity_ioctl_read_metadata(filp,
+						    (const void __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
@@ -1388,6 +1394,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case FS_IOC_GETFSMAP:
 	case FS_IOC_ENABLE_VERITY:
 	case FS_IOC_MEASURE_VERITY:
+	case FS_IOC_READ_VERITY_METADATA:
 	case EXT4_IOC_CLEAR_ES_CACHE:
 	case EXT4_IOC_GETSTATE:
 	case EXT4_IOC_GET_ES_CACHE:
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f585545277d77..d0aefb5b97fac 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3357,6 +3357,14 @@ static int f2fs_ioc_measure_verity(struct file *filp, unsigned long arg)
 	return fsverity_ioctl_measure(filp, (void __user *)arg);
 }
 
+static int f2fs_ioc_read_verity_metadata(struct file *filp, unsigned long arg)
+{
+	if (!f2fs_sb_has_verity(F2FS_I_SB(file_inode(filp))))
+		return -EOPNOTSUPP;
+
+	return fsverity_ioctl_read_metadata(filp, (const void __user *)arg);
+}
+
 static int f2fs_ioc_getfslabel(struct file *filp, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -4272,6 +4280,8 @@ static long __f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return f2fs_ioc_enable_verity(filp, arg);
 	case FS_IOC_MEASURE_VERITY:
 		return f2fs_ioc_measure_verity(filp, arg);
+	case FS_IOC_READ_VERITY_METADATA:
+		return f2fs_ioc_read_verity_metadata(filp, arg);
 	case FS_IOC_GETFSLABEL:
 		return f2fs_ioc_getfslabel(filp, arg);
 	case FS_IOC_SETFSLABEL:
@@ -4523,6 +4533,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F2FS_IOC_RESIZE_FS:
 	case FS_IOC_ENABLE_VERITY:
 	case FS_IOC_MEASURE_VERITY:
+	case FS_IOC_READ_VERITY_METADATA:
 	case FS_IOC_GETFSLABEL:
 	case FS_IOC_SETFSLABEL:
 	case F2FS_IOC_GET_COMPRESS_BLOCKS:
diff --git a/fs/verity/Makefile b/fs/verity/Makefile
index 570e9136334d4..435559a4fa9ea 100644
--- a/fs/verity/Makefile
+++ b/fs/verity/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_FS_VERITY) += enable.o \
 			   init.o \
 			   measure.o \
 			   open.o \
+			   read_metadata.o \
 			   verify.o
 
 obj-$(CONFIG_FS_VERITY_BUILTIN_SIGNATURES) += signature.o
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
new file mode 100644
index 0000000000000..43be990fd53e4
--- /dev/null
+++ b/fs/verity/read_metadata.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Ioctl to read verity metadata
+ *
+ * Copyright 2021 Google LLC
+ */
+
+#include "fsverity_private.h"
+
+#include <linux/uaccess.h>
+
+/**
+ * fsverity_ioctl_read_metadata() - read verity metadata from a file
+ * @filp: file to read the metadata from
+ * @uarg: user pointer to fsverity_read_metadata_arg
+ *
+ * Return: length read on success, 0 on EOF, -errno on failure
+ */
+int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
+{
+	struct inode *inode = file_inode(filp);
+	const struct fsverity_info *vi;
+	struct fsverity_read_metadata_arg arg;
+	int length;
+	void __user *buf;
+
+	vi = fsverity_get_info(inode);
+	if (!vi)
+		return -ENODATA; /* not a verity file */
+	/*
+	 * Note that we don't have to explicitly check that the file is open for
+	 * reading, since verity files can only be opened for reading.
+	 */
+
+	if (copy_from_user(&arg, uarg, sizeof(arg)))
+		return -EFAULT;
+
+	if (arg.__reserved)
+		return -EINVAL;
+
+	/* offset + length must not overflow. */
+	if (arg.offset + arg.length < arg.offset)
+		return -EINVAL;
+
+	/* Ensure that the return value will fit in INT_MAX. */
+	length = min_t(u64, arg.length, INT_MAX);
+
+	buf = u64_to_user_ptr(arg.buf_ptr);
+
+	switch (arg.metadata_type) {
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(fsverity_ioctl_read_metadata);
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index c1144a4503920..b568b3c7d095e 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -138,6 +138,10 @@ int fsverity_file_open(struct inode *inode, struct file *filp);
 int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 void fsverity_cleanup_inode(struct inode *inode);
 
+/* read_metadata.c */
+
+int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg);
+
 /* verify.c */
 
 bool fsverity_verify_page(struct page *page);
@@ -183,6 +187,14 @@ static inline void fsverity_cleanup_inode(struct inode *inode)
 {
 }
 
+/* read_metadata.c */
+
+static inline int fsverity_ioctl_read_metadata(struct file *filp,
+					       const void __user *uarg)
+{
+	return -EOPNOTSUPP;
+}
+
 /* verify.c */
 
 static inline bool fsverity_verify_page(struct page *page)
diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
index 33f44156f8ea5..e062751294d01 100644
--- a/include/uapi/linux/fsverity.h
+++ b/include/uapi/linux/fsverity.h
@@ -83,7 +83,17 @@ struct fsverity_formatted_digest {
 	__u8 digest[];
 };
 
+struct fsverity_read_metadata_arg {
+	__u64 metadata_type;
+	__u64 offset;
+	__u64 length;
+	__u64 buf_ptr;
+	__u64 __reserved;
+};
+
 #define FS_IOC_ENABLE_VERITY	_IOW('f', 133, struct fsverity_enable_arg)
 #define FS_IOC_MEASURE_VERITY	_IOWR('f', 134, struct fsverity_digest)
+#define FS_IOC_READ_VERITY_METADATA \
+	_IOWR('f', 135, struct fsverity_read_metadata_arg)
 
 #endif /* _UAPI_LINUX_FSVERITY_H */
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
                   ` (2 preceding siblings ...)
  2021-01-15 18:18 ` [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:10   ` Jaegeuk Kim
  2021-01-15 18:18 ` [PATCH 5/6] fs-verity: support reading descriptor " Eric Biggers
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

Add support for FS_VERITY_METADATA_TYPE_MERKLE_TREE to
FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
retrieve the Merkle tree of a verity file for serving to a client which
implements fs-verity compatible verification.  See the patch which
introduced FS_IOC_READ_VERITY_METADATA for more details.

This has been tested using a new xfstest which calls this ioctl via a
new subcommand for the 'fsverity' program from fsverity-utils.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fsverity.rst | 10 +++-
 fs/verity/read_metadata.c              | 70 ++++++++++++++++++++++++++
 include/uapi/linux/fsverity.h          |  2 +
 3 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 9ef7a7de60085..50b47a6d9ea11 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -234,6 +234,8 @@ need this ioctl.
 
 This ioctl takes in a pointer to the following structure::
 
+   #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
+
    struct fsverity_read_metadata_arg {
            __u64 metadata_type;
            __u64 offset;
@@ -242,7 +244,13 @@ This ioctl takes in a pointer to the following structure::
            __u64 __reserved;
    };
 
-``metadata_type`` specifies the type of metadata to read.
+``metadata_type`` specifies the type of metadata to read:
+
+- ``FS_VERITY_METADATA_TYPE_MERKLE_TREE`` reads the blocks of the
+  Merkle tree.  The blocks are returned in order from the root level
+  to the leaf level.  Within each level, the blocks are returned in
+  the same order that their hashes are themselves hashed.
+  See `Merkle tree`_ for more information.
 
 The semantics are similar to those of ``pread()``.  ``offset``
 specifies the offset in bytes into the metadata item to read from, and
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 43be990fd53e4..0f8ad2991cf90 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -7,8 +7,75 @@
 
 #include "fsverity_private.h"
 
+#include <linux/backing-dev.h>
+#include <linux/highmem.h>
+#include <linux/sched/signal.h>
 #include <linux/uaccess.h>
 
+static int fsverity_read_merkle_tree(struct inode *inode,
+				     const struct fsverity_info *vi,
+				     void __user *buf, u64 offset, int length)
+{
+	const struct fsverity_operations *vops = inode->i_sb->s_vop;
+	u64 end_offset;
+	unsigned int offs_in_page;
+	pgoff_t index, last_index;
+	int retval = 0;
+	int err = 0;
+
+	end_offset = min(offset + length, vi->tree_params.tree_size);
+	if (offset >= end_offset)
+		return 0;
+	offs_in_page = offset_in_page(offset);
+	last_index = (end_offset - 1) >> PAGE_SHIFT;
+
+	/*
+	 * Iterate through each Merkle tree page in the requested range and copy
+	 * the requested portion to userspace.  Note that the Merkle tree block
+	 * size isn't important here, as we are returning a byte stream; i.e.,
+	 * we can just work with pages even if the tree block size != PAGE_SIZE.
+	 */
+	for (index = offset >> PAGE_SHIFT; index <= last_index; index++) {
+		unsigned long num_ra_pages =
+			min_t(unsigned long, last_index - index + 1,
+			      inode->i_sb->s_bdi->io_pages);
+		unsigned int bytes_to_copy = min_t(u64, end_offset - offset,
+						   PAGE_SIZE - offs_in_page);
+		struct page *page;
+		const void *virt;
+
+		page = vops->read_merkle_tree_page(inode, index, num_ra_pages);
+		if (IS_ERR(page)) {
+			err = PTR_ERR(page);
+			fsverity_err(inode,
+				     "Error %d reading Merkle tree page %lu",
+				     err, index);
+			break;
+		}
+
+		virt = kmap(page);
+		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) {
+			kunmap(page);
+			put_page(page);
+			err = -EFAULT;
+			break;
+		}
+		kunmap(page);
+		put_page(page);
+
+		retval += bytes_to_copy;
+		buf += bytes_to_copy;
+		offset += bytes_to_copy;
+
+		if (fatal_signal_pending(current))  {
+			err = -EINTR;
+			break;
+		}
+		cond_resched();
+		offs_in_page = 0;
+	}
+	return retval ? retval : err;
+}
 /**
  * fsverity_ioctl_read_metadata() - read verity metadata from a file
  * @filp: file to read the metadata from
@@ -48,6 +115,9 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
 	buf = u64_to_user_ptr(arg.buf_ptr);
 
 	switch (arg.metadata_type) {
+	case FS_VERITY_METADATA_TYPE_MERKLE_TREE:
+		return fsverity_read_merkle_tree(inode, vi, buf, arg.offset,
+						 length);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
index e062751294d01..94003b153cb3d 100644
--- a/include/uapi/linux/fsverity.h
+++ b/include/uapi/linux/fsverity.h
@@ -83,6 +83,8 @@ struct fsverity_formatted_digest {
 	__u8 digest[];
 };
 
+#define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
+
 struct fsverity_read_metadata_arg {
 	__u64 metadata_type;
 	__u64 offset;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/6] fs-verity: support reading descriptor with ioctl
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
                   ` (3 preceding siblings ...)
  2021-01-15 18:18 ` [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:11   ` Jaegeuk Kim
  2021-01-15 18:18 ` [PATCH 6/6] fs-verity: support reading signature " Eric Biggers
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

Add support for FS_VERITY_METADATA_TYPE_DESCRIPTOR to
FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
retrieve the fs-verity descriptor of a file for serving to a client
which implements fs-verity compatible verification.  See the patch which
introduced FS_IOC_READ_VERITY_METADATA for more details.

"fs-verity descriptor" here means only the part that userspace cares
about because it is hashed to produce the file digest.  It doesn't
include the signature which ext4 and f2fs append to the
fsverity_descriptor struct when storing it on-disk, since that way of
storing the signature is an implementation detail.  The next patch adds
a separate metadata_type value for retrieving the signature separately.

This has been tested using a new xfstest which calls this ioctl via a
new subcommand for the 'fsverity' program from fsverity-utils.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fsverity.rst |  4 +++
 fs/verity/read_metadata.c              | 40 ++++++++++++++++++++++++++
 include/uapi/linux/fsverity.h          |  1 +
 3 files changed, 45 insertions(+)

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 50b47a6d9ea11..6dc5772037ef9 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -235,6 +235,7 @@ need this ioctl.
 This ioctl takes in a pointer to the following structure::
 
    #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
+   #define FS_VERITY_METADATA_TYPE_DESCRIPTOR      2
 
    struct fsverity_read_metadata_arg {
            __u64 metadata_type;
@@ -252,6 +253,9 @@ This ioctl takes in a pointer to the following structure::
   the same order that their hashes are themselves hashed.
   See `Merkle tree`_ for more information.
 
+- ``FS_VERITY_METADATA_TYPE_DESCRIPTOR`` reads the fs-verity
+  descriptor.  See `fs-verity descriptor`_.
+
 The semantics are similar to those of ``pread()``.  ``offset``
 specifies the offset in bytes into the metadata item to read from, and
 ``length`` specifies the maximum number of bytes to read from the
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 0f8ad2991cf90..2dea6dd3bb05a 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -76,6 +76,44 @@ static int fsverity_read_merkle_tree(struct inode *inode,
 	}
 	return retval ? retval : err;
 }
+
+/* Copy the requested portion of the buffer to userspace. */
+static int fsverity_read_buffer(void __user *dst, u64 offset, int length,
+				const void *src, size_t src_length)
+{
+	if (offset >= src_length)
+		return 0;
+	src += offset;
+	src_length -= offset;
+
+	length = min_t(size_t, length, src_length);
+
+	if (copy_to_user(dst, src, length))
+		return -EFAULT;
+
+	return length;
+}
+
+static int fsverity_read_descriptor(struct inode *inode,
+				    void __user *buf, u64 offset, int length)
+{
+	struct fsverity_descriptor *desc;
+	size_t desc_size;
+	int res;
+
+	res = fsverity_get_descriptor(inode, &desc, &desc_size);
+	if (res)
+		return res;
+
+	/* don't include the signature */
+	desc_size = offsetof(struct fsverity_descriptor, signature);
+	desc->sig_size = 0;
+
+	res = fsverity_read_buffer(buf, offset, length, desc, desc_size);
+
+	kfree(desc);
+	return res;
+}
 /**
  * fsverity_ioctl_read_metadata() - read verity metadata from a file
  * @filp: file to read the metadata from
@@ -118,6 +156,8 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
 	case FS_VERITY_METADATA_TYPE_MERKLE_TREE:
 		return fsverity_read_merkle_tree(inode, vi, buf, arg.offset,
 						 length);
+	case FS_VERITY_METADATA_TYPE_DESCRIPTOR:
+		return fsverity_read_descriptor(inode, buf, arg.offset, length);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
index 94003b153cb3d..41abc283dbccb 100644
--- a/include/uapi/linux/fsverity.h
+++ b/include/uapi/linux/fsverity.h
@@ -84,6 +84,7 @@ struct fsverity_formatted_digest {
 };
 
 #define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
+#define FS_VERITY_METADATA_TYPE_DESCRIPTOR	2
 
 struct fsverity_read_metadata_arg {
 	__u64 metadata_type;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/6] fs-verity: support reading signature with ioctl
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
                   ` (4 preceding siblings ...)
  2021-01-15 18:18 ` [PATCH 5/6] fs-verity: support reading descriptor " Eric Biggers
@ 2021-01-15 18:18 ` Eric Biggers
  2021-01-28  1:11   ` Jaegeuk Kim
  2021-01-22 23:26 ` [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Victor Hsieh
  2021-02-01 17:41 ` Eric Biggers
  7 siblings, 1 reply; 21+ messages in thread
From: Eric Biggers @ 2021-01-15 18:18 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-api,
	Theodore Ts'o, Jaegeuk Kim, Victor Hsieh

From: Eric Biggers <ebiggers@google.com>

Add support for FS_VERITY_METADATA_TYPE_SIGNATURE to
FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
retrieve the built-in signature (if present) of a verity file for
serving to a client which implements fs-verity compatible verification.
See the patch which introduced FS_IOC_READ_VERITY_METADATA for more
details.

The ability for userspace to read the built-in signatures is also useful
because it allows a system that is using the in-kernel signature
verification to migrate to userspace signature verification.

This has been tested using a new xfstest which calls this ioctl via a
new subcommand for the 'fsverity' program from fsverity-utils.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/filesystems/fsverity.rst |  9 +++++++-
 fs/verity/read_metadata.c              | 30 ++++++++++++++++++++++++++
 include/uapi/linux/fsverity.h          |  1 +
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 6dc5772037ef9..1d831e3cbcb33 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -236,6 +236,7 @@ This ioctl takes in a pointer to the following structure::
 
    #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
    #define FS_VERITY_METADATA_TYPE_DESCRIPTOR      2
+   #define FS_VERITY_METADATA_TYPE_SIGNATURE       3
 
    struct fsverity_read_metadata_arg {
            __u64 metadata_type;
@@ -256,6 +257,10 @@ This ioctl takes in a pointer to the following structure::
 - ``FS_VERITY_METADATA_TYPE_DESCRIPTOR`` reads the fs-verity
   descriptor.  See `fs-verity descriptor`_.
 
+- ``FS_VERITY_METADATA_TYPE_SIGNATURE`` reads the signature which was
+  passed to FS_IOC_ENABLE_VERITY, if any.  See `Built-in signature
+  verification`_.
+
 The semantics are similar to those of ``pread()``.  ``offset``
 specifies the offset in bytes into the metadata item to read from, and
 ``length`` specifies the maximum number of bytes to read from the
@@ -279,7 +284,9 @@ FS_IOC_READ_VERITY_METADATA can fail with the following errors:
 - ``EINTR``: the ioctl was interrupted before any data was read
 - ``EINVAL``: reserved fields were set, or ``offset + length``
   overflowed
-- ``ENODATA``: the file is not a verity file
+- ``ENODATA``: the file is not a verity file, or
+  FS_VERITY_METADATA_TYPE_SIGNATURE was requested but the file doesn't
+  have a built-in signature
 - ``ENOTTY``: this type of filesystem does not implement fs-verity, or
   this ioctl is not yet implemented on it
 - ``EOPNOTSUPP``: the kernel was not configured with fs-verity
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 2dea6dd3bb05a..7e2d0c7bdf0de 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -114,6 +114,34 @@ static int fsverity_read_descriptor(struct inode *inode,
 	kfree(desc);
 	return res;
 }
+
+static int fsverity_read_signature(struct inode *inode,
+				   void __user *buf, u64 offset, int length)
+{
+	struct fsverity_descriptor *desc;
+	size_t desc_size;
+	int res;
+
+	res = fsverity_get_descriptor(inode, &desc, &desc_size);
+	if (res)
+		return res;
+
+	if (desc->sig_size == 0) {
+		res = -ENODATA;
+		goto out;
+	}
+
+	/*
+	 * Include only the signature.  Note that fsverity_get_descriptor()
+	 * already verified that sig_size is in-bounds.
+	 */
+	res = fsverity_read_buffer(buf, offset, length, desc->signature,
+				   le32_to_cpu(desc->sig_size));
+out:
+	kfree(desc);
+	return res;
+}
+
 /**
  * fsverity_ioctl_read_metadata() - read verity metadata from a file
  * @filp: file to read the metadata from
@@ -158,6 +186,8 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
 						 length);
 	case FS_VERITY_METADATA_TYPE_DESCRIPTOR:
 		return fsverity_read_descriptor(inode, buf, arg.offset, length);
+	case FS_VERITY_METADATA_TYPE_SIGNATURE:
+		return fsverity_read_signature(inode, buf, arg.offset, length);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
index 41abc283dbccb..15384e22e331e 100644
--- a/include/uapi/linux/fsverity.h
+++ b/include/uapi/linux/fsverity.h
@@ -85,6 +85,7 @@ struct fsverity_formatted_digest {
 
 #define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
 #define FS_VERITY_METADATA_TYPE_DESCRIPTOR	2
+#define FS_VERITY_METADATA_TYPE_SIGNATURE	3
 
 struct fsverity_read_metadata_arg {
 	__u64 metadata_type;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/6] fs-verity: add an ioctl to read verity metadata
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
                   ` (5 preceding siblings ...)
  2021-01-15 18:18 ` [PATCH 6/6] fs-verity: support reading signature " Eric Biggers
@ 2021-01-22 23:26 ` Victor Hsieh
  2021-01-25 18:41   ` Eric Biggers
  2021-02-01 17:41 ` Eric Biggers
  7 siblings, 1 reply; 21+ messages in thread
From: Victor Hsieh @ 2021-01-22 23:26 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Jaegeuk Kim

LGTM. Thanks!

Reviewed-by: Victor Hsieh <victorhsieh@google.com>

On Fri, Jan 15, 2021 at 10:19 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> [This patchset applies to v5.11-rc3]
>
> Add an ioctl FS_IOC_READ_VERITY_METADATA which allows reading verity
> metadata from a file that has fs-verity enabled, including:
>
> - The Merkle tree
> - The fsverity_descriptor (not including the signature if present)
> - The built-in signature, if present
>
> This ioctl has similar semantics to pread().  It is passed the type of
> metadata to read (one of the above three), and a buffer, offset, and
> size.  It returns the number of bytes read or an error.
>
> This ioctl doesn't make any assumption about where the metadata is
> stored on-disk.  It does assume the metadata is in a stable format, but
> that's basically already the case:
>
> - The Merkle tree and fsverity_descriptor are defined by how fs-verity
>   file digests are computed; see the "File digest computation" section
>   of Documentation/filesystems/fsverity.rst.  Technically, the way in
>   which the levels of the tree are ordered relative to each other wasn't
>   previously specified, but it's logical to put the root level first.
>
> - The built-in signature is the value passed to FS_IOC_ENABLE_VERITY.
>
> This ioctl is useful because it allows writing a server program that
> takes a verity file and serves it to a client program, such that the
> client can do its own fs-verity compatible verification of the file.
> This only makes sense if the client doesn't trust the server and if the
> server needs to provide the storage for the client.
>
> More concretely, there is interest in using this ability in Android to
> export APK files (which are protected by fs-verity) to "protected VMs".
> This would use Protected KVM (https://lwn.net/Articles/836693), which
> provides an isolated execution environment without having to trust the
> traditional "host".  A "guest" VM can boot from a signed image and
> perform specific tasks in a minimum trusted environment using files that
> have fs-verity enabled on the host, without trusting the host or
> requiring that the guest has its own trusted storage.
>
> Technically, it would be possible to duplicate the metadata and store it
> in separate files for serving.  However, that would be less efficient
> and would require extra care in userspace to maintain file consistency.
>
> In addition to the above, the ability to read the built-in signatures is
> useful because it allows a system that is using the in-kernel signature
> verification to migrate to userspace signature verification.
>
> This patchset has been tested by new xfstests which call this new ioctl
> via a new subcommand for the 'fsverity' program from fsverity-utils.
>
> Eric Biggers (6):
>   fs-verity: factor out fsverity_get_descriptor()
>   fs-verity: don't pass whole descriptor to fsverity_verify_signature()
>   fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
>   fs-verity: support reading Merkle tree with ioctl
>   fs-verity: support reading descriptor with ioctl
>   fs-verity: support reading signature with ioctl
>
>  Documentation/filesystems/fsverity.rst |  76 ++++++++++
>  fs/ext4/ioctl.c                        |   7 +
>  fs/f2fs/file.c                         |  11 ++
>  fs/verity/Makefile                     |   1 +
>  fs/verity/fsverity_private.h           |  13 +-
>  fs/verity/open.c                       | 133 +++++++++++------
>  fs/verity/read_metadata.c              | 195 +++++++++++++++++++++++++
>  fs/verity/signature.c                  |  20 +--
>  include/linux/fsverity.h               |  12 ++
>  include/uapi/linux/fsverity.h          |  14 ++
>  10 files changed, 417 insertions(+), 65 deletions(-)
>  create mode 100644 fs/verity/read_metadata.c
>
>
> base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
> --
> 2.30.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/6] fs-verity: add an ioctl to read verity metadata
  2021-01-22 23:26 ` [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Victor Hsieh
@ 2021-01-25 18:41   ` Eric Biggers
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Biggers @ 2021-01-25 18:41 UTC (permalink / raw)
  To: Victor Hsieh
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Jaegeuk Kim

On Fri, Jan 22, 2021 at 03:26:48PM -0800, Victor Hsieh wrote:
> LGTM. Thanks!
> 
> Reviewed-by: Victor Hsieh <victorhsieh@google.com>
> 
> On Fri, Jan 15, 2021 at 10:19 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > [This patchset applies to v5.11-rc3]
> >
> > Add an ioctl FS_IOC_READ_VERITY_METADATA which allows reading verity
> > metadata from a file that has fs-verity enabled, including:

Thanks Victor.  Does anyone else have comments on this patchset?

- Eric

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  2021-01-15 18:18 ` [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl Eric Biggers
@ 2021-01-28  1:03   ` Jaegeuk Kim
  2021-02-07  7:46   ` [f2fs-dev] " Chao Yu
  1 sibling, 0 replies; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:03 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add an ioctl FS_IOC_READ_VERITY_METADATA which will allow reading verity
> metadata from a file that has fs-verity enabled, including:
> 
> - The Merkle tree
> - The fsverity_descriptor (not including the signature if present)
> - The built-in signature, if present
> 
> This ioctl has similar semantics to pread().  It is passed the type of
> metadata to read (one of the above three), and a buffer, offset, and
> size.  It returns the number of bytes read or an error.
> 
> Separate patches will add support for each of the above metadata types.
> This patch just adds the ioctl itself.
> 
> This ioctl doesn't make any assumption about where the metadata is
> stored on-disk.  It does assume the metadata is in a stable format, but
> that's basically already the case:
> 
> - The Merkle tree and fsverity_descriptor are defined by how fs-verity
>   file digests are computed; see the "File digest computation" section
>   of Documentation/filesystems/fsverity.rst.  Technically, the way in
>   which the levels of the tree are ordered relative to each other wasn't
>   previously specified, but it's logical to put the root level first.
> 
> - The built-in signature is the value passed to FS_IOC_ENABLE_VERITY.
> 
> This ioctl is useful because it allows writing a server program that
> takes a verity file and serves it to a client program, such that the
> client can do its own fs-verity compatible verification of the file.
> This only makes sense if the client doesn't trust the server and if the
> server needs to provide the storage for the client.
> 
> More concretely, there is interest in using this ability in Android to
> export APK files (which are protected by fs-verity) to "protected VMs".
> This would use Protected KVM (https://lwn.net/Articles/836693), which
> provides an isolated execution environment without having to trust the
> traditional "host".  A "guest" VM can boot from a signed image and
> perform specific tasks in a minimum trusted environment using files that
> have fs-verity enabled on the host, without trusting the host or
> requiring that the guest has its own trusted storage.
> 
> Technically, it would be possible to duplicate the metadata and store it
> in separate files for serving.  However, that would be less efficient
> and would require extra care in userspace to maintain file consistency.
> 
> In addition to the above, the ability to read the built-in signatures is
> useful because it allows a system that is using the in-kernel signature
> verification to migrate to userspace signature verification.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  Documentation/filesystems/fsverity.rst | 57 ++++++++++++++++++++++++++
>  fs/ext4/ioctl.c                        |  7 ++++
>  fs/f2fs/file.c                         | 11 +++++
>  fs/verity/Makefile                     |  1 +
>  fs/verity/read_metadata.c              | 55 +++++++++++++++++++++++++
>  include/linux/fsverity.h               | 12 ++++++
>  include/uapi/linux/fsverity.h          | 10 +++++
>  7 files changed, 153 insertions(+)
>  create mode 100644 fs/verity/read_metadata.c
> 
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index e0204a23e997e..9ef7a7de60085 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -217,6 +217,63 @@ FS_IOC_MEASURE_VERITY can fail with the following errors:
>  - ``EOVERFLOW``: the digest is longer than the specified
>    ``digest_size`` bytes.  Try providing a larger buffer.
>  
> +FS_IOC_READ_VERITY_METADATA
> +---------------------------
> +
> +The FS_IOC_READ_VERITY_METADATA ioctl reads verity metadata from a
> +verity file.  This ioctl is available since Linux v5.12.
> +
> +This ioctl allows writing a server program that takes a verity file
> +and serves it to a client program, such that the client can do its own
> +fs-verity compatible verification of the file.  This only makes sense
> +if the client doesn't trust the server and if the server needs to
> +provide the storage for the client.
> +
> +This is a fairly specialized use case, and most fs-verity users won't
> +need this ioctl.
> +
> +This ioctl takes in a pointer to the following structure::
> +
> +   struct fsverity_read_metadata_arg {
> +           __u64 metadata_type;
> +           __u64 offset;
> +           __u64 length;
> +           __u64 buf_ptr;
> +           __u64 __reserved;
> +   };
> +
> +``metadata_type`` specifies the type of metadata to read.
> +
> +The semantics are similar to those of ``pread()``.  ``offset``
> +specifies the offset in bytes into the metadata item to read from, and
> +``length`` specifies the maximum number of bytes to read from the
> +metadata item.  ``buf_ptr`` is the pointer to the buffer to read into,
> +cast to a 64-bit integer.  ``__reserved`` must be 0.  On success, the
> +number of bytes read is returned.  0 is returned at the end of the
> +metadata item.  The returned length may be less than ``length``, for
> +example if the ioctl is interrupted.
> +
> +The metadata returned by FS_IOC_READ_VERITY_METADATA isn't guaranteed
> +to be authenticated against the file digest that would be returned by
> +`FS_IOC_MEASURE_VERITY`_, as the metadata is expected to be used to
> +implement fs-verity compatible verification anyway (though absent a
> +malicious disk, the metadata will indeed match).  E.g. to implement
> +this ioctl, the filesystem is allowed to just read the Merkle tree
> +blocks from disk without actually verifying the path to the root node.
> +
> +FS_IOC_READ_VERITY_METADATA can fail with the following errors:
> +
> +- ``EFAULT``: the caller provided inaccessible memory
> +- ``EINTR``: the ioctl was interrupted before any data was read
> +- ``EINVAL``: reserved fields were set, or ``offset + length``
> +  overflowed
> +- ``ENODATA``: the file is not a verity file
> +- ``ENOTTY``: this type of filesystem does not implement fs-verity, or
> +  this ioctl is not yet implemented on it
> +- ``EOPNOTSUPP``: the kernel was not configured with fs-verity
> +  support, or the filesystem superblock has not had the 'verity'
> +  feature enabled on it.  (See `Filesystem support`_.)
> +
>  FS_IOC_GETFLAGS
>  ---------------
>  
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 524e134324475..56dc58adba8ac 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1306,6 +1306,12 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			return -EOPNOTSUPP;
>  		return fsverity_ioctl_measure(filp, (void __user *)arg);
>  
> +	case FS_IOC_READ_VERITY_METADATA:
> +		if (!ext4_has_feature_verity(sb))
> +			return -EOPNOTSUPP;
> +		return fsverity_ioctl_read_metadata(filp,
> +						    (const void __user *)arg);
> +
>  	default:
>  		return -ENOTTY;
>  	}
> @@ -1388,6 +1394,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case FS_IOC_GETFSMAP:
>  	case FS_IOC_ENABLE_VERITY:
>  	case FS_IOC_MEASURE_VERITY:
> +	case FS_IOC_READ_VERITY_METADATA:
>  	case EXT4_IOC_CLEAR_ES_CACHE:
>  	case EXT4_IOC_GETSTATE:
>  	case EXT4_IOC_GET_ES_CACHE:
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f585545277d77..d0aefb5b97fac 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3357,6 +3357,14 @@ static int f2fs_ioc_measure_verity(struct file *filp, unsigned long arg)
>  	return fsverity_ioctl_measure(filp, (void __user *)arg);
>  }
>  
> +static int f2fs_ioc_read_verity_metadata(struct file *filp, unsigned long arg)
> +{
> +	if (!f2fs_sb_has_verity(F2FS_I_SB(file_inode(filp))))
> +		return -EOPNOTSUPP;
> +
> +	return fsverity_ioctl_read_metadata(filp, (const void __user *)arg);
> +}
> +
>  static int f2fs_ioc_getfslabel(struct file *filp, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
> @@ -4272,6 +4280,8 @@ static long __f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		return f2fs_ioc_enable_verity(filp, arg);
>  	case FS_IOC_MEASURE_VERITY:
>  		return f2fs_ioc_measure_verity(filp, arg);
> +	case FS_IOC_READ_VERITY_METADATA:
> +		return f2fs_ioc_read_verity_metadata(filp, arg);
>  	case FS_IOC_GETFSLABEL:
>  		return f2fs_ioc_getfslabel(filp, arg);
>  	case FS_IOC_SETFSLABEL:
> @@ -4523,6 +4533,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case F2FS_IOC_RESIZE_FS:
>  	case FS_IOC_ENABLE_VERITY:
>  	case FS_IOC_MEASURE_VERITY:
> +	case FS_IOC_READ_VERITY_METADATA:
>  	case FS_IOC_GETFSLABEL:
>  	case FS_IOC_SETFSLABEL:
>  	case F2FS_IOC_GET_COMPRESS_BLOCKS:
> diff --git a/fs/verity/Makefile b/fs/verity/Makefile
> index 570e9136334d4..435559a4fa9ea 100644
> --- a/fs/verity/Makefile
> +++ b/fs/verity/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_FS_VERITY) += enable.o \
>  			   init.o \
>  			   measure.o \
>  			   open.o \
> +			   read_metadata.o \
>  			   verify.o
>  
>  obj-$(CONFIG_FS_VERITY_BUILTIN_SIGNATURES) += signature.o
> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> new file mode 100644
> index 0000000000000..43be990fd53e4
> --- /dev/null
> +++ b/fs/verity/read_metadata.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Ioctl to read verity metadata
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include "fsverity_private.h"
> +
> +#include <linux/uaccess.h>
> +
> +/**
> + * fsverity_ioctl_read_metadata() - read verity metadata from a file
> + * @filp: file to read the metadata from
> + * @uarg: user pointer to fsverity_read_metadata_arg
> + *
> + * Return: length read on success, 0 on EOF, -errno on failure
> + */
> +int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
> +{
> +	struct inode *inode = file_inode(filp);
> +	const struct fsverity_info *vi;
> +	struct fsverity_read_metadata_arg arg;
> +	int length;
> +	void __user *buf;
> +
> +	vi = fsverity_get_info(inode);
> +	if (!vi)
> +		return -ENODATA; /* not a verity file */
> +	/*
> +	 * Note that we don't have to explicitly check that the file is open for
> +	 * reading, since verity files can only be opened for reading.
> +	 */
> +
> +	if (copy_from_user(&arg, uarg, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (arg.__reserved)
> +		return -EINVAL;
> +
> +	/* offset + length must not overflow. */
> +	if (arg.offset + arg.length < arg.offset)
> +		return -EINVAL;
> +
> +	/* Ensure that the return value will fit in INT_MAX. */
> +	length = min_t(u64, arg.length, INT_MAX);
> +
> +	buf = u64_to_user_ptr(arg.buf_ptr);
> +
> +	switch (arg.metadata_type) {
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(fsverity_ioctl_read_metadata);
> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index c1144a4503920..b568b3c7d095e 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -138,6 +138,10 @@ int fsverity_file_open(struct inode *inode, struct file *filp);
>  int fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
>  void fsverity_cleanup_inode(struct inode *inode);
>  
> +/* read_metadata.c */
> +
> +int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg);
> +
>  /* verify.c */
>  
>  bool fsverity_verify_page(struct page *page);
> @@ -183,6 +187,14 @@ static inline void fsverity_cleanup_inode(struct inode *inode)
>  {
>  }
>  
> +/* read_metadata.c */
> +
> +static inline int fsverity_ioctl_read_metadata(struct file *filp,
> +					       const void __user *uarg)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  /* verify.c */
>  
>  static inline bool fsverity_verify_page(struct page *page)
> diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
> index 33f44156f8ea5..e062751294d01 100644
> --- a/include/uapi/linux/fsverity.h
> +++ b/include/uapi/linux/fsverity.h
> @@ -83,7 +83,17 @@ struct fsverity_formatted_digest {
>  	__u8 digest[];
>  };
>  
> +struct fsverity_read_metadata_arg {
> +	__u64 metadata_type;
> +	__u64 offset;
> +	__u64 length;
> +	__u64 buf_ptr;
> +	__u64 __reserved;
> +};
> +
>  #define FS_IOC_ENABLE_VERITY	_IOW('f', 133, struct fsverity_enable_arg)
>  #define FS_IOC_MEASURE_VERITY	_IOWR('f', 134, struct fsverity_digest)
> +#define FS_IOC_READ_VERITY_METADATA \
> +	_IOWR('f', 135, struct fsverity_read_metadata_arg)
>  
>  #endif /* _UAPI_LINUX_FSVERITY_H */
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor()
  2021-01-15 18:18 ` [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor() Eric Biggers
@ 2021-01-28  1:04   ` Jaegeuk Kim
  0 siblings, 0 replies; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:04 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> The FS_IOC_READ_VERITY_METADATA ioctl will need to return the fs-verity
> descriptor (and signature) to userspace.
> 
> There are a few ways we could implement this:
> 
> - Save a copy of the descriptor (and signature) in the fsverity_info
>   struct that hangs off of the in-memory inode.  However, this would
>   waste memory since most of the time it wouldn't be needed.
> 
> - Regenerate the descriptor from the merkle_tree_params in the
>   fsverity_info.  However, this wouldn't work for the signature, nor for
>   the salt which the merkle_tree_params only contains indirectly as part
>   of the 'hashstate'.  It would also be error-prone.
> 
> - Just get them from the filesystem again.  The disadvantage is that in
>   general we can't trust that they haven't been maliciously changed
>   since the file has opened.  However, the use cases for
>   FS_IOC_READ_VERITY_METADATA don't require that it verifies the chain
>   of trust.  So this is okay as long as we do some basic validation.
> 
> In preparation for implementing the third option, factor out a helper
> function fsverity_get_descriptor() which gets the descriptor (and
> appended signature) from the filesystem and does some basic validation.
> 
> As part of this, start checking the sig_size field for overflow.
> Currently fsverity_verify_signature() does this.  But the new ioctl will
> need this too, so do it earlier.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/verity/fsverity_private.h |   7 +-
>  fs/verity/open.c             | 130 +++++++++++++++++++++++------------
>  2 files changed, 91 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> index 6413d28664d6d..6c9caccc06021 100644
> --- a/fs/verity/fsverity_private.h
> +++ b/fs/verity/fsverity_private.h
> @@ -122,12 +122,17 @@ int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
>  				     const u8 *salt, size_t salt_size);
>  
>  struct fsverity_info *fsverity_create_info(const struct inode *inode,
> -					   void *desc, size_t desc_size);
> +					   struct fsverity_descriptor *desc,
> +					   size_t desc_size);
>  
>  void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
>  
>  void fsverity_free_info(struct fsverity_info *vi);
>  
> +int fsverity_get_descriptor(struct inode *inode,
> +			    struct fsverity_descriptor **desc_ret,
> +			    size_t *desc_size_ret);
> +
>  int __init fsverity_init_info_cache(void);
>  void __init fsverity_exit_info_cache(void);
>  
> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index 228d0eca3e2e5..a987bb785e9b0 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -142,45 +142,17 @@ static int compute_file_digest(struct fsverity_hash_alg *hash_alg,
>  }
>  
>  /*
> - * Validate the given fsverity_descriptor and create a new fsverity_info from
> - * it.  The signature (if present) is also checked.
> + * Create a new fsverity_info from the given fsverity_descriptor (with optional
> + * appended signature), and check the signature if present.  The
> + * fsverity_descriptor must have already undergone basic validation.
>   */
>  struct fsverity_info *fsverity_create_info(const struct inode *inode,
> -					   void *_desc, size_t desc_size)
> +					   struct fsverity_descriptor *desc,
> +					   size_t desc_size)
>  {
> -	struct fsverity_descriptor *desc = _desc;
>  	struct fsverity_info *vi;
>  	int err;
>  
> -	if (desc_size < sizeof(*desc)) {
> -		fsverity_err(inode, "Unrecognized descriptor size: %zu bytes",
> -			     desc_size);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	if (desc->version != 1) {
> -		fsverity_err(inode, "Unrecognized descriptor version: %u",
> -			     desc->version);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	if (memchr_inv(desc->__reserved, 0, sizeof(desc->__reserved))) {
> -		fsverity_err(inode, "Reserved bits set in descriptor");
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	if (desc->salt_size > sizeof(desc->salt)) {
> -		fsverity_err(inode, "Invalid salt_size: %u", desc->salt_size);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	if (le64_to_cpu(desc->data_size) != inode->i_size) {
> -		fsverity_err(inode,
> -			     "Wrong data_size: %llu (desc) != %lld (inode)",
> -			     le64_to_cpu(desc->data_size), inode->i_size);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>  	vi = kmem_cache_zalloc(fsverity_info_cachep, GFP_KERNEL);
>  	if (!vi)
>  		return ERR_PTR(-ENOMEM);
> @@ -245,15 +217,57 @@ void fsverity_free_info(struct fsverity_info *vi)
>  	kmem_cache_free(fsverity_info_cachep, vi);
>  }
>  
> -/* Ensure the inode has an ->i_verity_info */
> -static int ensure_verity_info(struct inode *inode)
> +static bool validate_fsverity_descriptor(struct inode *inode,
> +					 const struct fsverity_descriptor *desc,
> +					 size_t desc_size)
>  {
> -	struct fsverity_info *vi = fsverity_get_info(inode);
> -	struct fsverity_descriptor *desc;
> -	int res;
> +	if (desc_size < sizeof(*desc)) {
> +		fsverity_err(inode, "Unrecognized descriptor size: %zu bytes",
> +			     desc_size);
> +		return false;
> +	}
>  
> -	if (vi)
> -		return 0;
> +	if (desc->version != 1) {
> +		fsverity_err(inode, "Unrecognized descriptor version: %u",
> +			     desc->version);
> +		return false;
> +	}
> +
> +	if (memchr_inv(desc->__reserved, 0, sizeof(desc->__reserved))) {
> +		fsverity_err(inode, "Reserved bits set in descriptor");
> +		return false;
> +	}
> +
> +	if (desc->salt_size > sizeof(desc->salt)) {
> +		fsverity_err(inode, "Invalid salt_size: %u", desc->salt_size);
> +		return false;
> +	}
> +
> +	if (le64_to_cpu(desc->data_size) != inode->i_size) {
> +		fsverity_err(inode,
> +			     "Wrong data_size: %llu (desc) != %lld (inode)",
> +			     le64_to_cpu(desc->data_size), inode->i_size);
> +		return false;
> +	}
> +
> +	if (le32_to_cpu(desc->sig_size) > desc_size - sizeof(*desc)) {
> +		fsverity_err(inode, "Signature overflows verity descriptor");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * Read the inode's fsverity_descriptor (with optional appended signature) from
> + * the filesystem, and do basic validation of it.
> + */
> +int fsverity_get_descriptor(struct inode *inode,
> +			    struct fsverity_descriptor **desc_ret,
> +			    size_t *desc_size_ret)
> +{
> +	int res;
> +	struct fsverity_descriptor *desc;
>  
>  	res = inode->i_sb->s_vop->get_verity_descriptor(inode, NULL, 0);
>  	if (res < 0) {
> @@ -272,20 +286,46 @@ static int ensure_verity_info(struct inode *inode)
>  	res = inode->i_sb->s_vop->get_verity_descriptor(inode, desc, res);
>  	if (res < 0) {
>  		fsverity_err(inode, "Error %d reading verity descriptor", res);
> -		goto out_free_desc;
> +		kfree(desc);
> +		return res;
> +	}
> +
> +	if (!validate_fsverity_descriptor(inode, desc, res)) {
> +		kfree(desc);
> +		return -EINVAL;
>  	}
>  
> -	vi = fsverity_create_info(inode, desc, res);
> +	*desc_ret = desc;
> +	*desc_size_ret = res;
> +	return 0;
> +}
> +
> +/* Ensure the inode has an ->i_verity_info */
> +static int ensure_verity_info(struct inode *inode)
> +{
> +	struct fsverity_info *vi = fsverity_get_info(inode);
> +	struct fsverity_descriptor *desc;
> +	size_t desc_size;
> +	int err;
> +
> +	if (vi)
> +		return 0;
> +
> +	err = fsverity_get_descriptor(inode, &desc, &desc_size);
> +	if (err)
> +		return err;
> +
> +	vi = fsverity_create_info(inode, desc, desc_size);
>  	if (IS_ERR(vi)) {
> -		res = PTR_ERR(vi);
> +		err = PTR_ERR(vi);
>  		goto out_free_desc;
>  	}
>  
>  	fsverity_set_info(inode, vi);
> -	res = 0;
> +	err = 0;
>  out_free_desc:
>  	kfree(desc);
> -	return res;
> +	return err;
>  }
>  
>  /**
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature()
  2021-01-15 18:18 ` [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature() Eric Biggers
@ 2021-01-28  1:04   ` Jaegeuk Kim
  2021-01-28  3:24   ` Amy Parker
  1 sibling, 0 replies; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:04 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Now that fsverity_get_descriptor() validates the sig_size field,
> fsverity_verify_signature() doesn't need to do it.
> 
> Just change the prototype of fsverity_verify_signature() to take the
> signature directly rather than take a fsverity_descriptor.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  fs/verity/fsverity_private.h |  6 ++----
>  fs/verity/open.c             |  3 ++-
>  fs/verity/signature.c        | 20 ++++++--------------
>  3 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> index 6c9caccc06021..a7920434bae50 100644
> --- a/fs/verity/fsverity_private.h
> +++ b/fs/verity/fsverity_private.h
> @@ -140,15 +140,13 @@ void __init fsverity_exit_info_cache(void);
>  
>  #ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
>  int fsverity_verify_signature(const struct fsverity_info *vi,
> -			      const struct fsverity_descriptor *desc,
> -			      size_t desc_size);
> +			      const u8 *signature, size_t sig_size);
>  
>  int __init fsverity_init_signature(void);
>  #else /* !CONFIG_FS_VERITY_BUILTIN_SIGNATURES */
>  static inline int
>  fsverity_verify_signature(const struct fsverity_info *vi,
> -			  const struct fsverity_descriptor *desc,
> -			  size_t desc_size)
> +			  const u8 *signature, size_t sig_size)
>  {
>  	return 0;
>  }
> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index a987bb785e9b0..60ff8af7219fe 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -181,7 +181,8 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
>  		 vi->tree_params.hash_alg->name,
>  		 vi->tree_params.digest_size, vi->file_digest);
>  
> -	err = fsverity_verify_signature(vi, desc, desc_size);
> +	err = fsverity_verify_signature(vi, desc->signature,
> +					le32_to_cpu(desc->sig_size));
>  out:
>  	if (err) {
>  		fsverity_free_info(vi);
> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index 012468eda2a78..143a530a80088 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -29,21 +29,19 @@ static struct key *fsverity_keyring;
>  /**
>   * fsverity_verify_signature() - check a verity file's signature
>   * @vi: the file's fsverity_info
> - * @desc: the file's fsverity_descriptor
> - * @desc_size: size of @desc
> + * @signature: the file's built-in signature
> + * @sig_size: size of signature in bytes, or 0 if no signature
>   *
> - * If the file's fs-verity descriptor includes a signature of the file digest,
> - * verify it against the certificates in the fs-verity keyring.
> + * If the file includes a signature of its fs-verity file digest, verify it
> + * against the certificates in the fs-verity keyring.
>   *
>   * Return: 0 on success (signature valid or not required); -errno on failure
>   */
>  int fsverity_verify_signature(const struct fsverity_info *vi,
> -			      const struct fsverity_descriptor *desc,
> -			      size_t desc_size)
> +			      const u8 *signature, size_t sig_size)
>  {
>  	const struct inode *inode = vi->inode;
>  	const struct fsverity_hash_alg *hash_alg = vi->tree_params.hash_alg;
> -	const u32 sig_size = le32_to_cpu(desc->sig_size);
>  	struct fsverity_formatted_digest *d;
>  	int err;
>  
> @@ -56,11 +54,6 @@ int fsverity_verify_signature(const struct fsverity_info *vi,
>  		return 0;
>  	}
>  
> -	if (sig_size > desc_size - sizeof(*desc)) {
> -		fsverity_err(inode, "Signature overflows verity descriptor");
> -		return -EBADMSG;
> -	}
> -
>  	d = kzalloc(sizeof(*d) + hash_alg->digest_size, GFP_KERNEL);
>  	if (!d)
>  		return -ENOMEM;
> @@ -70,8 +63,7 @@ int fsverity_verify_signature(const struct fsverity_info *vi,
>  	memcpy(d->digest, vi->file_digest, hash_alg->digest_size);
>  
>  	err = verify_pkcs7_signature(d, sizeof(*d) + hash_alg->digest_size,
> -				     desc->signature, sig_size,
> -				     fsverity_keyring,
> +				     signature, sig_size, fsverity_keyring,
>  				     VERIFYING_UNSPECIFIED_SIGNATURE,
>  				     NULL, NULL);
>  	kfree(d);
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl
  2021-01-15 18:18 ` [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl Eric Biggers
@ 2021-01-28  1:10   ` Jaegeuk Kim
  2021-01-28  2:17     ` Eric Biggers
  0 siblings, 1 reply; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:10 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add support for FS_VERITY_METADATA_TYPE_MERKLE_TREE to
> FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
> retrieve the Merkle tree of a verity file for serving to a client which
> implements fs-verity compatible verification.  See the patch which
> introduced FS_IOC_READ_VERITY_METADATA for more details.
> 
> This has been tested using a new xfstest which calls this ioctl via a
> new subcommand for the 'fsverity' program from fsverity-utils.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/filesystems/fsverity.rst | 10 +++-
>  fs/verity/read_metadata.c              | 70 ++++++++++++++++++++++++++
>  include/uapi/linux/fsverity.h          |  2 +
>  3 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index 9ef7a7de60085..50b47a6d9ea11 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -234,6 +234,8 @@ need this ioctl.
>  
>  This ioctl takes in a pointer to the following structure::
>  
> +   #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
> +
>     struct fsverity_read_metadata_arg {
>             __u64 metadata_type;
>             __u64 offset;
> @@ -242,7 +244,13 @@ This ioctl takes in a pointer to the following structure::
>             __u64 __reserved;
>     };
>  
> -``metadata_type`` specifies the type of metadata to read.
> +``metadata_type`` specifies the type of metadata to read:
> +
> +- ``FS_VERITY_METADATA_TYPE_MERKLE_TREE`` reads the blocks of the
> +  Merkle tree.  The blocks are returned in order from the root level
> +  to the leaf level.  Within each level, the blocks are returned in
> +  the same order that their hashes are themselves hashed.
> +  See `Merkle tree`_ for more information.
>  
>  The semantics are similar to those of ``pread()``.  ``offset``
>  specifies the offset in bytes into the metadata item to read from, and
> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> index 43be990fd53e4..0f8ad2991cf90 100644
> --- a/fs/verity/read_metadata.c
> +++ b/fs/verity/read_metadata.c
> @@ -7,8 +7,75 @@
>  
>  #include "fsverity_private.h"
>  
> +#include <linux/backing-dev.h>
> +#include <linux/highmem.h>
> +#include <linux/sched/signal.h>
>  #include <linux/uaccess.h>
>  
> +static int fsverity_read_merkle_tree(struct inode *inode,
> +				     const struct fsverity_info *vi,
> +				     void __user *buf, u64 offset, int length)
> +{
> +	const struct fsverity_operations *vops = inode->i_sb->s_vop;
> +	u64 end_offset;
> +	unsigned int offs_in_page;
> +	pgoff_t index, last_index;
> +	int retval = 0;
> +	int err = 0;
> +
> +	end_offset = min(offset + length, vi->tree_params.tree_size);
> +	if (offset >= end_offset)
> +		return 0;
> +	offs_in_page = offset_in_page(offset);
> +	last_index = (end_offset - 1) >> PAGE_SHIFT;
> +
> +	/*
> +	 * Iterate through each Merkle tree page in the requested range and copy
> +	 * the requested portion to userspace.  Note that the Merkle tree block
> +	 * size isn't important here, as we are returning a byte stream; i.e.,
> +	 * we can just work with pages even if the tree block size != PAGE_SIZE.
> +	 */
> +	for (index = offset >> PAGE_SHIFT; index <= last_index; index++) {
> +		unsigned long num_ra_pages =
> +			min_t(unsigned long, last_index - index + 1,
> +			      inode->i_sb->s_bdi->io_pages);
> +		unsigned int bytes_to_copy = min_t(u64, end_offset - offset,
> +						   PAGE_SIZE - offs_in_page);
> +		struct page *page;
> +		const void *virt;
> +
> +		page = vops->read_merkle_tree_page(inode, index, num_ra_pages);
> +		if (IS_ERR(page)) {
> +			err = PTR_ERR(page);
> +			fsverity_err(inode,
> +				     "Error %d reading Merkle tree page %lu",
> +				     err, index);
> +			break;
> +		}
> +
> +		virt = kmap(page);
> +		if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) {
> +			kunmap(page);
> +			put_page(page);
> +			err = -EFAULT;
> +			break;
> +		}
> +		kunmap(page);
> +		put_page(page);
> +
> +		retval += bytes_to_copy;
> +		buf += bytes_to_copy;
> +		offset += bytes_to_copy;
> +
> +		if (fatal_signal_pending(current))  {
> +			err = -EINTR;
> +			break;
> +		}
> +		cond_resched();
> +		offs_in_page = 0;
> +	}

Minor thought:
How about invalidating or truncating merkel tree pages?

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> +	return retval ? retval : err;
> +}
>  /**
>   * fsverity_ioctl_read_metadata() - read verity metadata from a file
>   * @filp: file to read the metadata from
> @@ -48,6 +115,9 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
>  	buf = u64_to_user_ptr(arg.buf_ptr);
>  
>  	switch (arg.metadata_type) {
> +	case FS_VERITY_METADATA_TYPE_MERKLE_TREE:
> +		return fsverity_read_merkle_tree(inode, vi, buf, arg.offset,
> +						 length);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
> index e062751294d01..94003b153cb3d 100644
> --- a/include/uapi/linux/fsverity.h
> +++ b/include/uapi/linux/fsverity.h
> @@ -83,6 +83,8 @@ struct fsverity_formatted_digest {
>  	__u8 digest[];
>  };
>  
> +#define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
> +
>  struct fsverity_read_metadata_arg {
>  	__u64 metadata_type;
>  	__u64 offset;
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/6] fs-verity: support reading descriptor with ioctl
  2021-01-15 18:18 ` [PATCH 5/6] fs-verity: support reading descriptor " Eric Biggers
@ 2021-01-28  1:11   ` Jaegeuk Kim
  0 siblings, 0 replies; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:11 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add support for FS_VERITY_METADATA_TYPE_DESCRIPTOR to
> FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
> retrieve the fs-verity descriptor of a file for serving to a client
> which implements fs-verity compatible verification.  See the patch which
> introduced FS_IOC_READ_VERITY_METADATA for more details.
> 
> "fs-verity descriptor" here means only the part that userspace cares
> about because it is hashed to produce the file digest.  It doesn't
> include the signature which ext4 and f2fs append to the
> fsverity_descriptor struct when storing it on-disk, since that way of
> storing the signature is an implementation detail.  The next patch adds
> a separate metadata_type value for retrieving the signature separately.
> 
> This has been tested using a new xfstest which calls this ioctl via a
> new subcommand for the 'fsverity' program from fsverity-utils.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  Documentation/filesystems/fsverity.rst |  4 +++
>  fs/verity/read_metadata.c              | 40 ++++++++++++++++++++++++++
>  include/uapi/linux/fsverity.h          |  1 +
>  3 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index 50b47a6d9ea11..6dc5772037ef9 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -235,6 +235,7 @@ need this ioctl.
>  This ioctl takes in a pointer to the following structure::
>  
>     #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
> +   #define FS_VERITY_METADATA_TYPE_DESCRIPTOR      2
>  
>     struct fsverity_read_metadata_arg {
>             __u64 metadata_type;
> @@ -252,6 +253,9 @@ This ioctl takes in a pointer to the following structure::
>    the same order that their hashes are themselves hashed.
>    See `Merkle tree`_ for more information.
>  
> +- ``FS_VERITY_METADATA_TYPE_DESCRIPTOR`` reads the fs-verity
> +  descriptor.  See `fs-verity descriptor`_.
> +
>  The semantics are similar to those of ``pread()``.  ``offset``
>  specifies the offset in bytes into the metadata item to read from, and
>  ``length`` specifies the maximum number of bytes to read from the
> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> index 0f8ad2991cf90..2dea6dd3bb05a 100644
> --- a/fs/verity/read_metadata.c
> +++ b/fs/verity/read_metadata.c
> @@ -76,6 +76,44 @@ static int fsverity_read_merkle_tree(struct inode *inode,
>  	}
>  	return retval ? retval : err;
>  }
> +
> +/* Copy the requested portion of the buffer to userspace. */
> +static int fsverity_read_buffer(void __user *dst, u64 offset, int length,
> +				const void *src, size_t src_length)
> +{
> +	if (offset >= src_length)
> +		return 0;
> +	src += offset;
> +	src_length -= offset;
> +
> +	length = min_t(size_t, length, src_length);
> +
> +	if (copy_to_user(dst, src, length))
> +		return -EFAULT;
> +
> +	return length;
> +}
> +
> +static int fsverity_read_descriptor(struct inode *inode,
> +				    void __user *buf, u64 offset, int length)
> +{
> +	struct fsverity_descriptor *desc;
> +	size_t desc_size;
> +	int res;
> +
> +	res = fsverity_get_descriptor(inode, &desc, &desc_size);
> +	if (res)
> +		return res;
> +
> +	/* don't include the signature */
> +	desc_size = offsetof(struct fsverity_descriptor, signature);
> +	desc->sig_size = 0;
> +
> +	res = fsverity_read_buffer(buf, offset, length, desc, desc_size);
> +
> +	kfree(desc);
> +	return res;
> +}
>  /**
>   * fsverity_ioctl_read_metadata() - read verity metadata from a file
>   * @filp: file to read the metadata from
> @@ -118,6 +156,8 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
>  	case FS_VERITY_METADATA_TYPE_MERKLE_TREE:
>  		return fsverity_read_merkle_tree(inode, vi, buf, arg.offset,
>  						 length);
> +	case FS_VERITY_METADATA_TYPE_DESCRIPTOR:
> +		return fsverity_read_descriptor(inode, buf, arg.offset, length);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
> index 94003b153cb3d..41abc283dbccb 100644
> --- a/include/uapi/linux/fsverity.h
> +++ b/include/uapi/linux/fsverity.h
> @@ -84,6 +84,7 @@ struct fsverity_formatted_digest {
>  };
>  
>  #define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
> +#define FS_VERITY_METADATA_TYPE_DESCRIPTOR	2
>  
>  struct fsverity_read_metadata_arg {
>  	__u64 metadata_type;
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/6] fs-verity: support reading signature with ioctl
  2021-01-15 18:18 ` [PATCH 6/6] fs-verity: support reading signature " Eric Biggers
@ 2021-01-28  1:11   ` Jaegeuk Kim
  0 siblings, 0 replies; 21+ messages in thread
From: Jaegeuk Kim @ 2021-01-28  1:11 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add support for FS_VERITY_METADATA_TYPE_SIGNATURE to
> FS_IOC_READ_VERITY_METADATA.  This allows a userspace server program to
> retrieve the built-in signature (if present) of a verity file for
> serving to a client which implements fs-verity compatible verification.
> See the patch which introduced FS_IOC_READ_VERITY_METADATA for more
> details.
> 
> The ability for userspace to read the built-in signatures is also useful
> because it allows a system that is using the in-kernel signature
> verification to migrate to userspace signature verification.
> 
> This has been tested using a new xfstest which calls this ioctl via a
> new subcommand for the 'fsverity' program from fsverity-utils.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  Documentation/filesystems/fsverity.rst |  9 +++++++-
>  fs/verity/read_metadata.c              | 30 ++++++++++++++++++++++++++
>  include/uapi/linux/fsverity.h          |  1 +
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index 6dc5772037ef9..1d831e3cbcb33 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -236,6 +236,7 @@ This ioctl takes in a pointer to the following structure::
>  
>     #define FS_VERITY_METADATA_TYPE_MERKLE_TREE     1
>     #define FS_VERITY_METADATA_TYPE_DESCRIPTOR      2
> +   #define FS_VERITY_METADATA_TYPE_SIGNATURE       3
>  
>     struct fsverity_read_metadata_arg {
>             __u64 metadata_type;
> @@ -256,6 +257,10 @@ This ioctl takes in a pointer to the following structure::
>  - ``FS_VERITY_METADATA_TYPE_DESCRIPTOR`` reads the fs-verity
>    descriptor.  See `fs-verity descriptor`_.
>  
> +- ``FS_VERITY_METADATA_TYPE_SIGNATURE`` reads the signature which was
> +  passed to FS_IOC_ENABLE_VERITY, if any.  See `Built-in signature
> +  verification`_.
> +
>  The semantics are similar to those of ``pread()``.  ``offset``
>  specifies the offset in bytes into the metadata item to read from, and
>  ``length`` specifies the maximum number of bytes to read from the
> @@ -279,7 +284,9 @@ FS_IOC_READ_VERITY_METADATA can fail with the following errors:
>  - ``EINTR``: the ioctl was interrupted before any data was read
>  - ``EINVAL``: reserved fields were set, or ``offset + length``
>    overflowed
> -- ``ENODATA``: the file is not a verity file
> +- ``ENODATA``: the file is not a verity file, or
> +  FS_VERITY_METADATA_TYPE_SIGNATURE was requested but the file doesn't
> +  have a built-in signature
>  - ``ENOTTY``: this type of filesystem does not implement fs-verity, or
>    this ioctl is not yet implemented on it
>  - ``EOPNOTSUPP``: the kernel was not configured with fs-verity
> diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
> index 2dea6dd3bb05a..7e2d0c7bdf0de 100644
> --- a/fs/verity/read_metadata.c
> +++ b/fs/verity/read_metadata.c
> @@ -114,6 +114,34 @@ static int fsverity_read_descriptor(struct inode *inode,
>  	kfree(desc);
>  	return res;
>  }
> +
> +static int fsverity_read_signature(struct inode *inode,
> +				   void __user *buf, u64 offset, int length)
> +{
> +	struct fsverity_descriptor *desc;
> +	size_t desc_size;
> +	int res;
> +
> +	res = fsverity_get_descriptor(inode, &desc, &desc_size);
> +	if (res)
> +		return res;
> +
> +	if (desc->sig_size == 0) {
> +		res = -ENODATA;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Include only the signature.  Note that fsverity_get_descriptor()
> +	 * already verified that sig_size is in-bounds.
> +	 */
> +	res = fsverity_read_buffer(buf, offset, length, desc->signature,
> +				   le32_to_cpu(desc->sig_size));
> +out:
> +	kfree(desc);
> +	return res;
> +}
> +
>  /**
>   * fsverity_ioctl_read_metadata() - read verity metadata from a file
>   * @filp: file to read the metadata from
> @@ -158,6 +186,8 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg)
>  						 length);
>  	case FS_VERITY_METADATA_TYPE_DESCRIPTOR:
>  		return fsverity_read_descriptor(inode, buf, arg.offset, length);
> +	case FS_VERITY_METADATA_TYPE_SIGNATURE:
> +		return fsverity_read_signature(inode, buf, arg.offset, length);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
> index 41abc283dbccb..15384e22e331e 100644
> --- a/include/uapi/linux/fsverity.h
> +++ b/include/uapi/linux/fsverity.h
> @@ -85,6 +85,7 @@ struct fsverity_formatted_digest {
>  
>  #define FS_VERITY_METADATA_TYPE_MERKLE_TREE	1
>  #define FS_VERITY_METADATA_TYPE_DESCRIPTOR	2
> +#define FS_VERITY_METADATA_TYPE_SIGNATURE	3
>  
>  struct fsverity_read_metadata_arg {
>  	__u64 metadata_type;
> -- 
> 2.30.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl
  2021-01-28  1:10   ` Jaegeuk Kim
@ 2021-01-28  2:17     ` Eric Biggers
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Biggers @ 2021-01-28  2:17 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: linux-fscrypt, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-api, Theodore Ts'o, Victor Hsieh

On Wed, Jan 27, 2021 at 05:10:39PM -0800, Jaegeuk Kim wrote:
> 
> Minor thought:
> How about invalidating or truncating merkel tree pages?
> 
> Reviewed-by: Jaegeuk Kim <jaegeuk@kernel.org>
> 

Removing them from the page cache after the read, you mean?  I'm not sure we can
assume that users of this ioctl would want the pages to *not* be cached, any
more than we could assume that for any regular read().  I think we should just
leave the pages cached (like a regular read) and not do anything special.  Like
other pagecache pages, the kernel will evict the Merkle tree pages eventually if
they aren't being accessed anymore and memory needs to be reclaimed.

- Eric

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature()
  2021-01-15 18:18 ` [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature() Eric Biggers
  2021-01-28  1:04   ` Jaegeuk Kim
@ 2021-01-28  3:24   ` Amy Parker
  1 sibling, 0 replies; 21+ messages in thread
From: Amy Parker @ 2021-01-28  3:24 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-fsdevel, Ext4 Developers List,
	linux-f2fs-devel, linux-api, Theodore Ts'o, Jaegeuk Kim,
	Victor Hsieh

On Fri, Jan 15, 2021 at 10:22 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Now that fsverity_get_descriptor() validates the sig_size field,
> fsverity_verify_signature() doesn't need to do it.
>
> Just change the prototype of fsverity_verify_signature() to take the
> signature directly rather than take a fsverity_descriptor.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Amy Parker <enbyamy@gmail.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/6] fs-verity: add an ioctl to read verity metadata
  2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
                   ` (6 preceding siblings ...)
  2021-01-22 23:26 ` [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Victor Hsieh
@ 2021-02-01 17:41 ` Eric Biggers
  7 siblings, 0 replies; 21+ messages in thread
From: Eric Biggers @ 2021-02-01 17:41 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: Theodore Ts'o, linux-api, linux-f2fs-devel, linux-fsdevel,
	Jaegeuk Kim, linux-ext4, Victor Hsieh

On Fri, Jan 15, 2021 at 10:18:13AM -0800, Eric Biggers wrote:
> [This patchset applies to v5.11-rc3]
> 
> Add an ioctl FS_IOC_READ_VERITY_METADATA which allows reading verity
> metadata from a file that has fs-verity enabled, including:
> 
> - The Merkle tree
> - The fsverity_descriptor (not including the signature if present)
> - The built-in signature, if present
> 
> This ioctl has similar semantics to pread().  It is passed the type of
> metadata to read (one of the above three), and a buffer, offset, and
> size.  It returns the number of bytes read or an error.
> 
> This ioctl doesn't make any assumption about where the metadata is
> stored on-disk.  It does assume the metadata is in a stable format, but
> that's basically already the case:
> 
> - The Merkle tree and fsverity_descriptor are defined by how fs-verity
>   file digests are computed; see the "File digest computation" section
>   of Documentation/filesystems/fsverity.rst.  Technically, the way in
>   which the levels of the tree are ordered relative to each other wasn't
>   previously specified, but it's logical to put the root level first.
> 
> - The built-in signature is the value passed to FS_IOC_ENABLE_VERITY.
> 
> This ioctl is useful because it allows writing a server program that
> takes a verity file and serves it to a client program, such that the
> client can do its own fs-verity compatible verification of the file.
> This only makes sense if the client doesn't trust the server and if the
> server needs to provide the storage for the client.
> 
> More concretely, there is interest in using this ability in Android to
> export APK files (which are protected by fs-verity) to "protected VMs".
> This would use Protected KVM (https://lwn.net/Articles/836693), which
> provides an isolated execution environment without having to trust the
> traditional "host".  A "guest" VM can boot from a signed image and
> perform specific tasks in a minimum trusted environment using files that
> have fs-verity enabled on the host, without trusting the host or
> requiring that the guest has its own trusted storage.
> 
> Technically, it would be possible to duplicate the metadata and store it
> in separate files for serving.  However, that would be less efficient
> and would require extra care in userspace to maintain file consistency.
> 
> In addition to the above, the ability to read the built-in signatures is
> useful because it allows a system that is using the in-kernel signature
> verification to migrate to userspace signature verification.
> 
> This patchset has been tested by new xfstests which call this new ioctl
> via a new subcommand for the 'fsverity' program from fsverity-utils.
> 
> Eric Biggers (6):
>   fs-verity: factor out fsverity_get_descriptor()
>   fs-verity: don't pass whole descriptor to fsverity_verify_signature()
>   fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
>   fs-verity: support reading Merkle tree with ioctl
>   fs-verity: support reading descriptor with ioctl
>   fs-verity: support reading signature with ioctl
> 
>  Documentation/filesystems/fsverity.rst |  76 ++++++++++
>  fs/ext4/ioctl.c                        |   7 +
>  fs/f2fs/file.c                         |  11 ++
>  fs/verity/Makefile                     |   1 +
>  fs/verity/fsverity_private.h           |  13 +-
>  fs/verity/open.c                       | 133 +++++++++++------
>  fs/verity/read_metadata.c              | 195 +++++++++++++++++++++++++
>  fs/verity/signature.c                  |  20 +--
>  include/linux/fsverity.h               |  12 ++
>  include/uapi/linux/fsverity.h          |  14 ++
>  10 files changed, 417 insertions(+), 65 deletions(-)
>  create mode 100644 fs/verity/read_metadata.c

All applied to fscrypt.git#fsverity for 5.12.

- Eric

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [f2fs-dev] [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  2021-01-15 18:18 ` [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl Eric Biggers
  2021-01-28  1:03   ` Jaegeuk Kim
@ 2021-02-07  7:46   ` Chao Yu
  2021-02-07  8:01     ` Eric Biggers
  1 sibling, 1 reply; 21+ messages in thread
From: Chao Yu @ 2021-02-07  7:46 UTC (permalink / raw)
  To: Eric Biggers, linux-fscrypt
  Cc: Theodore Ts'o, linux-api, linux-f2fs-devel, linux-fsdevel,
	Jaegeuk Kim, linux-ext4, Victor Hsieh

Hi Eric,

On 2021/1/16 2:18, Eric Biggers wrote:
> +static int f2fs_ioc_read_verity_metadata(struct file *filp, unsigned long arg)
> +{
> +	if (!f2fs_sb_has_verity(F2FS_I_SB(file_inode(filp))))
> +		return -EOPNOTSUPP;

One case is after we update kernel image, f2fs module may no longer support
compress algorithm which current file was compressed with, to avoid triggering
IO with empty compress engine (struct f2fs_compress_ops pointer):

It needs to add f2fs_is_compress_backend_ready() check condition here?

Thanks,

> +
> +	return fsverity_ioctl_read_metadata(filp, (const void __user *)arg);
> +}

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  2021-02-07  7:46   ` [f2fs-dev] " Chao Yu
@ 2021-02-07  8:01     ` Eric Biggers
  2021-02-07  8:32       ` Chao Yu
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Biggers @ 2021-02-07  8:01 UTC (permalink / raw)
  To: Chao Yu
  Cc: linux-fscrypt, Theodore Ts'o, linux-api, linux-f2fs-devel,
	linux-fsdevel, Jaegeuk Kim, linux-ext4, Victor Hsieh

On Sun, Feb 07, 2021 at 03:46:43PM +0800, Chao Yu wrote:
> Hi Eric,
> 
> On 2021/1/16 2:18, Eric Biggers wrote:
> > +static int f2fs_ioc_read_verity_metadata(struct file *filp, unsigned long arg)
> > +{
> > +	if (!f2fs_sb_has_verity(F2FS_I_SB(file_inode(filp))))
> > +		return -EOPNOTSUPP;
> 
> One case is after we update kernel image, f2fs module may no longer support
> compress algorithm which current file was compressed with, to avoid triggering
> IO with empty compress engine (struct f2fs_compress_ops pointer):
> 
> It needs to add f2fs_is_compress_backend_ready() check condition here?
> 
> Thanks,
> 
> > +
> > +	return fsverity_ioctl_read_metadata(filp, (const void __user *)arg);
> > +}

In that case it wouldn't have been possible to open the file, because
f2fs_file_open() checks for it.  So it's not necessary to repeat the same check
in every operation on the file descriptor.

- Eric

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl
  2021-02-07  8:01     ` Eric Biggers
@ 2021-02-07  8:32       ` Chao Yu
  0 siblings, 0 replies; 21+ messages in thread
From: Chao Yu @ 2021-02-07  8:32 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, Theodore Ts'o, linux-api, linux-f2fs-devel,
	linux-fsdevel, Jaegeuk Kim, linux-ext4, Victor Hsieh

On 2021/2/7 16:01, Eric Biggers wrote:
> On Sun, Feb 07, 2021 at 03:46:43PM +0800, Chao Yu wrote:
>> Hi Eric,
>>
>> On 2021/1/16 2:18, Eric Biggers wrote:
>>> +static int f2fs_ioc_read_verity_metadata(struct file *filp, unsigned long arg)
>>> +{
>>> +	if (!f2fs_sb_has_verity(F2FS_I_SB(file_inode(filp))))
>>> +		return -EOPNOTSUPP;
>>
>> One case is after we update kernel image, f2fs module may no longer support
>> compress algorithm which current file was compressed with, to avoid triggering
>> IO with empty compress engine (struct f2fs_compress_ops pointer):
>>
>> It needs to add f2fs_is_compress_backend_ready() check condition here?
>>
>> Thanks,
>>
>>> +
>>> +	return fsverity_ioctl_read_metadata(filp, (const void __user *)arg);
>>> +}
> 
> In that case it wouldn't have been possible to open the file, because
> f2fs_file_open() checks for it.  So it's not necessary to repeat the same check
> in every operation on the file descriptor.

Oh, yes, it's safe now.

I'm thinking we need to remove the check in f2fs_file_open(), because the check
will fail metadata access/update (via f{g,s}etxattr/ioctl), however original
intention of that check is only to avoid syscalls to touch compressed data w/o
the engine, anyway this is another topic.

The whole patchset looks fine to me, feel free to add:

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 
> - Eric
> .
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-02-07  8:35 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15 18:18 [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Eric Biggers
2021-01-15 18:18 ` [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor() Eric Biggers
2021-01-28  1:04   ` Jaegeuk Kim
2021-01-15 18:18 ` [PATCH 2/6] fs-verity: don't pass whole descriptor to fsverity_verify_signature() Eric Biggers
2021-01-28  1:04   ` Jaegeuk Kim
2021-01-28  3:24   ` Amy Parker
2021-01-15 18:18 ` [PATCH 3/6] fs-verity: add FS_IOC_READ_VERITY_METADATA ioctl Eric Biggers
2021-01-28  1:03   ` Jaegeuk Kim
2021-02-07  7:46   ` [f2fs-dev] " Chao Yu
2021-02-07  8:01     ` Eric Biggers
2021-02-07  8:32       ` Chao Yu
2021-01-15 18:18 ` [PATCH 4/6] fs-verity: support reading Merkle tree with ioctl Eric Biggers
2021-01-28  1:10   ` Jaegeuk Kim
2021-01-28  2:17     ` Eric Biggers
2021-01-15 18:18 ` [PATCH 5/6] fs-verity: support reading descriptor " Eric Biggers
2021-01-28  1:11   ` Jaegeuk Kim
2021-01-15 18:18 ` [PATCH 6/6] fs-verity: support reading signature " Eric Biggers
2021-01-28  1:11   ` Jaegeuk Kim
2021-01-22 23:26 ` [PATCH 0/6] fs-verity: add an ioctl to read verity metadata Victor Hsieh
2021-01-25 18:41   ` Eric Biggers
2021-02-01 17:41 ` Eric Biggers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).