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: <20210528124421.12a84cb7@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Fri, 28 May 2021 12:44:21 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
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 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.