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
>
>
next prev parent 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