[PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Hangyu Hua]

ctx->crypto_send.info is not protected by lock_sock in
do_tls_getsockopt_conf(). A race condition between do_tls_getsockopt_conf()
and do_tls_setsockopt_conf() can cause a NULL point dereference or
use-after-free read when memcpy.

Fixes: 3c4d7559159b ("tls: kernel TLS support")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 net/tls/tls_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 3735cb00905d..4956f5149b8e 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -374,6 +374,7 @@ static int do_tls_getsockopt_conf(struct sock *sk, char __user *optval,
 	}
 
 	/* get user crypto info */
+	lock_sock(sk);
 	if (tx) {
 		crypto_info = &ctx->crypto_send.info;
 		cctx = &ctx->tx;
@@ -381,6 +382,7 @@ static int do_tls_getsockopt_conf(struct sock *sk, char __user *optval,
 		crypto_info = &ctx->crypto_recv.info;
 		cctx = &ctx->rx;
 	}
+	release_sock(sk);
 
 	if (!TLS_CRYPTO_INFO_READY(crypto_info)) {
 		rc = -EBUSY;
-- 
2.34.1

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Florian Westphal]

Hangyu Hua <hbh25y@gmail.com> wrote:
> ctx->crypto_send.info is not protected by lock_sock in
> do_tls_getsockopt_conf(). A race condition between do_tls_getsockopt_conf()
> and do_tls_setsockopt_conf() can cause a NULL point dereference or
> use-after-free read when memcpy.

Its good practice to quote the relevant parts of the splat here.

> Fixes: 3c4d7559159b ("tls: kernel TLS support")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>  net/tls/tls_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 3735cb00905d..4956f5149b8e 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -374,6 +374,7 @@ static int do_tls_getsockopt_conf(struct sock *sk, char __user *optval,
>  	}
>  
>  	/* get user crypto info */
> +	lock_sock(sk);
>  	if (tx) {
>  		crypto_info = &ctx->crypto_send.info;
>  		cctx = &ctx->tx;
> @@ -381,6 +382,7 @@ static int do_tls_getsockopt_conf(struct sock *sk, char __user *optval,
>  		crypto_info = &ctx->crypto_recv.info;
>  		cctx = &ctx->rx;
>  	}
> +	release_sock(sk);

Could you elaborate how this fixes a UAF bug?

AFAIU do_tls_setsockopt_conf uses ctx-> as scratch space, but this means
that getsockopt can see partial states.

If so, can the setsockopt part be changed so that reads only see
consistent states?

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Jakub Kicinski]

On Fri, 24 Feb 2023 13:06:06 +0100 Florian Westphal wrote:
> Hangyu Hua <hbh25y@gmail.com> wrote:
> > ctx->crypto_send.info is not protected by lock_sock in
> > do_tls_getsockopt_conf(). A race condition between do_tls_getsockopt_conf()
> > and do_tls_setsockopt_conf() can cause a NULL point dereference or
> > use-after-free read when memcpy.  
> 
> Its good practice to quote the relevant parts of the splat here.

Right, the bug and the fix seem completely bogus.
Please make sure the bugs are real and the fixes you sent actually 
fix them.

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Sabrina Dubroca]

2023-02-24, 10:55:08 -0800, Jakub Kicinski wrote:
> On Fri, 24 Feb 2023 13:06:06 +0100 Florian Westphal wrote:
> > Hangyu Hua <hbh25y@gmail.com> wrote:
> > > ctx->crypto_send.info is not protected by lock_sock in
> > > do_tls_getsockopt_conf(). A race condition between do_tls_getsockopt_conf()
> > > and do_tls_setsockopt_conf() can cause a NULL point dereference or
> > > use-after-free read when memcpy.  
> > 
> > Its good practice to quote the relevant parts of the splat here.
> 
> Right, the bug and the fix seem completely bogus.
> Please make sure the bugs are real and the fixes you sent actually 
> fix them.

I suggested a change of locking in do_tls_getsockopt_conf this
morning [1]. The issue reported last seemed valid, but this patch is not
at all what I had in mind.
[1] https://lore.kernel.org/all/Y/ht6gQL+u6fj3dG@hog/

do_tls_setsockopt_conf fills crypto_info immediately from what
userspace gives us (and clears it on exit in case of failure), which
getsockopt could see since it's not locking the socket when it checks
TLS_CRYPTO_INFO_READY. So getsockopt would progress up to the point it
finally locks the socket, but if setsockopt failed, we could have
cleared TLS_CRYPTO_INFO_READY and freed iv/rec_seq.

-- 
Sabrina

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Jakub Kicinski]

On Fri, 24 Feb 2023 21:22:43 +0100 Sabrina Dubroca wrote:
> > Right, the bug and the fix seem completely bogus.
> > Please make sure the bugs are real and the fixes you sent actually 
> > fix them.  
> 
> I suggested a change of locking in do_tls_getsockopt_conf this
> morning [1]. The issue reported last seemed valid, but this patch is not
> at all what I had in mind.
> [1] https://lore.kernel.org/all/Y/ht6gQL+u6fj3dG@hog/

Ack, I read the messages out of order, sorry.

> do_tls_setsockopt_conf fills crypto_info immediately from what
> userspace gives us (and clears it on exit in case of failure), which
> getsockopt could see since it's not locking the socket when it checks
> TLS_CRYPTO_INFO_READY. So getsockopt would progress up to the point it
> finally locks the socket, but if setsockopt failed, we could have
> cleared TLS_CRYPTO_INFO_READY and freed iv/rec_seq.

Makes sense. We should just take the socket lock around all of
do_tls_getsockopt(), then? 

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Sabrina Dubroca]

2023-02-24, 13:06:25 -0800, Jakub Kicinski wrote:
> On Fri, 24 Feb 2023 21:22:43 +0100 Sabrina Dubroca wrote:
> > > Right, the bug and the fix seem completely bogus.
> > > Please make sure the bugs are real and the fixes you sent actually 
> > > fix them.  
> > 
> > I suggested a change of locking in do_tls_getsockopt_conf this
> > morning [1]. The issue reported last seemed valid, but this patch is not
> > at all what I had in mind.
> > [1] https://lore.kernel.org/all/Y/ht6gQL+u6fj3dG@hog/
> 
> Ack, I read the messages out of order, sorry.
> 
> > do_tls_setsockopt_conf fills crypto_info immediately from what
> > userspace gives us (and clears it on exit in case of failure), which
> > getsockopt could see since it's not locking the socket when it checks
> > TLS_CRYPTO_INFO_READY. So getsockopt would progress up to the point it
> > finally locks the socket, but if setsockopt failed, we could have
> > cleared TLS_CRYPTO_INFO_READY and freed iv/rec_seq.
> 
> Makes sense. We should just take the socket lock around all of
> do_tls_getsockopt(), then? 

That would make things simple and consistent. My idea was just taking
the existing lock_sock in do_tls_getsockopt_conf out of the switch and
put it just above TLS_CRYPTO_INFO_READY.

While we're at it, should we move the

    ctx->prot_info.version != TLS_1_3_VERSION

check in do_tls_setsockopt_no_pad under lock_sock?  I don't think that
can do anything wrong (we'd have to get past this check just before a
failing setsockopt clears crypto_info, and even then we're just
reading a bit from the context), it just looks a bit strange. Or just
lock the socket around all of do_tls_setsockopt_no_pad, like the other
options we have.

-- 
Sabrina

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Jakub Kicinski]

On Fri, 24 Feb 2023 22:48:57 +0100 Sabrina Dubroca wrote:
> 2023-02-24, 13:06:25 -0800, Jakub Kicinski wrote:
> > On Fri, 24 Feb 2023 21:22:43 +0100 Sabrina Dubroca wrote:  
>  [...]  
> > > 
> > > I suggested a change of locking in do_tls_getsockopt_conf this
> > > morning [1]. The issue reported last seemed valid, but this patch is not
> > > at all what I had in mind.
> > > [1] https://lore.kernel.org/all/Y/ht6gQL+u6fj3dG@hog/  
> > 
> > Ack, I read the messages out of order, sorry.
> >   
> > > do_tls_setsockopt_conf fills crypto_info immediately from what
> > > userspace gives us (and clears it on exit in case of failure), which
> > > getsockopt could see since it's not locking the socket when it checks
> > > TLS_CRYPTO_INFO_READY. So getsockopt would progress up to the point it
> > > finally locks the socket, but if setsockopt failed, we could have
> > > cleared TLS_CRYPTO_INFO_READY and freed iv/rec_seq.  
> > 
> > Makes sense. We should just take the socket lock around all of
> > do_tls_getsockopt(), then?   
> 
> That would make things simple and consistent. My idea was just taking
> the existing lock_sock in do_tls_getsockopt_conf out of the switch and
> put it just above TLS_CRYPTO_INFO_READY.
> 
> While we're at it, should we move the
> 
>     ctx->prot_info.version != TLS_1_3_VERSION
> 
> check in do_tls_setsockopt_no_pad under lock_sock?

Yes, or READ_ONCE(), same for do_tls_getsockopt_tx_zc() and its access
on ctx->zerocopy_sendfile.

>  I don't think that
> can do anything wrong (we'd have to get past this check just before a
> failing setsockopt clears crypto_info, and even then we're just
> reading a bit from the context), it just looks a bit strange. Or just
> lock the socket around all of do_tls_setsockopt_no_pad, like the other
> options we have.

The delayed locking feels like a premature optimization, we'll keep
having such issues with new options. Hence my vote to lock all of
do_tls_getsockopt().

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Hangyu Hua]

On 25/2/2023 06:17, Jakub Kicinski wrote:
> On Fri, 24 Feb 2023 22:48:57 +0100 Sabrina Dubroca wrote:
>> 2023-02-24, 13:06:25 -0800, Jakub Kicinski wrote:
>>> On Fri, 24 Feb 2023 21:22:43 +0100 Sabrina Dubroca wrote:
>>   [...]
>>>>
>>>> I suggested a change of locking in do_tls_getsockopt_conf this
>>>> morning [1]. The issue reported last seemed valid, but this patch is not
>>>> at all what I had in mind.
>>>> [1] https://lore.kernel.org/all/Y/ht6gQL+u6fj3dG@hog/
>>>
>>> Ack, I read the messages out of order, sorry.
>>>    
>>>> do_tls_setsockopt_conf fills crypto_info immediately from what
>>>> userspace gives us (and clears it on exit in case of failure), which
>>>> getsockopt could see since it's not locking the socket when it checks
>>>> TLS_CRYPTO_INFO_READY. So getsockopt would progress up to the point it
>>>> finally locks the socket, but if setsockopt failed, we could have
>>>> cleared TLS_CRYPTO_INFO_READY and freed iv/rec_seq.
>>>
>>> Makes sense. We should just take the socket lock around all of
>>> do_tls_getsockopt(), then?
>>
>> That would make things simple and consistent. My idea was just taking
>> the existing lock_sock in do_tls_getsockopt_conf out of the switch and
>> put it just above TLS_CRYPTO_INFO_READY.

I know what you mean. I just think lock crypto_info can fix this simply.

The original situation is:

thread1				thread2(do_tls_getsockopt_conf)

lock_sock(sk)
do_tls_setsockopt_conf(crypto_info->cipher_type set)

				crypto_info = xxx
				cctx = &ctx->tx
				if(!TLS_CRYPTO_INFO_READY(crypto_info))
				
tls_set_device_offload(kmalloc cctx->iv)
tls_set_sw_offload(fail and cctx->iv may not set to NULL)
do_tls_setsockopt_conf(set crypto_info->cipher_type to NULL)
release_sock(sk)

				lock_sock(sk)
				memcpy(xxx, cctx->iv, xxx)
				release_sock(sk)

If we lock crypto_info:

thread1				thread2(do_tls_getsockopt_conf)

lock_sock(sk)
do_tls_setsockopt_conf(crypto_info->cipher_type set)			
tls_set_device_offload(kmalloc cctx->iv)
tls_set_sw_offload(fail and cctx->iv may not set to NULL)
do_tls_setsockopt_conf(set crypto_info->cipher_type to NULL)
release_sock(sk)

				lock_sock(sk)
				crypto_info = xxx
				cctx = &ctx->tx
				release_sock(sk)
				if(!TLS_CRYPTO_INFO_READY(crypto_info))
				lock_sock(sk)
				memcpy(xxx, cctx->iv, xxx)
				release_sock(sk)

>>
>> While we're at it, should we move the
>>
>>      ctx->prot_info.version != TLS_1_3_VERSION
>>
>> check in do_tls_setsockopt_no_pad under lock_sock?
> 
> Yes, or READ_ONCE(), same for do_tls_getsockopt_tx_zc() and its access
> on ctx->zerocopy_sendfile.
> 
>>   I don't think that
>> can do anything wrong (we'd have to get past this check just before a
>> failing setsockopt clears crypto_info, and even then we're just
>> reading a bit from the context), it just looks a bit strange. Or just
>> lock the socket around all of do_tls_setsockopt_no_pad, like the other
>> options we have.
> 
> The delayed locking feels like a premature optimization, we'll keep
> having such issues with new options. Hence my vote to lock all of
> do_tls_getsockopt().

In order to reduce ambiguity, I think it may be a good idea only to
lock do_tls_getsockopt_conf() like we did in do_tls_setsockopt()

It will look like:

static int do_tls_getsockopt(struct sock *sk, int optname,
			     char __user *optval, int __user *optlen)
{
	int rc = 0;

	switch (optname) {
	case TLS_TX:
	case TLS_RX:
+		lock_sock(sk);
		rc = do_tls_getsockopt_conf(sk, optval, optlen,
					    optname == TLS_TX);
+		release_sock(sk);
		break;
	case TLS_TX_ZEROCOPY_RO:
		rc = do_tls_getsockopt_tx_zc(sk, optval, optlen);
		break;
	case TLS_RX_EXPECT_NO_PAD:
		rc = do_tls_getsockopt_no_pad(sk, optval, optlen);
		break;
	default:
		rc = -ENOPROTOOPT;
		break;
	}
	return rc;
}

Of cause, I will clean the lock in do_tls_getsockopt_conf(). What do you
guys think?

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Jakub Kicinski]

On Mon, 27 Feb 2023 11:26:18 +0800 Hangyu Hua wrote:
> In order to reduce ambiguity, I think it may be a good idea only to
> lock do_tls_getsockopt_conf() like we did in do_tls_setsockopt()
> 
> It will look like:
> 
> static int do_tls_getsockopt(struct sock *sk, int optname,
> 			     char __user *optval, int __user *optlen)
> {
> 	int rc = 0;
> 
> 	switch (optname) {
> 	case TLS_TX:
> 	case TLS_RX:
> +		lock_sock(sk);
> 		rc = do_tls_getsockopt_conf(sk, optval, optlen,
> 					    optname == TLS_TX);
> +		release_sock(sk);
> 		break;
> 	case TLS_TX_ZEROCOPY_RO:
> 		rc = do_tls_getsockopt_tx_zc(sk, optval, optlen);
> 		break;
> 	case TLS_RX_EXPECT_NO_PAD:
> 		rc = do_tls_getsockopt_no_pad(sk, optval, optlen);
> 		break;
> 	default:
> 		rc = -ENOPROTOOPT;
> 		break;
> 	}
> 	return rc;
> }
> 
> Of cause, I will clean the lock in do_tls_getsockopt_conf(). What do you
> guys think?

I'd suggest to take the lock around the entire switch statement.

Re: [PATCH] net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf()

[Hangyu Hua]

On 28/2/2023 03:07, Jakub Kicinski wrote:
> On Mon, 27 Feb 2023 11:26:18 +0800 Hangyu Hua wrote:
>> In order to reduce ambiguity, I think it may be a good idea only to
>> lock do_tls_getsockopt_conf() like we did in do_tls_setsockopt()
>>
>> It will look like:
>>
>> static int do_tls_getsockopt(struct sock *sk, int optname,
>> 			     char __user *optval, int __user *optlen)
>> {
>> 	int rc = 0;
>>
>> 	switch (optname) {
>> 	case TLS_TX:
>> 	case TLS_RX:
>> +		lock_sock(sk);
>> 		rc = do_tls_getsockopt_conf(sk, optval, optlen,
>> 					    optname == TLS_TX);
>> +		release_sock(sk);
>> 		break;
>> 	case TLS_TX_ZEROCOPY_RO:
>> 		rc = do_tls_getsockopt_tx_zc(sk, optval, optlen);
>> 		break;
>> 	case TLS_RX_EXPECT_NO_PAD:
>> 		rc = do_tls_getsockopt_no_pad(sk, optval, optlen);
>> 		break;
>> 	default:
>> 		rc = -ENOPROTOOPT;
>> 		break;
>> 	}
>> 	return rc;
>> }
>>
>> Of cause, I will clean the lock in do_tls_getsockopt_conf(). What do you
>> guys think?
> 
> I'd suggest to take the lock around the entire switch statement.

I get it. I will send a v2 later.

Thanks,
Hangyu