On Wed, 31 Jul 2019 13:57:26 +0000, Boris Pismenny wrote:
> > diff --git a/Documentation/networking/tls-offload.rst
> > b/Documentation/networking/tls-offload.rst
> > index 048e5ca44824..2bc3ab5515d8 100644
> > --- a/Documentation/networking/tls-offload.rst
> > +++ b/Documentation/networking/tls-offload.rst
> > @@ -499,15 +499,6 @@ offloads, old connections will remain active after
> > flags are cleared.
> > Known bugs
> > ==========
> >
> > -skb_orphan() leaks clear text
> > ------------------------------
> > -
> > -Currently drivers depend on the :c:member:`sk` member of
> > -:c:type:`struct sk_buff <sk_buff>` to identify segments requiring
> > -encryption. Any operation which removes or does not preserve the socket
> > -association such as :c:func:`skb_orphan` or :c:func:`skb_clone`
> > -will cause the driver to miss the packets and lead to clear text leaks.
> > -
> > Redirects leak clear text
> > -------------------------
> Doesn't this patch cover the redirect case as well?
Ah, good catch! I thought this entry was for socket redirect, which
will be a separate fix, but it seems we don't have an entry for that
one.
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 228db3998e46..90f3f552c789 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -814,6 +814,7 @@ enum sock_flags {
> > SOCK_TXTIME,
> > SOCK_XDP, /* XDP is attached */
> > SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
> > + SOCK_CRYPTO_TX_PLAIN_TEXT, /* Generate skbs with decrypted flag set */
> > };
> >
> > #define SK_FLAGS_TIMESTAMP ((1UL << SOCK_TIMESTAMP) | (1UL <<
> > SOCK_TIMESTAMPING_RX_SOFTWARE))
> > @@ -2480,8 +2481,26 @@ static inline bool sk_fullsock(const struct sock *sk)
> > return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
> > }
> >
> > +static inline void sk_set_tx_crypto(const struct sock *sk, struct sk_buff
> > *skb)
> > +{
> > +#ifdef CONFIG_TLS_DEVICE
> > + skb->decrypted = sock_flag(sk, SOCK_CRYPTO_TX_PLAIN_TEXT);
> > +#endif
> > +}
>
> Have you considered the following alternative to calling
> sk_set_tx_crypto() - Add a new MSG_DECRYPTE flag that will set the
> skb->decrypted bit in the do_tcp_sendpages function.
>
> Then, the rest of the TCP code can propagate this bit from the existing skb.
That'd also work marking the socket as crypto seemed easy enough. I was
planning on using sk_rx_crypto_match() for socket redirect also, so
it'd be symmetrical. Is there a preference for using the internal flags?
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index f62f0e7e3cdd..dee93eab02fe 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -984,6 +984,7 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page
> > *page, int offset,
> > if (!skb)
> > goto wait_for_memory;
> >
> > + sk_set_tx_crypto(sk, skb);
> > skb_entail(sk, skb);
> > copy = size_goal;
> > }
> > @@ -993,7 +994,8 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page
> > *page, int offset,
> >
> > i = skb_shinfo(skb)->nr_frags;
> > can_coalesce = skb_can_coalesce(skb, i, page, offset);
> > - if (!can_coalesce && i >= sysctl_max_skb_frags) {
> > + if ((!can_coalesce && i >= sysctl_max_skb_frags) ||
> > + !sk_tx_crypto_match(sk, skb)) {
>
> I see that sk_tx_crypto_match is called only here to handle cases where
> the socket expected crypto offload, while the skb is not marked
> decrypted. AFAIU, this should not be possible, because we set the
> skb->eor bit for the last plaintext skb before sending any traffic that
> expects crypto offload.
Ack, I missed the tcp_skb_can_collapse_to() above.
