Re: [PATCH net v3] net/tls: prevent skb_orphan() from leaking TLS plain text with offload

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Thu, 8 Aug 2019 11:59:18 -0400, Willem de Bruijn wrote:
> > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > index 7c0b2b778703..43922d86e510 100644
> > --- a/net/tls/tls_device.c
> > +++ b/net/tls/tls_device.c
> > @@ -373,9 +373,9 @@ static int tls_push_data(struct sock *sk,
> >         struct tls_context *tls_ctx = tls_get_ctx(sk);
> >         struct tls_prot_info *prot = &tls_ctx->prot_info;
> >         struct tls_offload_context_tx *ctx = tls_offload_ctx_tx(tls_ctx);
> > -       int tls_push_record_flags = flags | MSG_SENDPAGE_NOTLAST;
> >         int more = flags & (MSG_SENDPAGE_NOTLAST | MSG_MORE);
> >         struct tls_record_info *record = ctx->open_record;
> > +       int tls_push_record_flags;
> >         struct page_frag *pfrag;
> >         size_t orig_size = size;
> >         u32 max_open_record_len;
> > @@ -390,6 +390,9 @@ static int tls_push_data(struct sock *sk,
> >         if (sk->sk_err)
> >                 return -sk->sk_err;
> >
> > +       flags |= MSG_SENDPAGE_DECRYPTED;
> > +       tls_push_record_flags = flags | MSG_SENDPAGE_NOTLAST;
> > +  
> 
> Without being too familiar with this code: can this plaintext flag be
> set once, closer to the call to do_tcp_sendpages, in tls_push_sg?
> 
> Instead of two locations with multiple non-trivial codepaths between
> them and do_tcp_sendpages.
> 
> Or are there paths where the flag is not set? Which I imagine would
> imply already passing s/w encrypted ciphertext.

tls_push_sg() is shared with sw path which doesn't have the device
validation. 

Device TLS can read tls_push_sg() via tls_push_partial_record() and
tls_push_data(). tls_push_data() is addressed directly here,
tls_push_partial_record() is again shared with SW path, so we have to
address it by adding the flag in tls_device_write_space().

The alternative is to add a conditional to tls_push_sg() which is 
a little less nice from performance and layering PoV but it is a lot
simpler..

Should I change?