[PATCH] crypto: Fix logical operator in _aead_recvmsg()

[PATCH] crypto: Fix logical operator in _aead_recvmsg()

[George Rurikov]

From: MrRurikov <grurikovsherbakov@yandex.ru>

After having been compared to a NULL value at algif_aead.c:191, pointer
'tsgl_src' is passed as 2nd parameter in call to function
'crypto_aead_copy_sgl' at algif_aead.c:244, where it is dereferenced at
algif_aead.c:85.

Change logical operator from && to || because pointer 'tsgl_src' is NULL,
then 'proccessed' will still be non-null

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Cc: stable@vger.kernel.org
Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
Signed-off-by: MrRurikov <grurikovsherbakov@yandex.ru>
---
 crypto/algif_aead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7d58cbbce4af..135f09a4b3f8 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -191,7 +191,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
                if (tsgl_src)
                        break;
        }
-       if (processed && !tsgl_src) {
+       if (processed || !tsgl_src) {
                err = -EFAULT;
                goto free;
        }
--
2.34.1

Заявление о конфиденциальности

Данное электронное письмо и любые приложения к нему являются конфиденциальными и предназначены исключительно для адресата. Если Вы не являетесь адресатом данного письма, пожалуйста, уведомите немедленно отправителя, не раскрывайте содержание другим лицам, не используйте его в каких-либо целях, не храните и не копируйте информацию любым способом.

Re: [PATCH] crypto: Fix logical operator in _aead_recvmsg()

[Greg KH]

On Mon, Sep 16, 2024 at 10:44:22AM +0300, George Rurikov wrote:
> Заявление о конфиденциальности
> 
> Данное электронное письмо и любые приложения к нему являются конфиденциальными и предназначены исключительно для адресата. Если Вы не являетесь адресатом данного письма, пожалуйста, уведомите немедленно отправителя, не раскрывайте содержание другим лицам, не используйте его в каких-либо целях, не храните и не копируйте информацию любым способом.
> 

Now deleted.

Re: [PATCH] crypto: Fix logical operator in _aead_recvmsg()

[Herbert Xu]

On Mon, Sep 16, 2024 at 10:44:22AM +0300, George Rurikov wrote:
> From: MrRurikov <grurikovsherbakov@yandex.ru>
> 
> After having been compared to a NULL value at algif_aead.c:191, pointer
> 'tsgl_src' is passed as 2nd parameter in call to function
> 'crypto_aead_copy_sgl' at algif_aead.c:244, where it is dereferenced at
> algif_aead.c:85.
> 
> Change logical operator from && to || because pointer 'tsgl_src' is NULL,
> then 'proccessed' will still be non-null
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Cc: stable@vger.kernel.org
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> Signed-off-by: MrRurikov <grurikovsherbakov@yandex.ru>
> ---
>  crypto/algif_aead.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Cc Stephan.

> 
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index 7d58cbbce4af..135f09a4b3f8 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -191,7 +191,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
>                 if (tsgl_src)
>                         break;
>         }
> -       if (processed && !tsgl_src) {
> +       if (processed || !tsgl_src) {
>                 err = -EFAULT;
>                 goto free;
>         }
> --
> 2.34.1
> 
> Заявление о конфиденциальности
> 
> Данное электронное письмо и любые приложения к нему являются конфиденциальными и предназначены исключительно для адресата. Если Вы не являетесь адресатом данного письма, пожалуйста, уведомите немедленно отправителя, не раскрывайте содержание другим лицам, не используйте его в каких-либо целях, не храните и не копируйте информацию любым способом.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: Fix logical operator in _aead_recvmsg()

[Stephan Mueller]

Am Montag, 16. September 2024, 03:38:56 GMT-5 schrieb Herbert Xu:

Hi George,

> On Mon, Sep 16, 2024 at 10:44:22AM +0300, George Rurikov wrote:
> > From: MrRurikov <grurikovsherbakov@yandex.ru>
> > 
> > After having been compared to a NULL value at algif_aead.c:191, pointer
> > 'tsgl_src' is passed as 2nd parameter in call to function
> > 'crypto_aead_copy_sgl' at algif_aead.c:244, where it is dereferenced at
> > algif_aead.c:85.
> > 
> > Change logical operator from && to || because pointer 'tsgl_src' is NULL,
> > then 'proccessed' will still be non-null
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> > Signed-off-by: MrRurikov <grurikovsherbakov@yandex.ru>
> > ---
> > 
> >  crypto/algif_aead.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Cc Stephan.

I am not sure that this is a valid finding. An issue exists when there is 
processed != 0 and TSGL is NULL. Otherwise, the subsequent copy operation for 
this part will simply copy nothing: even if TSGL is NULL, the processed value 
is 0 and this is uses as the length parameter in the copy operation. 
Technically any copy operation is prevented in the following code that is 
invoked by the used crypto_null cipher:

static int skcipher_walk_skcipher(struct skcipher_walk *walk,
                                  struct skcipher_request *req)
{
...
	/* here we have the value of processed */
        walk->total = req->cryptlen;

...
	/* here we stop processing */
        if (unlikely(!walk->total))
                return 0;

	/* here we dereference the TSGL */
	scatterwalk_start(&walk->in, req->src);

You see, the processing stops before the dereferencing.

In any case, the check as it currently is, allows the use of, say, you request 
a tag from just the key without any AAD or input data. Mathematically this is 
a valid operation.

Thus, as of now I do not see (a) a technical issue and (b) a mathematical 
issue.

Could you please help me understand the issue you think you are seeing?

Ciao
Stephan