But crypto_sync_skcipher_setkey maybe return -ENOMEM, Should this be
modified?
thanks!
On 2024/7/15 0:18, Chuck Lever III wrote:
On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@xxxxxxx> wrote:
On Sat, 13 Jul 2024, Chuck Lever wrote:
On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote:
On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote:
Refactor the code in krb5_DK to return PTR_ERR when an error occurs.
My understanding of the current code is that if either
crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize()
fails, then krb5_DK() returns -EINVAL. At the only call site for
krb5_DK(), that return code is unconditionally discarded. Thus I
don't see that the proposed change is necessary or improves
anything.
My understanding is wrong ;-)
True, but I think your conclusion was correct.
krb5_DK() returns zero or -EINVAL.
It is only used by krb5_derive_key_v2(), which returns zero or -EINVAL,
or -ENOMEM.
These are really the only three interesting return codes.
Leaking other error codes to callers is not desirable, IMO.
But looking at the current implementation of
crypto_alloc_sync_skcipher(), it returns either
ERR_PTR(-EINVAL) or a valid pointer; it doesn't return any
other error value. Since it never returns -ENOMEM, there
still doesn't seem to be a technical reason for modifying
krb5_DK() to pass errors through.
krb4_derive_key_v2() is only used as a ->derive_key() method.
This is called from krb5_derive_key(), and various unit tests in
gss_krb5_tests.c
krb5_derive_key() is only called in gss_krb5_mech.c, and each call site
is of the form:
if (krb5_derive_key(...)) goto out;
so it doesn't matter what error is returned.
The unit test calls are all followed by
KUNIT_ASSERT_EQ(test, err, 0);
so the only place the err is used is (presumably) in failure reports
from the unit tests.
So the proposed change seems unnecessary from a practical perspective.
Maybe it is justified from an aesthetic perspective, but I think that
should be clearly stated in the commit message. e.g.
This change has no practical effect as all non-zero error statuses
are treated equally, however the distinction between EINVAL and ENOMEM
may be relevant at some future time and it seems cleaner to maintain
the distinction.
NeilBrown
The return code isn't discarded. A non-zero return code from
krb5_DK() is carried back up the call stack. The logic in
krb5_derive_key_v2() does not use the kernel's usual error flow
form, so I missed this.
However, it still isn't clear to me why the error behavior here
needs to change. It's possible, for example, that -EINVAL is
perfectly adequate to indicate when sync_skcipher() can't find the
specified encryption algorithm (gk5e->encrypt_name).
Specifying the wrong encryption type: -EINVAL. That makes sense.
Signed-off-by: Gaosheng Cui <cuigaosheng1@xxxxxxxxxx>
---
v2: Update IS_ERR to PTR_ERR, thanks very much!
net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c
index 4eb19c3a54c7..5ac8d06ab2c0 100644
--- a/net/sunrpc/auth_gss/gss_krb5_keys.c
+++ b/net/sunrpc/auth_gss/gss_krb5_keys.c
@@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e,
goto err_return;
cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0);
- if (IS_ERR(cipher))
+ if (IS_ERR(cipher)) {
+ ret = PTR_ERR(cipher);
goto err_return;
+ }
+
blocksize = crypto_sync_skcipher_blocksize(cipher);
- if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len))
+ ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len);
+ if (ret)
goto err_free_cipher;
ret = -ENOMEM;
--
2.25.1
--
Chuck Lever
--
Chuck Lever
--
Chuck Lever
