| 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 |
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?
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.
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.
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?
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.
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().
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?
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.
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 --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;
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(+)