[PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Gaosheng Cui]

Refactor the code in krb5_DK to return PTR_ERR when an error occurs.

Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
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

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Hariprasad Kelam]

On 2024-07-12 at 12:54:23, Gaosheng Cui (cuigaosheng1@huawei.com) wrote:
> Refactor the code in krb5_DK to return PTR_ERR when an error occurs.
> 
 nit: Please change "-next" to "net-next" in subject

 Reviewed-by: Hariprasad Kelam <hkelam@marvell.com>

> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
> 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
> 
> 

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Jeff Layton]

On Fri, 2024-07-12 at 15:24 +0800, Gaosheng Cui wrote:
> Refactor the code in krb5_DK to return PTR_ERR when an error occurs.
> 
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
> 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;

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Chuck Lever]

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.


> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
> 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

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Chuck Lever]

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  ;-)

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@huawei.com>
> > ---
> > 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

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[cuigaosheng]

Thanks for reviewing this patch.

I think this modification makes sense, for example, if 
crypto_sync_skcipher_setkey

return -ENOMEM, it's better to return -ENOMEM than to return -EINVAL,

just like elsewhere.

On 2024/7/12 22:28, 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  ;-)
>
> 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@huawei.com>
>>> ---
>>> 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
>>

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[NeilBrown]

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.

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@huawei.com>
> > > ---
> > > 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
> 

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Chuck Lever III]

> On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@suse.de> 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@huawei.com>
>>>> ---
>>>> 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

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[cuigaosheng]

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@suse.de> 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@huawei.com>
>>>>> ---
>>>>> 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
>
>

Re: [PATCH -next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK

[Chuck Lever]

On Mon, Jul 15, 2024 at 09:44:30AM +0800, cuigaosheng wrote:
> But crypto_sync_skcipher_setkey maybe return -ENOMEM, Should this be
> modified?

Auditing this path is a bit challenging. The obvious memory failure
case is skcipher_setkey_unaligned(), but that is called only if
the cipher does not provide its own ->setkey method. (Did you see a
failure case that I missed?)

So you'll have to generate a list of ciphers that krb5_DK() uses
(which is only a few) and then check the ->setkey method of each
of those ciphers.

My guess is the skcipher_setkey fallback isn't used for any of the
ciphers that SunRPC GSS currently cares about.


> On 2024/7/15 0:18, Chuck Lever III wrote:
> > 
> > > On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@suse.de> 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@huawei.com>
> > > > > > ---
> > > > > > 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
> > 
> > 

-- 
Chuck Lever