diff mbox series

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

Message ID 20230224105811.27467-1-hbh25y@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: tls: fix possible race condition between do_tls_getsockopt_conf() and do_tls_setsockopt_conf() | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Hangyu Hua Feb. 24, 2023, 10:58 a.m. UTC
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(+)

Comments

Florian Westphal Feb. 24, 2023, 12:06 p.m. UTC | #1
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?
Jakub Kicinski Feb. 24, 2023, 6:55 p.m. UTC | #2
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.
Sabrina Dubroca Feb. 24, 2023, 8:22 p.m. UTC | #3
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.
Jakub Kicinski Feb. 24, 2023, 9:06 p.m. UTC | #4
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?
Sabrina Dubroca Feb. 24, 2023, 9:48 p.m. UTC | #5
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.
Jakub Kicinski Feb. 24, 2023, 10:17 p.m. UTC | #6
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().
Hangyu Hua Feb. 27, 2023, 3:26 a.m. UTC | #7
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?
Jakub Kicinski Feb. 27, 2023, 7:07 p.m. UTC | #8
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.
Hangyu Hua Feb. 28, 2023, 1:48 a.m. UTC | #9
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
diff mbox series

Patch

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;