| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YBINg601GssLLfAh@google.com>
Date: Wed, 27 Jan 2021 17:04:03 -0800
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
linux-api@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
Victor Hsieh <victorhsieh@...gle.com>
Subject: Re: [PATCH 1/6] fs-verity: factor out fsverity_get_descriptor()
On 01/15, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.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@...gle.com>
Reviewed-by: Jaegeuk Kim <jaegeuk@...nel.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
Powered by blists - more mailing lists