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

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

On Wed, 7 Aug 2019 14:46:23 -0400, Willem de Bruijn wrote:
> > > > @@ -984,6 +984,9 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
> > > >                         if (!skb)
> > > >                                 goto wait_for_memory;
> > > >
> > > > +#ifdef CONFIG_TLS_DEVICE
> > > > +                       skb->decrypted = !!(flags & MSG_SENDPAGE_DECRYPTED);
> > > > +#endif  
> > >
> > > Nothing is stopping userspace from passing this new flag. In send
> > > (tcp_sendmsg_locked) it is ignored. But can it reach do_tcp_sendpages
> > > through tcp_bpf_sendmsg?  
> >
> > Ah, I think you're right, thanks for checking that :( I don't entirely
> > follow how 0608c69c9a80 ("bpf: sk_msg, sock{map|hash} redirect through
> > ULP") is safe then.
> >
> > One option would be to clear the flags kernel would previously ignore
> > in tcp_bpf_sendmsg(). But I feel like we should just go back to marking
> > the socket, since we don't need the per-message flexibility of a flag.
> >
> > WDYT?  
> 
> I don't feel strongly either way. Passing flags from send through
> tcp_bpf_sendmsg is probably unintentional, so should probably be
> addressed anyway? Then this is a bit simpler.

FWIW I had a closer look at the tcp_bpf_sendmsg() flags, and
MSG_SENDPAGE_NOPOLICY should be okay to let though there.

That flag is only meaningful to tls in case sockmap is layered 
on top of tls and we'd always set it before calling tls.

v3 coming soon..