Re: [PATCH net] tls: device: keep tls_sw_fallback() from encrypting its own output

[Sabrina Dubroca <sd@queasysnail.net>]

2026-10-19, 15:26:51 -0600, Rishikesh Jethwani wrote:
> A socket offloaded through a bond has every skb validated twice, once
> at the master and again at the slave. tls_validate_xmit_skb() passes
> the master-level call through for that reason (netif_is_bond_master()),
> but tls_validate_xmit_skb_sw(), the validator tls_device_down()
> installs, calls tls_sw_fallback() unconditionally, so it encrypts the
> master-level ciphertext a second time with the same rcd_sn and IV. This
> cancels the CTR keystream and exposes plaintext on the wire. For a
> complete record encrypted twice in one skb, the second pass also
> generates a valid GCM tag, so the peer accepts corrupted data without
> an authentication failure.
> 
> Guard on the content rather than the device. Offload plaintext always
> carries the decrypted bit (tls_push_data() and
> tls_device_write_space() send with MSG_SENDPAGE_DECRYPTED), so
> tls_sw_fallback() can pass through any skb without it: such a skb needs
> no offload encryption. complete_skb() now clears the bit that
> skb_copy_header() copied from the plaintext original, so the fallback's
> own output passes through any later validation pass untouched. This
> also does not rely on every path validating a skb at both the master
> and the slave.
> 
> Tested on two hosts back to back over a mlx5 port, the only slave of
> a bond0 (balance-xor, layer3+4), so each transmit is validated at the

As in another patch, a couple of selftests for "offload over
bond->NIC" would be good.

> master and again at the slave. Run from each host as sender, a TLS 1.2
> AES-GCM-128 TX socket kept open across an administrative bounce of the
> slave leaked every later record as plaintext before the fix; after it
> each record is encrypted exactly once and the kTLS peer reads the
> stream intact.

The wording and verbosity of this section ("administrative bounce", 3
lines to say "before it didn't work and now it does" which IMO
shouldn't even have to be said) needs to be cleaned up. The rest of
the commit message as well.

> Fixes: c55dcdd435aa ("net/tls: Fix use-after-free after the TLS device goes down and up")
> Cc: stable@vger.kernel.org # needs commit fc6d80eb5044 ("tcp: prevent collapsing skbs across boundary in rtx queue")
> Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
> ---
>  net/tls/tls_device_fallback.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
> index 3b7d0ab2bcf1..24fba12febd1 100644
> --- a/net/tls/tls_device_fallback.c
> +++ b/net/tls/tls_device_fallback.c
> @@ -192,6 +192,14 @@ static void complete_skb(struct sk_buff *nskb, struct sk_buff *skb, int headln)
>  	skb->destructor = NULL;
>  	skb->sk = NULL;
>  
> +	/* nskb carries ciphertext now, but skb_copy_header() inherited the
> +	 * plaintext original's decrypted bit. Clear it so the bit keeps meaning
> +	 * "still-plaintext, needs an encryptor": a requeued nskb would otherwise
> +	 * be re-validated and encrypted a second time on any re-entry into
> +	 * tls_sw_fallback().
> +	 */

"nskb is encrypted now" is enough. the rest of this comment (why it
was set and what the bug would be) really doesn't belong here.

> +	nskb->decrypted = 0;
> +
>  	update_chksum(nskb, headln);
>  
>  	/* sock_efree means skb must gone through skb_orphan_partial() */
> @@ -371,6 +379,17 @@ static struct sk_buff *tls_sw_fallback(struct sock *sk, struct sk_buff *skb)
>  	s32 sync_size = 0;
>  	u64 rcd_sn;
>  
> +	/* Only skbs carrying the decrypted bit hold plaintext that awaits
> +	 * offload encryption; anything else needs no work here. That
> +	 * includes this function's own output (complete_skb() clears the
> +	 * bit skb_copy_header() inherited), which is seen again when a skb
> +	 * is validated a second time, e.g. a bond master handing it to a
> +	 * slave via bond_dev_queue_xmit(). fill_sg_in() would map it back
> +	 * onto its own record by tcp_seq, reusing rcd_sn and IV.
> +	 */

Sorry, I'm not reading all this. 0 or 1 line of comment here, nothing more.

> +	if (!skb_is_decrypted(skb))
> +		return skb;
> +
>  	/* worst case is:
>  	 * MAX_SKB_FRAGS in tls_record_info
>  	 * MAX_SKB_FRAGS + 1 in SKB head and frags.
> -- 
> 2.50.1
> 

-- 
Sabrina