public inbox for [email protected]
 help / color / mirror / Atom feed
From: Soheil Hassas Yeganeh <[email protected]>
To: Stefan Metzmacher <[email protected]>
Cc: Victor Stewart <[email protected]>,
	io-uring <[email protected]>,
	Luke Hsiao <[email protected]>,
	"David S. Miller" <[email protected]>,
	Jann Horn <[email protected]>, Arjun Roy <[email protected]>,
	netdev <[email protected]>, Jens Axboe <[email protected]>
Subject: Re: [RFC 0/1] whitelisting UDP GSO and GRO cmsgs
Date: Mon, 30 Nov 2020 09:57:16 -0500	[thread overview]
Message-ID: <CACSApvZW-UN9_To0J-bO6SMYKJgF9oFvsKk14D-7Tx4zzc8JUw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>

On Mon, Nov 30, 2020 at 5:52 AM Stefan Metzmacher <[email protected]> wrote:
>
> Am 28.11.20 um 20:03 schrieb Victor Stewart:
> > On Thu, Nov 26, 2020 at 7:36 AM Stefan Metzmacher <[email protected]> wrote:
> >>
> >> Am 23.11.20 um 17:29 schrieb Victor Stewart:
> >>> On Mon, Nov 23, 2020 at 4:13 PM Stefan Metzmacher <[email protected]> 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 [email protected]
> >> 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 <[email protected]>
> 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 <[email protected]>
>     Signed-off-by: Arjun Roy <[email protected]>
>     Acked-by: Eric Dumazet <[email protected]>
>     Reviewed-by: Jann Horn <[email protected]>
>     Reviewed-by: Jens Axboe <[email protected]>
>     Signed-off-by: Luke Hsiao <[email protected]>
>     Signed-off-by: David S. Miller <[email protected]>
>
> > 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
>
>

  reply	other threads:[~2020-11-30 14:58 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-23 15:29 [RFC 0/1] whitelisting UDP GSO and GRO cmsgs Victor Stewart
2020-11-23 16:13 ` Stefan Metzmacher
     [not found]   ` <CAM1kxwhUcXLKU=2hCVaBngOKRL_kgMX4ONy9kpzKW+ZBZraEYw@mail.gmail.com>
     [not found]     ` <[email protected]>
2020-11-28 19:03       ` Victor Stewart
2020-11-30 10:52         ` Stefan Metzmacher
2020-11-30 14:57           ` Soheil Hassas Yeganeh [this message]
2020-11-30 15:05             ` Stefan Metzmacher
2020-11-30 15:15               ` Soheil Hassas Yeganeh
2020-11-30 16:17                 ` Victor Stewart
2020-11-30 16:20                   ` Soheil Hassas Yeganeh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACSApvZW-UN9_To0J-bO6SMYKJgF9oFvsKk14D-7Tx4zzc8JUw@mail.gmail.com \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox