After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails
before kmalloc cctx->iv. This may cause info leak when we call
do_tls_getsockopt_conf().

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 net/tls/tls_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 6c593788dc25..a63f6f727f58 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1241,8 +1241,10 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
 	kfree(start_marker_record);
 free_rec_seq:
 	kfree(ctx->tx.rec_seq);
+	ctx->tx.rec_seq = NULL;
 free_iv:
 	kfree(ctx->tx.iv);
+	ctx->tx.iv = NULL;
 release_netdev:
 	dev_put(netdev);
 	return rc;
-- 
2.34.1

2023-02-23, 17:05:08 +0800, Hangyu Hua wrote:
> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails
> before kmalloc cctx->iv. This may cause info leak when we call
> do_tls_getsockopt_conf().

Is there really an issue here?

If both tls_set_device_offload and tls_set_sw_offload fail,
do_tls_setsockopt_conf will clear crypto_{send,recv} from the context.
Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so
we won't try to access iv or rec_seq.

-- 
Sabrina

On 23/2/2023 19:15, Sabrina Dubroca wrote:
> 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote:
>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails
>> before kmalloc cctx->iv. This may cause info leak when we call
>> do_tls_getsockopt_conf().
> 
> Is there really an issue here?
> 
> If both tls_set_device_offload and tls_set_sw_offload fail,
> do_tls_setsockopt_conf will clear crypto_{send,recv} from the context.
> Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so
> we won't try to access iv or rec_seq.
> 

My bad. I forget memzero_explicit. Then this is harmless. But I still 
think it is better to set them to NULL like tls_set_sw_offload's error 
path because we don't know there are another way to do this(I will 
change the commit log). What do you think?

On 24/2/2023 11:07, Hangyu Hua wrote:
> On 23/2/2023 19:15, Sabrina Dubroca wrote:
>> 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote:
>>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
>>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it 
>>> fails
>>> before kmalloc cctx->iv. This may cause info leak when we call
>>> do_tls_getsockopt_conf().
>>
>> Is there really an issue here?
>>
>> If both tls_set_device_offload and tls_set_sw_offload fail,
>> do_tls_setsockopt_conf will clear crypto_{send,recv} from the context.
>> Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so
>> we won't try to access iv or rec_seq.
>>
> 
> My bad. I forget memzero_explicit. Then this is harmless. But I still 
> think it is better to set them to NULL like tls_set_sw_offload's error 
> path because we don't know there are another way to do this(I will 
> change the commit log). What do you think?

Like a rare case, there is a race condition between
do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous
condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not
protected by lock_sock in do_tls_getsockopt_conf. It's just too
difficult to satisfy both conditions at the same time.

2023-02-24, 11:33:29 +0800, Hangyu Hua wrote:
> On 24/2/2023 11:07, Hangyu Hua wrote:
> > On 23/2/2023 19:15, Sabrina Dubroca wrote:
> > > 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote:
> > > > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
> > > > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL
> > > > if it fails
> > > > before kmalloc cctx->iv. This may cause info leak when we call
> > > > do_tls_getsockopt_conf().
> > > 
> > > Is there really an issue here?
> > > 
> > > If both tls_set_device_offload and tls_set_sw_offload fail,
> > > do_tls_setsockopt_conf will clear crypto_{send,recv} from the context.
> > > Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so
> > > we won't try to access iv or rec_seq.
> > > 
> > 
> > My bad. I forget memzero_explicit. Then this is harmless. But I still
> > think it is better to set them to NULL like tls_set_sw_offload's error
> > path because we don't know there are another way to do this(I will
> > change the commit log). What do you think?

Yes, I guess for consistency between functions it would be ok.

> Like a rare case, there is a race condition between
> do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous
> condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not
> protected by lock_sock in do_tls_getsockopt_conf. It's just too
> difficult to satisfy both conditions at the same time.

Ugh, thanks for noticing this. We should move the lock_sock in
getsockopt before TLS_CRYPTO_INFO_READY. Do you want to write that
patch?

Thanks.

-- 
Sabrina

On 24/2/2023 15:57, Sabrina Dubroca wrote:
> 2023-02-24, 11:33:29 +0800, Hangyu Hua wrote:
>> On 24/2/2023 11:07, Hangyu Hua wrote:
>>> On 23/2/2023 19:15, Sabrina Dubroca wrote:
>>>> 2023-02-23, 17:05:08 +0800, Hangyu Hua wrote:
>>>>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
>>>>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL
>>>>> if it fails
>>>>> before kmalloc cctx->iv. This may cause info leak when we call
>>>>> do_tls_getsockopt_conf().
>>>>
>>>> Is there really an issue here?
>>>>
>>>> If both tls_set_device_offload and tls_set_sw_offload fail,
>>>> do_tls_setsockopt_conf will clear crypto_{send,recv} from the context.
>>>> Then the TLS_CRYPTO_INFO_READY in do_tls_getsockopt_conf will fail, so
>>>> we won't try to access iv or rec_seq.
>>>>
>>>
>>> My bad. I forget memzero_explicit. Then this is harmless. But I still
>>> think it is better to set them to NULL like tls_set_sw_offload's error
>>> path because we don't know there are another way to do this(I will
>>> change the commit log). What do you think?
> 
> Yes, I guess for consistency between functions it would be ok.
> 
>> Like a rare case, there is a race condition between
>> do_tls_getsockopt_conf and do_tls_setsockopt_conf while the previous
>> condition is met. TLS_CRYPTO_INFO_READY(crypto_info) is not
>> protected by lock_sock in do_tls_getsockopt_conf. It's just too
>> difficult to satisfy both conditions at the same time.
> 
> Ugh, thanks for noticing this. We should move the lock_sock in
> getsockopt before TLS_CRYPTO_INFO_READY. Do you want to write that
> patch?
> 
> Thanks.
> 

I see. I will make a new patch to fix the race and send v2 of this.

Thanks,
Hangyu

On 2023/2/23 17:05, Hangyu Hua wrote:
> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But
> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails
> before kmalloc cctx->iv. This may cause info leak when we call
> do_tls_getsockopt_conf().

Should we use kfree_sensitive() here if info leaking is what we want to
avoid?

> 
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>  net/tls/tls_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 6c593788dc25..a63f6f727f58 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -1241,8 +1241,10 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
>  	kfree(start_marker_record);
>  free_rec_seq:
>  	kfree(ctx->tx.rec_seq);
> +	ctx->tx.rec_seq = NULL;
>  free_iv:
>  	kfree(ctx->tx.iv);
> +	ctx->tx.iv = NULL;
>  release_netdev:
>  	dev_put(netdev);
>  	return rc;
>