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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
>
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