LinuxLists.cc - kernel tainted while exporting shash context using af

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Am Sonntag, 25. Oktober 2015, 11:56:27 schrieb Harsh Jain:

Hi Harsh,

>Hi,
>
>
>When trying to calculate HMAC(SHA1) with openssl using af-alg engine
>kernel crashes.Find below the command used and kernel. I have added
>some debug print in logs.
>
>Command used : ./openssl dgst -engine af_alg -sha1 -hmac "key" r.txt
>kernel version : 3.17.8
>
>Initial Investigation : In shash_desc structure tfm pointer contains
>5a5a5a5a5a5a5a5a(invalid pointer), when it triess to reference export
>function pointer it crashes. As per my understanding tfm object should
>have pointer of memory bloack allocated in
>"crypto_init_shash_ops_async" function
>
>Please give some pointers to debug the issue.Any documentation to
>understand the crypto-api code.

May I ask you to send 2 things: the source code of the OpenSSL af_alg engine
that you use (IIRC it is not included upstream, I want to be sure I used the
right one).

Further, can you attach an strace of the aforementioned command?

Note, I am playing and abusing the AF_ALG interface for quite some time with
[1] but I did not come across any issues like the one you describe here.

[1] http://www.chronox.de/libkcapi.html

Ciao
Stephan

2015-10-26 06:19:12

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Hi Stephan,

I also tried test program in libkcapi and it works. libkcapi opens
socket of type "hmac(sha1)" .Openssl opens multiple "sha1" type socket
and uses the partial results to calculate hmac.

"crypto_ahash_init()" function initialises the *tfm variable in
crypto_shash structure.It gets called when user calls write() system
call. To give a try I updated the hash_accept() function and re-run.
This time kernel didn't crashed but result calculated is wrong.

How accept() sys call decide weather to call alg_accept() or hash_accept()?

Find attached patch and strace.Right now af_alg code is not accessible
to me. I will share it tomorrow.

Thanks and Regards
Harsh jain

On Sun, Oct 25, 2015 at 5:28 PM, Stephan Mueller <[email protected]> wrote:
> Am Sonntag, 25. Oktober 2015, 11:56:27 schrieb Harsh Jain:
>
> Hi Harsh,
>
>>Hi,
>>
>>
>>When trying to calculate HMAC(SHA1) with openssl using af-alg engine
>>kernel crashes.Find below the command used and kernel. I have added
>>some debug print in logs.
>>
>>Command used : ./openssl dgst -engine af_alg -sha1 -hmac "key" r.txt
>>kernel version : 3.17.8
>>
>>Initial Investigation : In shash_desc structure tfm pointer contains
>>5a5a5a5a5a5a5a5a(invalid pointer), when it triess to reference export
>>function pointer it crashes. As per my understanding tfm object should
>>have pointer of memory bloack allocated in
>>"crypto_init_shash_ops_async" function
>>
>>Please give some pointers to debug the issue.Any documentation to
>>understand the crypto-api code.
>
> May I ask you to send 2 things: the source code of the OpenSSL af_alg engine
> that you use (IIRC it is not included upstream, I want to be sure I used the
> right one).
>
> Further, can you attach an strace of the aforementioned command?
>
> Note, I am playing and abusing the AF_ALG interface for quite some time with
> [1] but I did not come across any issues like the one you describe here.
>
> [1] http://www.chronox.de/libkcapi.html
>
> Ciao
> Stephan

Attachments:

algif_hash.patch (506.00 B)
strace.log (8.88 kB)
Download all attachments

2015-10-26 09:21:03

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Hi Stephan,

I tried 1 more patch. This time result is correct. Find attached patch
file. Is there any side effect of this patch.

Regards
Harsh Jain

On Mon, Oct 26, 2015 at 11:49 AM, Harsh Jain <[email protected]> wrote:
> Hi Stephan,
>
> I also tried test program in libkcapi and it works. libkcapi opens
> socket of type "hmac(sha1)" .Openssl opens multiple "sha1" type socket
> and uses the partial results to calculate hmac.
>
> "crypto_ahash_init()" function initialises the *tfm variable in
> crypto_shash structure.It gets called when user calls write() system
> call. To give a try I updated the hash_accept() function and re-run.
> This time kernel didn't crashed but result calculated is wrong.
>
> How accept() sys call decide weather to call alg_accept() or hash_accept()?
>
> Find attached patch and strace.Right now af_alg code is not accessible
> to me. I will share it tomorrow.
>
>
>
> Thanks and Regards
> Harsh jain
>
> On Sun, Oct 25, 2015 at 5:28 PM, Stephan Mueller <[email protected]> wrote:
>> Am Sonntag, 25. Oktober 2015, 11:56:27 schrieb Harsh Jain:
>>
>> Hi Harsh,
>>
>>>Hi,
>>>
>>>
>>>When trying to calculate HMAC(SHA1) with openssl using af-alg engine
>>>kernel crashes.Find below the command used and kernel. I have added
>>>some debug print in logs.
>>>
>>>Command used : ./openssl dgst -engine af_alg -sha1 -hmac "key" r.txt
>>>kernel version : 3.17.8
>>>
>>>Initial Investigation : In shash_desc structure tfm pointer contains
>>>5a5a5a5a5a5a5a5a(invalid pointer), when it triess to reference export
>>>function pointer it crashes. As per my understanding tfm object should
>>>have pointer of memory bloack allocated in
>>>"crypto_init_shash_ops_async" function
>>>
>>>Please give some pointers to debug the issue.Any documentation to
>>>understand the crypto-api code.
>>
>> May I ask you to send 2 things: the source code of the OpenSSL af_alg engine
>> that you use (IIRC it is not included upstream, I want to be sure I used the
>> right one).
>>
>> Further, can you attach an strace of the aforementioned command?
>>
>> Note, I am playing and abusing the AF_ALG interface for quite some time with
>> [1] but I did not come across any issues like the one you describe here.
>>
>> [1] http://www.chronox.de/libkcapi.html
>>
>> Ciao
>> Stephan

Attachments:

algif_hash_2.patch (517.00 B)

2015-10-28 00:10:03

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Am Montag, 26. Oktober 2015, 14:51:01 schrieb Harsh Jain:

Hi Harsh,

> Hi Stephan,
>
> I tried 1 more patch. This time result is correct. Find attached patch
> file. Is there any side effect of this patch.

The strace is enlightening.

The user space code does an accept on an already accepted FD

It seems your user space does something like:

socket()
fd = bind()
fd1 = accept(fd)
fd2 = accept(fd1)
fd3 = accept(fd2)
...

That is an error in the user space code. The correct way would be like the
code in [1] with all the lines until the line 553 (the code afterwards is for
vmsplice).

So, the code goes like that:

tfmfd = socket()
bind(tfmfd)
opfd = accept(tfmfd);

>From now on, you use opfd for all sendmsg/recvmsg operations.

However, any error in user space should not crash the kernel. So, a fix should
be done. But I think your code is not correct as it solidifies a broken user
space code.

I would rather think the following patch should be added to prevent the oops.
At least for me, multiple accepts does not crash the kernel. Can you please
test whether this patch ensures you kernel stays sane?

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 1396ad0..785df23 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -183,6 +183,9 @@ static int hash_accept(struct socket *sock, struct socket
*newsock, int flags)
struct hash_ctx *ctx2;
int err;

+ if (!ctx->more)
+ return -EINVAL;
+
err = crypto_ahash_export(req, state);
if (err)
return err;

[1] https://github.com/smuellerDD/libkcapi/blob/master/lib/kcapi-kernel-if.c#L534

--
Ciao
Stephan

2015-10-28 00:55:18

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:

Hi Harsh,

>
>
> However, any error in user space should not crash the kernel. So, a fix
> should be done. But I think your code is not correct as it solidifies a
> broken user space code.

After thinking a bit again, I think your approach is correct after all. I was
able to reproduce the crash by simply adding more accept calls to my test
code. And I can confirm that your patch works, for hashes.

*BUT* it does NOT work for HMAC as the key is set on the TFM and the
subsequent accepts do not transport the key. Albeit your code prevents the
kernel from crashing, the HMAC calculation will be done with an empty key as
the setkey operation does not reach the TFM handle in the subordinate accept()
call.

So, I would think that the second accept is simply broken, for HMAC at least.

Herbert, what is the purpose of that subordinate accept that is implemented
with hash_accept? As this is broken for HMACs, should it be removed entirely?

--
Ciao
Stephan

2015-10-28 10:54:36

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Hi Stephan,

I tried your patch on my machine. Kernel is not crashing. The openssl
break with this. Can you share HMAC program which you are suspecting
it will not work or do you already have some test written in
libkcapi/test.sh which will fail.

Regards
Harsh Jain

On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <[email protected]> wrote:
> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:
>
> Hi Harsh,
>
>>
>>
>> However, any error in user space should not crash the kernel. So, a fix
>> should be done. But I think your code is not correct as it solidifies a
>> broken user space code.
>
> After thinking a bit again, I think your approach is correct after all. I was
> able to reproduce the crash by simply adding more accept calls to my test
> code. And I can confirm that your patch works, for hashes.
>
> *BUT* it does NOT work for HMAC as the key is set on the TFM and the
> subsequent accepts do not transport the key. Albeit your code prevents the
> kernel from crashing, the HMAC calculation will be done with an empty key as
> the setkey operation does not reach the TFM handle in the subordinate accept()
> call.
>
> So, I would think that the second accept is simply broken, for HMAC at least.
>
> Herbert, what is the purpose of that subordinate accept that is implemented
> with hash_accept? As this is broken for HMACs, should it be removed entirely?
>
> --
> Ciao
> Stephan

2015-10-28 11:24:00

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain:

Hi Harsh,

>Hi Stephan,
>
>I tried your patch on my machine. Kernel is not crashing. The openssl
>break with this. Can you share HMAC program which you are suspecting
>it will not work or do you already have some test written in
>libkcapi/test.sh which will fail.

See comments above test/kcapi-main.c:cavs_hash

* HMAC command line invocation:
* $ ./kcapi -x 3 -c "hmac(sha1)" -k 6e77ebd479da794707bc6cde3694f552ea892dab
-p
31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0fe63c599365e04d596c05736beaab58
* 7f204ea665666f5bd2b370e546d1b408005e4d85

To do that, apply your patch and then

1. open lib/kcapi-kernel-if.c and change line 567 from

handle->opfd = accept(handle->tfmfd, NULL, 0);

to

handle->opfd = accept(handle->tfmfd, NULL, 0);
handle->opfd = accept(handle->opfd, NULL, 0);
handle->opfd = accept(handle->opfd, NULL, 0);
handle->opfd = accept(handle->opfd, NULL, 0);
handle->opfd = accept(handle->opfd, NULL, 0);

You will see that the hash commands will pass, the HMAC fails

Without your patch, the kernel crashes (same as with your OpenSSL code).

The reason is that setkey is applied on the TFM that is not conveyed to the
subsequent TFMs generated with new accepts.
>
>
>Regards
>Harsh Jain
>
>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <[email protected]> wrote:
>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:
>>
>> Hi Harsh,
>>
>>> However, any error in user space should not crash the kernel. So, a fix
>>> should be done. But I think your code is not correct as it solidifies a
>>> broken user space code.
>>
>> After thinking a bit again, I think your approach is correct after all. I
>> was able to reproduce the crash by simply adding more accept calls to my
>> test code. And I can confirm that your patch works, for hashes.
>>
>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the
>> subsequent accepts do not transport the key. Albeit your code prevents the
>> kernel from crashing, the HMAC calculation will be done with an empty key
>> as
>> the setkey operation does not reach the TFM handle in the subordinate
>> accept() call.
>>
>> So, I would think that the second accept is simply broken, for HMAC at
>> least.
>>
>> Herbert, what is the purpose of that subordinate accept that is implemented
>> with hash_accept? As this is broken for HMACs, should it be removed
>> entirely?
>>
>> --
>> Ciao
>> Stephan
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html

Ciao
Stephan

2015-10-30 08:32:29

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Hi Stephan,

If we add sendmsg() in between 2 accept calls then the setkey problem
will happen?

handle->opfd = accept(handle->tfmfd, NULL, 0);
sendmsg()
handle->opfd = accept(handle->opfd, NULL, 0);
sendmsg()
handle->opfd = accept(handle->opfd, NULL, 0);

If yes, Then may be it is expected behavior and user is supposed to
set the key explicitly with some other system call.Why I am saying
this is. I remember somewhere in kernel code I read some comment
related to setkey operations.

In that case my patch should work. 1 doubt I have related to patch is
do I need to set "ctx->more" =1 after initialisation.

Correct me If I am wrong.

Thanks for your support.

regards
Harsh Jain

On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <[email protected]> wrote:
> Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain:
>
> Hi Harsh,
>
>>Hi Stephan,
>>
>>I tried your patch on my machine. Kernel is not crashing. The openssl
>>break with this. Can you share HMAC program which you are suspecting
>>it will not work or do you already have some test written in
>>libkcapi/test.sh which will fail.
>
> See comments above test/kcapi-main.c:cavs_hash
>
> * HMAC command line invocation:
> * $ ./kcapi -x 3 -c "hmac(sha1)" -k 6e77ebd479da794707bc6cde3694f552ea892dab
> -p
> 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0fe63c599365e04d596c05736beaab58
> * 7f204ea665666f5bd2b370e546d1b408005e4d85
>
> To do that, apply your patch and then
>
> 1. open lib/kcapi-kernel-if.c and change line 567 from
>
> handle->opfd = accept(handle->tfmfd, NULL, 0);
>
>
> to
>
> handle->opfd = accept(handle->tfmfd, NULL, 0);
> handle->opfd = accept(handle->opfd, NULL, 0);
> handle->opfd = accept(handle->opfd, NULL, 0);
> handle->opfd = accept(handle->opfd, NULL, 0);
> handle->opfd = accept(handle->opfd, NULL, 0);
>
> You will see that the hash commands will pass, the HMAC fails
>
> Without your patch, the kernel crashes (same as with your OpenSSL code).
>
> The reason is that setkey is applied on the TFM that is not conveyed to the
> subsequent TFMs generated with new accepts.
>>
>>
>>Regards
>>Harsh Jain
>>
>>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <[email protected]> wrote:
>>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:
>>>
>>> Hi Harsh,
>>>
>>>> However, any error in user space should not crash the kernel. So, a fix
>>>> should be done. But I think your code is not correct as it solidifies a
>>>> broken user space code.
>>>
>>> After thinking a bit again, I think your approach is correct after all. I
>>> was able to reproduce the crash by simply adding more accept calls to my
>>> test code. And I can confirm that your patch works, for hashes.
>>>
>>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the
>>> subsequent accepts do not transport the key. Albeit your code prevents the
>>> kernel from crashing, the HMAC calculation will be done with an empty key
>>> as
>>> the setkey operation does not reach the TFM handle in the subordinate
>>> accept() call.
>>>
>>> So, I would think that the second accept is simply broken, for HMAC at
>>> least.
>>>
>>> Herbert, what is the purpose of that subordinate accept that is implemented
>>> with hash_accept? As this is broken for HMACs, should it be removed
>>> entirely?
>>>
>>> --
>>> Ciao
>>> Stephan
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>>the body of a message to [email protected]
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> Ciao
> Stephan

2015-10-30 11:10:56

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Am Freitag, 30. Oktober 2015, 14:02:27 schrieb Harsh Jain:

Hi Harsh,

>Hi Stephan,
>
>If we add sendmsg() in between 2 accept calls then the setkey problem
>will happen?
>
>handle->opfd = accept(handle->tfmfd, NULL, 0);
>sendmsg()
>handle->opfd = accept(handle->opfd, NULL, 0);
>sendmsg()
>handle->opfd = accept(handle->opfd, NULL, 0);

Without testing, I would very much expect that, because the setkey does not
apply to the subordinate tfm.
>
>If yes, Then may be it is expected behavior and user is supposed to
>set the key explicitly with some other system call.Why I am saying
>this is. I remember somewhere in kernel code I read some comment
>related to setkey operations.

I would like to wait for Herbert to chime in here on how he thinks this would
work.
>
>In that case my patch should work. 1 doubt I have related to patch is
>do I need to set "ctx->more" =1 after initialisation.
>
>Correct me If I am wrong.
>
>
>Thanks for your support.
>
>
>regards
>Harsh Jain
>
>On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <[email protected]> wrote:
>> Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain:
>>
>> Hi Harsh,
>>
>>>Hi Stephan,
>>>
>>>I tried your patch on my machine. Kernel is not crashing. The openssl
>>>break with this. Can you share HMAC program which you are suspecting
>>>it will not work or do you already have some test written in
>>>libkcapi/test.sh which will fail.
>>>
>> See comments above test/kcapi-main.c:cavs_hash
>>
>> * HMAC command line invocation:
>> * $ ./kcapi -x 3 -c "hmac(sha1)" -k
>> 6e77ebd479da794707bc6cde3694f552ea892dab
>>
>> -p
>> 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd
>> 4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c
>> 55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0
>> fe63c599365e04d596c05736beaab58>
>> * 7f204ea665666f5bd2b370e546d1b408005e4d85
>>
>> To do that, apply your patch and then
>>
>> 1. open lib/kcapi-kernel-if.c and change line 567 from
>>
>> handle->opfd = accept(handle->tfmfd, NULL, 0);
>>
>>
>> to
>>
>> handle->opfd = accept(handle->tfmfd, NULL, 0);
>> handle->opfd = accept(handle->opfd, NULL, 0);
>> handle->opfd = accept(handle->opfd, NULL, 0);
>> handle->opfd = accept(handle->opfd, NULL, 0);
>> handle->opfd = accept(handle->opfd, NULL, 0);
>>
>> You will see that the hash commands will pass, the HMAC fails
>>
>> Without your patch, the kernel crashes (same as with your OpenSSL code).
>>
>> The reason is that setkey is applied on the TFM that is not conveyed to the
>> subsequent TFMs generated with new accepts.
>>
>>>Regards
>>>Harsh Jain
>>>
>>>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <[email protected]>
wrote:
>>>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:
>>>>
>>>> Hi Harsh,
>>>>
>>>>> However, any error in user space should not crash the kernel. So, a fix
>>>>> should be done. But I think your code is not correct as it solidifies a
>>>>> broken user space code.
>>>>
>>>> After thinking a bit again, I think your approach is correct after all. I
>>>> was able to reproduce the crash by simply adding more accept calls to my
>>>> test code. And I can confirm that your patch works, for hashes.
>>>>
>>>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the
>>>> subsequent accepts do not transport the key. Albeit your code prevents
>>>> the
>>>> kernel from crashing, the HMAC calculation will be done with an empty key
>>>> as
>>>> the setkey operation does not reach the TFM handle in the subordinate
>>>> accept() call.
>>>>
>>>> So, I would think that the second accept is simply broken, for HMAC at
>>>> least.
>>>>
>>>> Herbert, what is the purpose of that subordinate accept that is
>>>> implemented
>>>> with hash_accept? As this is broken for HMACs, should it be removed
>>>> entirely?
>>>>
>>>> --
>>>> Ciao
>>>> Stephan
>>>
>>>--
>>>To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>>>the body of a message to [email protected]
>>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>> Ciao
>> Stephan

Ciao
Stephan

2015-10-30 12:16:57

[permalink] [raw]

Subject: crypto: algif_hash - Only export and import on sockets with data

On Fri, Oct 30, 2015 at 12:10:51PM +0100, Stephan Mueller wrote:
> Am Freitag, 30. Oktober 2015, 14:02:27 schrieb Harsh Jain:
>
> Hi Harsh,
>
> >Hi Stephan,
> >
> >If we add sendmsg() in between 2 accept calls then the setkey problem
> >will happen?
> >
> >handle->opfd = accept(handle->tfmfd, NULL, 0);
> >sendmsg()
> >handle->opfd = accept(handle->opfd, NULL, 0);
> >sendmsg()
> >handle->opfd = accept(handle->opfd, NULL, 0);
>
> Without testing, I would very much expect that, because the setkey does not
> apply to the subordinate tfm.

setkey should be needed as the subsequent accept will all be based
on the same parent fd, meaning that they will all use a single tfm.

Please try the following patch.

---8<---
The hash_accept call fails to work on sockets that have not received
any data. For some algorithm implementations it may cause crashes.

This patch fixes this by ensuring that we only export and import on
sockets that have received data.

Cc: [email protected]
Reported-by: Harsh Jain <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 1396ad0..bbda6b4 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -181,11 +181,14 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags)
struct sock *sk2;
struct alg_sock *ask2;
struct hash_ctx *ctx2;
+ bool more = ctx->more;
int err;

- err = crypto_ahash_export(req, state);
- if (err)
- return err;
+ if (more) {
+ err = crypto_ahash_export(req, state);
+ if (err)
+ return err;
+ }

err = af_alg_accept(ask->parent, newsock);
if (err)
@@ -194,12 +197,14 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags)
sk2 = newsock->sk;
ask2 = alg_sk(sk2);
ctx2 = ask2->private;
- ctx2->more = 1;
+ ctx2->more = more;

- err = crypto_ahash_import(&ctx2->req, state);
- if (err) {
- sock_orphan(sk2);
- sock_put(sk2);
+ if (more) {
+ err = crypto_ahash_import(&ctx2->req, state);
+ if (err) {
+ sock_orphan(sk2);
+ sock_put(sk2);
+ }
}

return err;
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-10-30 23:45:53

[permalink] [raw]

Subject: Re: crypto: algif_hash - Only export and import on sockets with data

Am Freitag, 30. Oktober 2015, 20:16:51 schrieb Herbert Xu:

Hi Herbert,

>
> setkey should be needed as the subsequent accept will all be based
> on the same parent fd, meaning that they will all use a single tfm.
>
> Please try the following patch.

Testing complete: patch solves the oops and allows to successfully perform
HMAC even when having subsequent accepts and operating on those subsequent
accepts.

--
Ciao
Stephan

2015-11-01 09:11:27

[permalink] [raw]

Subject: [PATCH v2] crypto: algif_hash - Only export and import on sockets with data

On Sat, Oct 31, 2015 at 12:45:47AM +0100, Stephan Mueller wrote:
>
> Testing complete: patch solves the oops and allows to successfully perform
> HMAC even when having subsequent accepts and operating on those subsequent
> accepts.

Thanks Stephan!

Unfortunately my patch is incomplete as some other thread could
change ctx->more while we're in the middle of the accept call.

So here is an updated version.

---8<---
The hash_accept call fails to work on sockets that have not received
any data. For some algorithm implementations it may cause crashes.

This patch fixes this by ensuring that we only export and import on
sockets that have received data.

Cc: [email protected]
Reported-by: Harsh Jain <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 1396ad0..b4c24fe 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -181,9 +181,14 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags)
struct sock *sk2;
struct alg_sock *ask2;
struct hash_ctx *ctx2;
+ bool more;
int err;

- err = crypto_ahash_export(req, state);
+ lock_sock(sk);
+ more = ctx->more;
+ err = more ? crypto_ahash_export(req, state) : 0;
+ release_sock(sk);
+
if (err)
return err;

@@ -194,7 +199,10 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags)
sk2 = newsock->sk;
ask2 = alg_sk(sk2);
ctx2 = ask2->private;
- ctx2->more = 1;
+ ctx2->more = more;
+
+ if (!more)
+ return err;

err = crypto_ahash_import(&ctx2->req, state);
if (err) {
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-11-01 11:06:29

[permalink] [raw]

Subject: Re: [PATCH v2] crypto: algif_hash - Only export and import on sockets with data

Am Sonntag, 1. November 2015, 17:11:19 schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 31, 2015 at 12:45:47AM +0100, Stephan Mueller wrote:
> > Testing complete: patch solves the oops and allows to successfully perform
> > HMAC even when having subsequent accepts and operating on those subsequent
> > accepts.
>
> Thanks Stephan!
>
> Unfortunately my patch is incomplete as some other thread could
> change ctx->more while we're in the middle of the accept call.

Tested-by: Stephan Mueller <[email protected]>

--
Ciao
Stephan

2015-11-01 11:07:16

[permalink] [raw]

Subject: Re: [PATCH v2] crypto: algif_hash - Only export and import on sockets with data

Am Sonntag, 1. November 2015, 17:11:19 schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 31, 2015 at 12:45:47AM +0100, Stephan Mueller wrote:
> > Testing complete: patch solves the oops and allows to successfully perform
> > HMAC even when having subsequent accepts and operating on those subsequent
> > accepts.
>
> Thanks Stephan!
>
> Unfortunately my patch is incomplete as some other thread could
> change ctx->more while we're in the middle of the accept call.
>
> So here is an updated version.

Shouldn't that patch to into stable (and hopefully into 4.3) as well?

--
Ciao
Stephan

2015-11-02 03:12:08

[permalink] [raw]

Subject: Re: [PATCH v2] crypto: algif_hash - Only export and import on sockets with data

On Sun, Nov 01, 2015 at 12:07:12PM +0100, Stephan Mueller wrote:
> Am Sonntag, 1. November 2015, 17:11:19 schrieb Herbert Xu:
>
> Hi Herbert,
>
> > On Sat, Oct 31, 2015 at 12:45:47AM +0100, Stephan Mueller wrote:
> > > Testing complete: patch solves the oops and allows to successfully perform
> > > HMAC even when having subsequent accepts and operating on those subsequent
> > > accepts.
> >
> > Thanks Stephan!
> >
> > Unfortunately my patch is incomplete as some other thread could
> > change ctx->more while we're in the middle of the accept call.
> >
> > So here is an updated version.
>
> Shouldn't that patch to into stable (and hopefully into 4.3) as well?

Yes it is going into stable.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-11-02 05:48:39

[permalink] [raw]

Subject: Re: kernel tainted while exporting shash context using af_alg interface

Hi,

I tried patch on my setup and its working fine.
Thanks Stephan, Herbert for your support.

Regards
Harsh Jain

On Fri, Oct 30, 2015 at 4:40 PM, Stephan Mueller <[email protected]> wrote:
> Am Freitag, 30. Oktober 2015, 14:02:27 schrieb Harsh Jain:
>
> Hi Harsh,
>
>>Hi Stephan,
>>
>>If we add sendmsg() in between 2 accept calls then the setkey problem
>>will happen?
>>
>>handle->opfd = accept(handle->tfmfd, NULL, 0);
>>sendmsg()
>>handle->opfd = accept(handle->opfd, NULL, 0);
>>sendmsg()
>>handle->opfd = accept(handle->opfd, NULL, 0);
>
> Without testing, I would very much expect that, because the setkey does not
> apply to the subordinate tfm.
>>
>>If yes, Then may be it is expected behavior and user is supposed to
>>set the key explicitly with some other system call.Why I am saying
>>this is. I remember somewhere in kernel code I read some comment
>>related to setkey operations.
>
> I would like to wait for Herbert to chime in here on how he thinks this would
> work.
>>
>>In that case my patch should work. 1 doubt I have related to patch is
>>do I need to set "ctx->more" =1 after initialisation.
>>
>>Correct me If I am wrong.
>>
>>
>>Thanks for your support.
>>
>>
>>regards
>>Harsh Jain
>>
>>On Wed, Oct 28, 2015 at 4:53 PM, Stephan Mueller <[email protected]> wrote:
>>> Am Mittwoch, 28. Oktober 2015, 16:24:34 schrieb Harsh Jain:
>>>
>>> Hi Harsh,
>>>
>>>>Hi Stephan,
>>>>
>>>>I tried your patch on my machine. Kernel is not crashing. The openssl
>>>>break with this. Can you share HMAC program which you are suspecting
>>>>it will not work or do you already have some test written in
>>>>libkcapi/test.sh which will fail.
>>>>
>>> See comments above test/kcapi-main.c:cavs_hash
>>>
>>> * HMAC command line invocation:
>>> * $ ./kcapi -x 3 -c "hmac(sha1)" -k
>>> 6e77ebd479da794707bc6cde3694f552ea892dab
>>>
>>> -p
>>> 31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd
>>> 4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c
>>> 55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0
>>> fe63c599365e04d596c05736beaab58>
>>> * 7f204ea665666f5bd2b370e546d1b408005e4d85
>>>
>>> To do that, apply your patch and then
>>>
>>> 1. open lib/kcapi-kernel-if.c and change line 567 from
>>>
>>> handle->opfd = accept(handle->tfmfd, NULL, 0);
>>>
>>>
>>> to
>>>
>>> handle->opfd = accept(handle->tfmfd, NULL, 0);
>>> handle->opfd = accept(handle->opfd, NULL, 0);
>>> handle->opfd = accept(handle->opfd, NULL, 0);
>>> handle->opfd = accept(handle->opfd, NULL, 0);
>>> handle->opfd = accept(handle->opfd, NULL, 0);
>>>
>>> You will see that the hash commands will pass, the HMAC fails
>>>
>>> Without your patch, the kernel crashes (same as with your OpenSSL code).
>>>
>>> The reason is that setkey is applied on the TFM that is not conveyed to the
>>> subsequent TFMs generated with new accepts.
>>>
>>>>Regards
>>>>Harsh Jain
>>>>
>>>>On Wed, Oct 28, 2015 at 6:25 AM, Stephan Mueller <[email protected]>
> wrote:
>>>>> Am Mittwoch, 28. Oktober 2015, 01:09:58 schrieb Stephan Mueller:
>>>>>
>>>>> Hi Harsh,
>>>>>
>>>>>> However, any error in user space should not crash the kernel. So, a fix
>>>>>> should be done. But I think your code is not correct as it solidifies a
>>>>>> broken user space code.
>>>>>
>>>>> After thinking a bit again, I think your approach is correct after all. I
>>>>> was able to reproduce the crash by simply adding more accept calls to my
>>>>> test code. And I can confirm that your patch works, for hashes.
>>>>>
>>>>> *BUT* it does NOT work for HMAC as the key is set on the TFM and the
>>>>> subsequent accepts do not transport the key. Albeit your code prevents
>>>>> the
>>>>> kernel from crashing, the HMAC calculation will be done with an empty key
>>>>> as
>>>>> the setkey operation does not reach the TFM handle in the subordinate
>>>>> accept() call.
>>>>>
>>>>> So, I would think that the second accept is simply broken, for HMAC at
>>>>> least.
>>>>>
>>>>> Herbert, what is the purpose of that subordinate accept that is
>>>>> implemented
>>>>> with hash_accept? As this is broken for HMACs, should it be removed
>>>>> entirely?
>>>>>
>>>>> --
>>>>> Ciao
>>>>> Stephan
>>>>
>>>>--
>>>>To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>>>>the body of a message to [email protected]
>>>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>> Ciao
>>> Stephan
>
>
> Ciao
> Stephan

2015-11-05 07:42:06

[permalink] [raw]

Subject: Re: [PATCH v2] crypto: algif_hash - Only export and import on sockets with data

Hi herbert,

Which kernel versions will have this patch?

Regards
Harsh Jain

On Mon, Nov 2, 2015 at 8:42 AM, Herbert Xu <[email protected]> wrote:
> On Sun, Nov 01, 2015 at 12:07:12PM +0100, Stephan Mueller wrote:
>> Am Sonntag, 1. November 2015, 17:11:19 schrieb Herbert Xu:
>>
>> Hi Herbert,
>>
>> > On Sat, Oct 31, 2015 at 12:45:47AM +0100, Stephan Mueller wrote:
>> > > Testing complete: patch solves the oops and allows to successfully perform
>> > > HMAC even when having subsequent accepts and operating on those subsequent
>> > > accepts.
>> >
>> > Thanks Stephan!
>> >
>> > Unfortunately my patch is incomplete as some other thread could
>> > change ctx->more while we're in the middle of the accept call.
>> >
>> > So here is an updated version.
>>
>> Shouldn't that patch to into stable (and hopefully into 4.3) as well?
>
> Yes it is going into stable.
>
> Cheers,
> --
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-11-05 12:19:27