Re: [RFC 0/1] whitelisting UDP GSO and GRO cmsgs

[Soheil Hassas Yeganeh <soheil@google.com>]

On Mon, Nov 30, 2020 at 5:52 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 28.11.20 um 20:03 schrieb Victor Stewart:
> > On Thu, Nov 26, 2020 at 7:36 AM Stefan Metzmacher <metze@samba.org> wrote:
> >>
> >> Am 23.11.20 um 17:29 schrieb Victor Stewart:
> >>> On Mon, Nov 23, 2020 at 4:13 PM Stefan Metzmacher <metze@samba.org> wrote:
> >>>>
> >>>> Hi Victor,
> >>>>
> >>>> wouldn't it be enough to port the PROTO_CMSG_DATA_ONLY check to the sendmsg path?
> >>>>
> >>>> UDP sockets should have PROTO_CMSG_DATA_ONLY set.
> >>>>
> >>>> I guess that would fix your current problem.
> >>>
> >>> that would definitely solve the problem and is the easiest solution.
> >>>
> >>> but PROTO_CMSG_DATA_ONLY is only set on inet_stream_ops and
> >>> inet6_stream_ops but dgram?
> >>
> >> I guess PROTO_CMSG_DATA_ONLY should be added also for dgram sockets.
> >>
> >> Did you intend to remove the cc for the mailing list?
> >>
> >> I think in addition to the io-uring list, cc'ing netdev@vger.kernel.org
> >> would also be good.
> >
> > whoops forgot to reply all.
> >
> > before I CC netdev, what does PROTO_CMSG_DATA_ONLY actually mean?
>
> I don't really know, but I guess it means that, any supported CMSG type
> on that socket won't do any magic depending on the process state, like
> fd passing with SOL_SOCKET/SCM_RIGHTS or SCM_CREDENTIALS. The CMSG buffer
> would just be a plain byte array, which may only reference state attached
> to the specific socket or packet.
>
> I'd guess that the author and/or reviewers can clarify that, let's see what
> they'll answer.
>
> > I didn't find a clear explanation anywhere by searching the kernel, only
> > that it was defined as 1 and flagged on inet_stream_ops and
> > inet6_stream_ops.
> >
> > there must be a reason it was not initially included for dgrams?
>
> I can't think of any difference I guess the author just tried to get add support for the specific usecase
> that didn't work (MSG_ZEROCOPY in this case, most likely only tested with a tcp workload):
>
> commit 583bbf0624dfd8fc45f1049be1d4980be59451ff
> Author: Luke Hsiao <lukehsiao@google.com>
> Date:   Fri Aug 21 21:41:04 2020 -0700
>
>     io_uring: allow tcp ancillary data for __sys_recvmsg_sock()
>
>     For TCP tx zero-copy, the kernel notifies the process of completions by
>     queuing completion notifications on the socket error queue. This patch
>     allows reading these notifications via recvmsg to support TCP tx
>     zero-copy.
>
>     Ancillary data was originally disallowed due to privilege escalation
>     via io_uring's offloading of sendmsg() onto a kernel thread with kernel
>     credentials (https://crbug.com/project-zero/1975). So, we must ensure
>     that the socket type is one where the ancillary data types that are
>     delivered on recvmsg are plain data (no file descriptors or values that
>     are translated based on the identity of the calling process).

Thank you for CCing us.

The reason for PROTO_CMSG_DATA_ONLY is explained in the paragraph
above in the commit message.  PROTO_CMSG_DATA_ONLY is basically to
allow-list a protocol that is guaranteed not to have the privilege
escalation in https://crbug.com/project-zero/1975.  TCP doesn't have
that issue, and I believe UDP doesn't have that issue either (but
please audit and confirm that with +Jann Horn).

If you couldn't find any non-data CMSGs for UDP, you should just add
PROTO_CMSG_DATA_ONLY to inet dgram sockets instead of introducing
__sys_whitelisted_cmsghdrs as Stefan mentioned.

Thanks,
Soheil

>     This was tested by using io_uring to call recvmsg on the MSG_ERRQUEUE
>     with tx zero-copy enabled. Before this patch, we received -EINVALID from
>     this specific code path. After this patch, we could read tcp tx
>     zero-copy completion notifications from the MSG_ERRQUEUE.
>
>     Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
>     Signed-off-by: Arjun Roy <arjunroy@google.com>
>     Acked-by: Eric Dumazet <edumazet@google.com>
>     Reviewed-by: Jann Horn <jannh@google.com>
>     Reviewed-by: Jens Axboe <axboe@kernel.dk>
>     Signed-off-by: Luke Hsiao <lukehsiao@google.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> > but yes if there's nothing standing in the way of adding it for
> > dgrams, and it covers UDP_SEGMENT and UDP_GRO then that's of course
> > the least friction solution here.
>
> Yes, it would avoid whitelisting new specific usecases.
>
> metze
>
>