Re: [fsverity-utils PATCH 1/2] lib: add libfsverity_enable() and libfsverity_enable_with_sig()

[Luca Boccassi <Luca.Boccassi@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 5441 bytes --]

On Fri, 2020-11-13 at 16:15 -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Add convenience functions that wrap FS_IOC_ENABLE_VERITY but take a
> 'struct libfsverity_merkle_tree_params' instead of
> 'struct fsverity_enable_arg'.  This is useful because it allows
> libfsverity users to deal with one common struct.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  include/libfsverity.h | 36 ++++++++++++++++++++++++++++++++++
>  lib/enable.c          | 45 +++++++++++++++++++++++++++++++++++++++++++
>  programs/cmd_enable.c | 28 +++++++++++++++------------
>  3 files changed, 97 insertions(+), 12 deletions(-)
>  create mode 100644 lib/enable.c
> 
> diff --git a/include/libfsverity.h b/include/libfsverity.h
> index 8f78a13..a8aecaf 100644
> --- a/include/libfsverity.h
> +++ b/include/libfsverity.h
> @@ -112,6 +112,42 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
>  			const struct libfsverity_signature_params *sig_params,
>  			uint8_t **sig_ret, size_t *sig_size_ret);
>  
> +/**
> + * libfsverity_enable() - Enable fs-verity on a file
> + * @fd: read-only file descriptor to the file
> + * @params: pointer to the Merkle tree parameters
> + *
> + * This is a simple wrapper around the FS_IOC_ENABLE_VERITY ioctl.
> + *
> + * Return: 0 on success, -EINVAL for invalid arguments, or a negative errno
> + *	   value from the FS_IOC_ENABLE_VERITY ioctl.  See
> + *	   Documentation/filesystems/fsverity.rst in the kernel source tree for
> + *	   the possible error codes from FS_IOC_ENABLE_VERITY.
> + */
> +int
> +libfsverity_enable(int fd, const struct libfsverity_merkle_tree_params *params);
> +
> +/**
> + * libfsverity_enable_with_sig() - Enable fs-verity on a file, with a signature
> + * @fd: read-only file descriptor to the file
> + * @params: pointer to the Merkle tree parameters
> + * @sig: pointer to the file's signature
> + * @sig_size: size of the file's signature in bytes
> + *
> + * Like libfsverity_enable(), but allows specifying a built-in signature (i.e. a
> + * singature created with libfsverity_sign_digest()) to associate with the file.
> + * This is only needed if the in-kernel signature verification support is being
> + * used; it is not needed if signatures are being verified in userspace.
> + *
> + * If @sig is NULL and @sig_size is 0, this is the same as libfsverity_enable().
> + *
> + * Return: See libfsverity_enable().
> + */
> +int
> +libfsverity_enable_with_sig(int fd,
> +			    const struct libfsverity_merkle_tree_params *params,
> +			    const uint8_t *sig, size_t sig_size);
> +

I don't have a strong preference either way, but any specific reason
for a separate function rather than treating sig == NULL and sig_size
== 0 as a signature-less enable? For clients deploying files, it would
appear easier to me to just use empty parameters to choose between
signed/not signed, without having to also change which API to call. But
maybe there's some use case I'm missing where it's better to be
explicit.

>  /**
>   * libfsverity_find_hash_alg_by_name() - Find hash algorithm by name
>   * @name: Pointer to name of hash algorithm
> diff --git a/lib/enable.c b/lib/enable.c
> new file mode 100644
> index 0000000..dd77292
> --- /dev/null
> +++ b/lib/enable.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Implementation of libfsverity_enable() and libfsverity_enable_with_sig().
> + *
> + * Copyright 2020 Google LLC
> + *
> + * Use of this source code is governed by an MIT-style
> + * license that can be found in the LICENSE file or at
> + * https://opensource.org/licenses/MIT.
> + */
> +
> +#include "lib_private.h"
> +
> +#include <sys/ioctl.h>
> +
> +LIBEXPORT int
> +libfsverity_enable(int fd, const struct libfsverity_merkle_tree_params *params)
> +{
> +	return libfsverity_enable_with_sig(fd, params, NULL, 0);
> +}
> +
> +LIBEXPORT int
> +libfsverity_enable_with_sig(int fd,
> +			    const struct libfsverity_merkle_tree_params *params,
> +			    const uint8_t *sig, size_t sig_size)
> +{
> +	struct fsverity_enable_arg arg = {};
> +
> +	if (!params) {
> +		libfsverity_error_msg("missing required parameters for enable");
> +		return -EINVAL;
> +	}
> +
> +	arg.version = 1;
> +	arg.hash_algorithm = params->hash_algorithm;
> +	arg.block_size = params->block_size;
> +	arg.salt_size = params->salt_size;
> +	arg.salt_ptr = (uintptr_t)params->salt;
> +	arg.sig_size = sig_size;
> +	arg.sig_ptr = (uintptr_t)sig;
> +
> +	if (ioctl(fd, FS_IOC_ENABLE_VERITY, &arg) != 0)
> +		return -errno;
> +	return 0;
> +}

I'm ok with leaving file handling to clients - after all, depending on
infrastructure/bindings/helper libs/whatnot, file handling might vary
wildly.

But could we at least provide a default for block size and hash algo,
if none are specified?

While file handling is a generic concept and expected to be a solved
problem for most programs, figuring out what the default block size
should be or what hash algorithm to use is, are fs-verity specific
concepts that most clients (at least those that I deal with) wouldn't
care about in any way outside of this use, and would want it to "just
work".

Apart from these 2 comments, I ran a quick test of the 2 new series,
and everything works as expected. Thanks!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]