netdev - Re: [PATCH net 2/2] net/tls: Fix use-after-free after the TLS device goes down and up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <03669e23-0697-7a96-3d49-202e045cbf48@nvidia.com>
Date:   Mon, 31 May 2021 14:09:56 +0300
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Boris Pismenny <borisp@...dia.com>,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Aviad Yehezkel <aviadye@...dia.com>,
        "Tariq Toukan" <tariqt@...dia.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net 2/2] net/tls: Fix use-after-free after the TLS device
 goes down and up

On 2021-05-28 22:44, Jakub Kicinski wrote:
> On Fri, 28 May 2021 15:40:38 +0300 Maxim Mikityanskiy wrote:
>>>> @@ -961,6 +964,17 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx,
>>>>    
>>>>    	ctx->sw.decrypted |= is_decrypted;
>>>>    
>>>> +	if (unlikely(test_bit(TLS_RX_DEV_DEGRADED, &tls_ctx->flags))) {
>>>
>>> Why not put the check in tls_device_core_ctrl_rx_resync()?
>>> Would be less code, right?
>>
>> I see what you mean, and I considered this option, but I think my option
>> has better readability and is more future-proof. By doing an early
>> return, I skip all code irrelevant to the degraded mode, and even though
>> changing ctx->resync_nh_reset won't have effect in the degraded mode, it
>> will be easier for readers to understand that this part of code is not
>> relevant. Furthermore, if someone decides to add more code to
>> !is_encrypted branches in the future, there is a chance that the
>> degraded mode will be missed from consideration. With the early return
>> there is not problem, but if I follow your suggestion and do the check
>> only under is_encrypted, a future contributor unfamiliar with this
>> "degraded flow" might fail to add that check where it will be needed.
>>
>> This was the reason I implemented it this way. What do you think?
> 
> In general "someone may miss this in the future" is better served by
> adding a test case than code duplication. But we don't have infra to
> fake-offload TLS so I don't feel strongly. You can keep as is if that's
> your preference.

Yeah, I agree that we would benefit from having unit tests for such 
flows. But as we don't have the needed infrastructure, and you are fine 
with the current implementation, I'll keep it. The only thing I need to 
fix when resubmitting is the unneeded EXPORT_SYMBOL_GPL, right?

Thanks for reviewing.

>