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

[Rishikesh Jethwani]

On Wed, Oct 21, 2026 at 2:04 AM Sabrina Dubroca <sd@xxxxxxxxxxxxxxx> wrote:
>
> 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.

Will do. The hw offload test these would go into is still under
review:

https://lore.kernel.org/netdev/20261020181147.939575-2-rjethwani@xxxxxxxxxxxxxxx/

Would you prefer I fold this fix and the bond tests into the next
revision of that series, or wait until it's applied and send v2 of
this patch with the tests on top?

> > 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@xxxxxxxxxxxxxxx # needs commit fc6d80eb5044 ("tcp: prevent collapsing skbs across boundary in rtx queue")
> > Signed-off-by: Rishikesh Jethwani <rjethwani@xxxxxxxxxxxxxxx>
> > ---
> >  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