| Message ID | 20201114001529.185751-2-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add libfsverity_enable() API | expand |
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!
On Mon, Nov 16, 2020 at 11:52:57AM +0000, Luca Boccassi wrote: > 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_enable_with_sig() works since that; it allows sig == NULL and sig_size == 0. The reason I don't want the regular libfsverity_enable() to take the signature parameters is that I'd like to encourage people to do userspace signature verification instead. I want to avoid implying that the in-kernel signature verification is something that everyone should use. Same reason I didn't want 'fsverity digest' to output fsverity_formatted_digest by default. > > +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! First, providing defaults in libfsverity_enable() doesn't make sense unless the same defaults are provided in libfsverity_compute_digest() too. Second, PAGE_SIZE is a bad default block size. It really should be a fixed value, like 4096, so that e.g. if you sign files on both x86 and PowerPC, or sign on x86 and enable on PowerPC, you get the same results. So at least we shouldn't provide defaults unless it's done right. I suppose it's probably not too late to change the default for the fsverity program, though. I doubt anyone is using it with a non-4K PAGE_SIZE yet. Would it work for you if both libfsverity_compute_digest() and libfsverity_enable() defaulted to SHA-256 and 4096? - Eric
On Mon, 2020-11-16 at 09:41 -0800, Eric Biggers wrote: > On Mon, Nov 16, 2020 at 11:52:57AM +0000, Luca Boccassi wrote: > > 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_enable_with_sig() works since that; it allows sig == NULL and > sig_size == 0. > > The reason I don't want the regular libfsverity_enable() to take the signature > parameters is that I'd like to encourage people to do userspace signature > verification instead. I want to avoid implying that the in-kernel signature > verification is something that everyone should use. Same reason I didn't want > 'fsverity digest' to output fsverity_formatted_digest by default. Ok, I understand - makes sense to me, thanks. Maybe it's worth documenting in the the header description of the API that empty/null values are accepted and will result in enabling without signature check? > > > +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! > > First, providing defaults in libfsverity_enable() doesn't make sense unless the > same defaults are provided in libfsverity_compute_digest() too. > > Second, PAGE_SIZE is a bad default block size. It really should be a fixed > value, like 4096, so that e.g. if you sign files on both x86 and PowerPC, or > sign on x86 and enable on PowerPC, you get the same results. > > So at least we shouldn't provide defaults unless it's done right. > > I suppose it's probably not too late to change the default for the fsverity > program, though. I doubt anyone is using it with a non-4K PAGE_SIZE yet. > > Would it work for you if both libfsverity_compute_digest() and > libfsverity_enable() defaulted to SHA-256 and 4096? > > - Eric Yes, using those defaults in the functions would work perfectly fine for me, thank you!
On Mon, Nov 16, 2020 at 05:50:45PM +0000, Luca Boccassi wrote: > On Mon, 2020-11-16 at 09:41 -0800, Eric Biggers wrote: > > On Mon, Nov 16, 2020 at 11:52:57AM +0000, Luca Boccassi wrote: > > > 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_enable_with_sig() works since that; it allows sig == NULL and > > sig_size == 0. > > > > The reason I don't want the regular libfsverity_enable() to take the signature > > parameters is that I'd like to encourage people to do userspace signature > > verification instead. I want to avoid implying that the in-kernel signature > > verification is something that everyone should use. Same reason I didn't want > > 'fsverity digest' to output fsverity_formatted_digest by default. > > Ok, I understand - makes sense to me, thanks. > > Maybe it's worth documenting in the the header description of the API > that empty/null values are accepted and will result in enabling without > signature check? > It's already there: * If @sig is NULL and @sig_size is 0, this is the same as libfsverity_enable().
On Mon, 2020-11-16 at 10:42 -0800, Eric Biggers wrote: > On Mon, Nov 16, 2020 at 05:50:45PM +0000, Luca Boccassi wrote: > > On Mon, 2020-11-16 at 09:41 -0800, Eric Biggers wrote: > > > On Mon, Nov 16, 2020 at 11:52:57AM +0000, Luca Boccassi wrote: > > > > 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_enable_with_sig() works since that; it allows sig == NULL and > > > sig_size == 0. > > > > > > The reason I don't want the regular libfsverity_enable() to take the signature > > > parameters is that I'd like to encourage people to do userspace signature > > > verification instead. I want to avoid implying that the in-kernel signature > > > verification is something that everyone should use. Same reason I didn't want > > > 'fsverity digest' to output fsverity_formatted_digest by default. > > > > Ok, I understand - makes sense to me, thanks. > > > > Maybe it's worth documenting in the the header description of the API > > that empty/null values are accepted and will result in enabling without > > signature check? > > > > It's already there: > > * If @sig is NULL and @sig_size is 0, this is the same as libfsverity_enable(). Ah of course, sorry, right under my nose and still missed it :-)
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); + /** * 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; +} diff --git a/programs/cmd_enable.c b/programs/cmd_enable.c index d90d208..48d33c2 100644 --- a/programs/cmd_enable.c +++ b/programs/cmd_enable.c @@ -68,9 +68,10 @@ static const struct option longopts[] = { int fsverity_cmd_enable(const struct fsverity_command *cmd, int argc, char *argv[]) { - struct fsverity_enable_arg arg = { .version = 1 }; + struct libfsverity_merkle_tree_params tree_params = { .version = 1 }; u8 *salt = NULL; u8 *sig = NULL; + u32 sig_size = 0; struct filedes file; int status; int c; @@ -78,26 +79,28 @@ int fsverity_cmd_enable(const struct fsverity_command *cmd, while ((c = getopt_long(argc, argv, "", longopts, NULL)) != -1) { switch (c) { case OPT_HASH_ALG: - if (!parse_hash_alg_option(optarg, &arg.hash_algorithm)) + if (!parse_hash_alg_option(optarg, + &tree_params.hash_algorithm)) goto out_usage; break; case OPT_BLOCK_SIZE: - if (!parse_block_size_option(optarg, &arg.block_size)) + if (!parse_block_size_option(optarg, + &tree_params.block_size)) goto out_usage; break; case OPT_SALT: - if (!parse_salt_option(optarg, &salt, &arg.salt_size)) + if (!parse_salt_option(optarg, &salt, + &tree_params.salt_size)) goto out_usage; - arg.salt_ptr = (uintptr_t)salt; + tree_params.salt = salt; break; case OPT_SIGNATURE: if (sig != NULL) { error_msg("--signature can only be specified once"); goto out_usage; } - if (!read_signature(optarg, &sig, &arg.sig_size)) + if (!read_signature(optarg, &sig, &sig_size)) goto out_err; - arg.sig_ptr = (uintptr_t)sig; break; default: goto out_usage; @@ -110,15 +113,16 @@ int fsverity_cmd_enable(const struct fsverity_command *cmd, if (argc != 1) goto out_usage; - if (arg.hash_algorithm == 0) - arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT; + if (tree_params.hash_algorithm == 0) + tree_params.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT; - if (arg.block_size == 0) - arg.block_size = get_default_block_size(); + if (tree_params.block_size == 0) + tree_params.block_size = get_default_block_size(); if (!open_file(&file, argv[0], O_RDONLY, 0)) goto out_err; - if (ioctl(file.fd, FS_IOC_ENABLE_VERITY, &arg) != 0) { + + if (libfsverity_enable_with_sig(file.fd, &tree_params, sig, sig_size)) { error_msg_errno("FS_IOC_ENABLE_VERITY failed on '%s'", file.name); filedes_close(&file);