[PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[Liu Jian]

I got the below warning when do fuzzing test:
BUG: KASAN: null-ptr-deref in scatterwalk_copychunks+0x320/0x470
Read of size 4 at addr 0000000000000008 by task kworker/u8:1/9

CPU: 0 PID: 9 Comm: kworker/u8:1 Tainted: G           OE
Hardware name: linux,dummy-virt (DT)
Workqueue: pencrypt_parallel padata_parallel_worker
Call trace:
 dump_backtrace+0x0/0x420
 show_stack+0x34/0x44
 dump_stack+0x1d0/0x248
 __kasan_report+0x138/0x140
 kasan_report+0x44/0x6c
 __asan_load4+0x94/0xd0
 scatterwalk_copychunks+0x320/0x470
 skcipher_next_slow+0x14c/0x290
 skcipher_walk_next+0x2fc/0x480
 skcipher_walk_first+0x9c/0x110
 skcipher_walk_aead_common+0x380/0x440
 skcipher_walk_aead_encrypt+0x54/0x70
 ccm_encrypt+0x13c/0x4d0
 crypto_aead_encrypt+0x7c/0xfc
 pcrypt_aead_enc+0x28/0x84
 padata_parallel_worker+0xd0/0x2dc
 process_one_work+0x49c/0xbdc
 worker_thread+0x124/0x880
 kthread+0x210/0x260
 ret_from_fork+0x10/0x18

This is because the value of rec_seq of tls_crypto_info configured by the
user program is too large, for example, 0xffffffffffffff. In addition, TLS
is asynchronously accelerated. When tls_do_encryption() returns
-EINPROGRESS and sk->sk_err is set to EBADMSG due to rec_seq overflow,
skmsg is released before the asynchronous encryption process ends. As a
result, the UAF problem occurs during the asynchronous processing of the
encryption module.

I didn't see the rec_seq overflow causing other problems, so let's get rid
of the overflow error here.

Fixes: 635d93981786 ("net/tls: free record only on encryption error")
Signed-off-by: Liu Jian <liujian56@huawei.com>
---
 net/tls/tls.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tls/tls.h b/net/tls/tls.h
index 28a8c0e80e3c..3f0e10df8053 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -304,8 +304,7 @@ static inline void
 tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
 		      struct cipher_context *ctx)
 {
-	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
-		tls_err_abort(sk, -EBADMSG);
+	tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);
 
 	if (prot->version != TLS_1_3_VERSION &&
 	    prot->cipher_type != TLS_CIPHER_CHACHA20_POLY1305)
-- 
2.34.1

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[Sabrina Dubroca]

2023-09-06, 14:52:37 +0800, Liu Jian wrote:
> This is because the value of rec_seq of tls_crypto_info configured by the
> user program is too large, for example, 0xffffffffffffff. In addition, TLS
> is asynchronously accelerated. When tls_do_encryption() returns
> -EINPROGRESS and sk->sk_err is set to EBADMSG due to rec_seq overflow,
> skmsg is released before the asynchronous encryption process ends. As a
> result, the UAF problem occurs during the asynchronous processing of the
> encryption module.
> 
> I didn't see the rec_seq overflow causing other problems, so let's get rid
> of the overflow error here.
> 
> Fixes: 635d93981786 ("net/tls: free record only on encryption error")
> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---
>  net/tls/tls.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 28a8c0e80e3c..3f0e10df8053 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h
> @@ -304,8 +304,7 @@ static inline void
>  tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
>  		      struct cipher_context *ctx)
>  {
> -	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
> -		tls_err_abort(sk, -EBADMSG);
> +	tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);

That seems wrong. We can't allow the record number to wrap, if breaks
the crypto. See for example:
https://datatracker.ietf.org/doc/html/rfc5288#section-6.1

The real fix would be to stop the caller from freeing the skmsg and
record if we go async. Once we go through async crypto, the record etc
don't belong to the caller anymore, they've been transfered to the
async callback. I'd say we need both tests in bpf_exec_tx_verdict:
-EINPROGRESS (from before 635d93981786) and EBADMSG (from
635d93981786).

Actually we need to check for both -EINPROGRESS and -EBUSY as I've
recently found out.

I've been running the selftests with async crypto and have collected a
few fixes that I was going to post this week (but not this one, since
we don't have a selftest for wrapping rec_seq). One of the patches
adds -EBUSY checks for all existing -EINPROGRESS, since the crypto API
can return -EBUSY as well if we're going through the backlog queue.

-- 
Sabrina

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[Jakub Kicinski]

On Wed, 6 Sep 2023 13:02:37 +0200 Sabrina Dubroca wrote:
> I've been running the selftests with async crypto and have collected a
> few fixes that I was going to post this week (but not this one, since
> we don't have a selftest for wrapping rec_seq). One of the patches
> adds -EBUSY checks for all existing -EINPROGRESS, since the crypto API
> can return -EBUSY as well if we're going through the backlog queue.

BTW is it possible to fake async crypto for a test or does one need 
to have an actual accelerator?

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[Sabrina Dubroca]

2023-09-06, 08:02:31 -0700, Jakub Kicinski wrote:
> On Wed, 6 Sep 2023 13:02:37 +0200 Sabrina Dubroca wrote:
> > I've been running the selftests with async crypto and have collected a
> > few fixes that I was going to post this week (but not this one, since
> > we don't have a selftest for wrapping rec_seq). One of the patches
> > adds -EBUSY checks for all existing -EINPROGRESS, since the crypto API
> > can return -EBUSY as well if we're going through the backlog queue.
> 
> BTW is it possible to fake async crypto for a test or does one need 
> to have an actual accelerator?

That's what I did for my tests, forcing AESNI to go async. I'm going
to send my changes as RFC to linux-crypto@. I think syzbot would find
a few more bugs if they let it loose with forced async crypto.

Short version (without the debugfs toggles):

diff --git a/crypto/simd.c b/crypto/simd.c
index edaa479a1ec5..e3f3bf31fcca 100644
--- a/crypto/simd.c
+++ b/crypto/simd.c
@@ -317,7 +317,7 @@ static int simd_aead_encrypt(struct aead_request *req)
 	subreq = aead_request_ctx(req);
 	*subreq = *req;
 
-	if (!crypto_simd_usable() ||
+	if (true /* force async */ || !crypto_simd_usable() ||
 	    (in_atomic() && cryptd_aead_queued(ctx->cryptd_tfm)))
 		child = &ctx->cryptd_tfm->base;
 	else
@@ -338,7 +338,7 @@ static int simd_aead_decrypt(struct aead_request *req)
 	subreq = aead_request_ctx(req);
 	*subreq = *req;
 
-	if (!crypto_simd_usable() ||
+	if (true /* force async */ || !crypto_simd_usable() ||
 	    (in_atomic() && cryptd_aead_queued(ctx->cryptd_tfm)))
 		child = &ctx->cryptd_tfm->base;
 	else


-- 
Sabrina

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[liujian (CE)]

On 2023/9/6 19:02, Sabrina Dubroca wrote:
> 2023-09-06, 14:52:37 +0800, Liu Jian wrote:
>> This is because the value of rec_seq of tls_crypto_info configured by the
>> user program is too large, for example, 0xffffffffffffff. In addition, TLS
>> is asynchronously accelerated. When tls_do_encryption() returns
>> -EINPROGRESS and sk->sk_err is set to EBADMSG due to rec_seq overflow,
>> skmsg is released before the asynchronous encryption process ends. As a
>> result, the UAF problem occurs during the asynchronous processing of the
>> encryption module.
>>
>> I didn't see the rec_seq overflow causing other problems, so let's get rid
>> of the overflow error here.
>>
>> Fixes: 635d93981786 ("net/tls: free record only on encryption error")
>> Signed-off-by: Liu Jian <liujian56@huawei.com>
>> ---
>>   net/tls/tls.h | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/tls/tls.h b/net/tls/tls.h
>> index 28a8c0e80e3c..3f0e10df8053 100644
>> --- a/net/tls/tls.h
>> +++ b/net/tls/tls.h
>> @@ -304,8 +304,7 @@ static inline void
>>   tls_advance_record_sn(struct sock *sk, struct tls_prot_info *prot,
>>   		      struct cipher_context *ctx)
>>   {
>> -	if (tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size))
>> -		tls_err_abort(sk, -EBADMSG);
>> +	tls_bigint_increment(ctx->rec_seq, prot->rec_seq_size);
> 
> That seems wrong. We can't allow the record number to wrap, if breaks
> the crypto. See for example:
> https://datatracker.ietf.org/doc/html/rfc5288#section-6.1
> 
> The real fix would be to stop the caller from freeing the skmsg and
> record if we go async. Once we go through async crypto, the record etc
> don't belong to the caller anymore, they've been transfered to the
> async callback. I'd say we need both tests in bpf_exec_tx_verdict:
> -EINPROGRESS (from before 635d93981786) and EBADMSG (from
> 635d93981786).

Thanks for your review~

By the way, does the return of EBADMSG mean that the tls link needs to 
renegotiate the encryption information or re-establish the link?

And is this okay?
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1ed4a611631f..d1fc295b83b5 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -817,7 +817,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, 
struct sock *sk,
         psock = sk_psock_get(sk);
         if (!psock || !policy) {
                 err = tls_push_record(sk, flags, record_type);
-               if (err && sk->sk_err == EBADMSG) {
+               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
                         *copied -= sk_msg_free(sk, msg);
                         tls_free_open_rec(sk);
                         err = -sk->sk_err;
@@ -846,7 +846,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, 
struct sock *sk,
         switch (psock->eval) {
         case __SK_PASS:
                 err = tls_push_record(sk, flags, record_type);
-               if (err && sk->sk_err == EBADMSG) {
+               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
                         *copied -= sk_msg_free(sk, msg);
                         tls_free_open_rec(sk);
                         err = -sk->sk_err;

> 
> Actually we need to check for both -EINPROGRESS and -EBUSY as I've
> recently found out.
> 
> I've been running the selftests with async crypto and have collected a
> few fixes that I was going to post this week (but not this one, since
> we don't have a selftest for wrapping rec_seq). One of the patches
> adds -EBUSY checks for all existing -EINPROGRESS, since the crypto API
> can return -EBUSY as well if we're going through the backlog queue.
> 

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[Sabrina Dubroca]

2023-09-07, 20:59:51 +0800, liujian (CE) wrote:
> By the way, does the return of EBADMSG mean that the tls link needs to
> renegotiate the encryption information or re-establish the link?

We currently don't support key updates so closing this socket is the
only option for now. AFAIU when we set EBADMSG, we can't fix that socket.

> And is this okay?

Yes, this is what I had in mind.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 1ed4a611631f..d1fc295b83b5 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -817,7 +817,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg,
> struct sock *sk,
>         psock = sk_psock_get(sk);
>         if (!psock || !policy) {
>                 err = tls_push_record(sk, flags, record_type);
> -               if (err && sk->sk_err == EBADMSG) {
> +               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
>                         *copied -= sk_msg_free(sk, msg);
>                         tls_free_open_rec(sk);
>                         err = -sk->sk_err;
> @@ -846,7 +846,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg,
> struct sock *sk,
>         switch (psock->eval) {
>         case __SK_PASS:
>                 err = tls_push_record(sk, flags, record_type);
> -               if (err && sk->sk_err == EBADMSG) {
> +               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
>                         *copied -= sk_msg_free(sk, msg);
>                         tls_free_open_rec(sk);
>                         err = -sk->sk_err;

-- 
Sabrina

Re: [PATCH net] tls: do not return error when the tls_bigint overflows in tls_advance_record_sn()

[liujian (CE)]

On 2023/9/9 0:41, Sabrina Dubroca wrote:
> 2023-09-07, 20:59:51 +0800, liujian (CE) wrote:
>> By the way, does the return of EBADMSG mean that the tls link needs to
>> renegotiate the encryption information or re-establish the link?
> 
> We currently don't support key updates so closing this socket is the
> only option for now. AFAIU when we set EBADMSG, we can't fix that socket.
> 
Got it, thank you for your explanation.

>> And is this okay?
> 
> Yes, this is what I had in mind.
> 
Will send v2, thanks.

>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index 1ed4a611631f..d1fc295b83b5 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -817,7 +817,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg,
>> struct sock *sk,
>>          psock = sk_psock_get(sk);
>>          if (!psock || !policy) {
>>                  err = tls_push_record(sk, flags, record_type);
>> -               if (err && sk->sk_err == EBADMSG) {
>> +               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
>>                          *copied -= sk_msg_free(sk, msg);
>>                          tls_free_open_rec(sk);
>>                          err = -sk->sk_err;
>> @@ -846,7 +846,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg,
>> struct sock *sk,
>>          switch (psock->eval) {
>>          case __SK_PASS:
>>                  err = tls_push_record(sk, flags, record_type);
>> -               if (err && sk->sk_err == EBADMSG) {
>> +               if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
>>                          *copied -= sk_msg_free(sk, msg);
>>                          tls_free_open_rec(sk);
>>                          err = -sk->sk_err;
>