From b988f32c701760da8793f83f22f2043578e11b67 Mon Sep 17 00:00:00 2001 From: Rob N Date: Wed, 8 Mar 2023 09:05:14 +1100 Subject: [PATCH] Better handling for future crypto parameters The intent is that this is like ENOTSUP, but specifically for when something can't be done because we have no support for the requested crypto parameters; eg unlocking a dataset or receiving a stream encrypted with a suite we don't support. Its not intended to be recoverable without upgrading ZFS itself. If the request could be made to work by enabling a feature or modifying some other configuration item, then some other code should be used. load-key: In the future we might have more crypto suites (ie new values for the `encryption` property. Right now trying to load a key on such a future crypto suite will look up suite parameters off the end of the crypto table, resulting in misbehaviour and/or crashes (or, with debug enabled, trip the assertion in `zio_crypt_key_unwrap`). Instead, lets check the value we got from the dataset, and if we can't handle it, abort early. recv: When receiving a raw stream encrypted with an unknown crypto suite, `zfs recv` would report a generic `invalid backup stream` (EINVAL). While technically correct, its not super helpful, so lets ship a more specific error code and message. Reviewed-by: Tino Reichardt Reviewed-by: Brian Behlendorf Reviewed-by: Richard Yao Signed-off-by: Rob Norris Closes #14577 --- cmd/zdb/zdb.c | 3 ++- contrib/pyzfs/libzfs_core/_constants.py | 1 + include/sys/fs/zfs.h | 1 + lib/libzfs/libzfs_crypto.c | 5 +++++ lib/libzfs/libzfs_sendrecv.c | 6 ++++++ module/zfs/dsl_crypt.c | 16 ++++++++++++++-- 6 files changed, 29 insertions(+), 3 deletions(-) diff --git a/cmd/zdb/zdb.c b/cmd/zdb/zdb.c index 6c8d737130ee..c6198ee26416 100644 --- a/cmd/zdb/zdb.c +++ b/cmd/zdb/zdb.c @@ -3120,7 +3120,8 @@ zdb_load_key(objset_t *os) if (err != 0) fatal( "couldn't load encryption key for %s: %s", - encroot, strerror(err)); + encroot, err == ZFS_ERR_CRYPTO_NOTSUP ? + "crypto params not supported" : strerror(err)); ASSERT3U(dsl_dataset_get_keystatus(dd), ==, ZFS_KEYSTATUS_AVAILABLE); diff --git a/contrib/pyzfs/libzfs_core/_constants.py b/contrib/pyzfs/libzfs_core/_constants.py index 4db1de8d9a6c..5ee422dfa803 100644 --- a/contrib/pyzfs/libzfs_core/_constants.py +++ b/contrib/pyzfs/libzfs_core/_constants.py @@ -102,6 +102,7 @@ def enum(*sequential, **named): 'ZFS_ERR_VDEV_NOTSUP', 'ZFS_ERR_NOT_USER_NAMESPACE', 'ZFS_ERR_RESUME_EXISTS', + 'ZFS_ERR_CRYPTO_NOTSUP', ], {} ) diff --git a/include/sys/fs/zfs.h b/include/sys/fs/zfs.h index da2d0521655c..e869685c5e2d 100644 --- a/include/sys/fs/zfs.h +++ b/include/sys/fs/zfs.h @@ -1545,6 +1545,7 @@ typedef enum { ZFS_ERR_VDEV_NOTSUP, ZFS_ERR_NOT_USER_NAMESPACE, ZFS_ERR_RESUME_EXISTS, + ZFS_ERR_CRYPTO_NOTSUP, } zfs_errno_t; /* diff --git a/lib/libzfs/libzfs_crypto.c b/lib/libzfs/libzfs_crypto.c index 3ef883701082..e8351b22ff96 100644 --- a/lib/libzfs/libzfs_crypto.c +++ b/lib/libzfs/libzfs_crypto.c @@ -1407,6 +1407,11 @@ zfs_crypto_load_key(zfs_handle_t *zhp, boolean_t noop, "Incorrect key provided for '%s'."), zfs_get_name(zhp)); break; + case ZFS_ERR_CRYPTO_NOTSUP: + zfs_error_aux(zhp->zfs_hdl, dgettext(TEXT_DOMAIN, + "'%s' uses an unsupported encryption suite."), + zfs_get_name(zhp)); + break; } goto error; } diff --git a/lib/libzfs/libzfs_sendrecv.c b/lib/libzfs/libzfs_sendrecv.c index 66a22e333663..4b6e06df69be 100644 --- a/lib/libzfs/libzfs_sendrecv.c +++ b/lib/libzfs/libzfs_sendrecv.c @@ -5161,6 +5161,12 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap, "stream.")); (void) zfs_error(hdl, EZFS_BADVERSION, errbuf); break; + case ZFS_ERR_CRYPTO_NOTSUP: + zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, + "stream uses crypto parameters not compatible with " + "this pool")); + (void) zfs_error(hdl, EZFS_BADSTREAM, errbuf); + break; case EDQUOT: zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, "destination %s space quota exceeded."), name); diff --git a/module/zfs/dsl_crypt.c b/module/zfs/dsl_crypt.c index 408d038b4de4..dd936e74b1d9 100644 --- a/module/zfs/dsl_crypt.c +++ b/module/zfs/dsl_crypt.c @@ -541,6 +541,12 @@ dsl_crypto_key_open(objset_t *mos, dsl_wrapping_key_t *wkey, if (ret != 0) goto error; + /* handle a future crypto suite that we don't support */ + if (crypt >= ZIO_CRYPT_FUNCTIONS) { + ret = (SET_ERROR(ZFS_ERR_CRYPTO_NOTSUP)); + goto error; + } + ret = zap_lookup(mos, dckobj, DSL_CRYPTO_KEY_GUID, 8, 1, &guid); if (ret != 0) goto error; @@ -2141,10 +2147,16 @@ dsl_crypto_recv_raw_key_check(dsl_dataset_t *ds, nvlist_t *nvl, dmu_tx_t *tx) * wrapping key. */ ret = nvlist_lookup_uint64(nvl, DSL_CRYPTO_KEY_CRYPTO_SUITE, &intval); - if (ret != 0 || intval >= ZIO_CRYPT_FUNCTIONS || - intval <= ZIO_CRYPT_OFF) + if (ret != 0 || intval <= ZIO_CRYPT_OFF) return (SET_ERROR(EINVAL)); + /* + * Flag a future crypto suite that we don't support differently, so + * we can return a more useful error to the user. + */ + if (intval >= ZIO_CRYPT_FUNCTIONS) + return (SET_ERROR(ZFS_ERR_CRYPTO_NOTSUP)); + ret = nvlist_lookup_uint64(nvl, DSL_CRYPTO_KEY_GUID, &intval); if (ret != 0) return (SET_ERROR(EINVAL));