| Message ID | 20240916074422.503645-1-g.ryurikov@securitycode.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: Fix logical operator in _aead_recvmsg() | expand |
On Mon, Sep 16, 2024 at 10:44:22AM +0300, George Rurikov wrote: > Заявление о конфиденциальности > > Данное электронное письмо и любые приложения к нему являются конфиденциальными и предназначены исключительно для адресата. Если Вы не являетесь адресатом данного письма, пожалуйста, уведомите немедленно отправителя, не раскрывайте содержание другим лицам, не используйте его в каких-либо целях, не храните и не копируйте информацию любым способом. > Now deleted.
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 > > Заявление о конфиденциальности > > Данное электронное письмо и любые приложения к нему являются конфиденциальными и предназначены исключительно для адресата. Если Вы не являетесь адресатом данного письма, пожалуйста, уведомите немедленно отправителя, не раскрывайте содержание другим лицам, не используйте его в каких-либо целях, не храните и не копируйте информацию любым способом.
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
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; }