[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1749031.NF6adcYWfa@tauon.atsec.com>
Date: Mon, 16 Sep 2024 09:14:13 -0500
From: Stephan Mueller <smueller@...onox.de>
To: George Rurikov <g.ryurikov@...uritycode.ru>,
Herbert Xu <herbert@...dor.apana.org.au>
Cc: MrRurikov <grurikovsherbakov@...dex.ru>,
"David S . Miller" <davem@...emloft.net>, linux-crypto@...r.kernel.org,
linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org,
stable@...r.kernel.org
Subject: Re: [PATCH] crypto: Fix logical operator in _aead_recvmsg()
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@...dex.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@...r.kernel.org
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> > Signed-off-by: MrRurikov <grurikovsherbakov@...dex.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
Powered by blists - more mailing lists