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

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

[Victor Stewart]

so currently all cmsg headers are disabled through sendmsg and recvmsg
operations through io_uring because of
https://www.exploit-db.com/exploits/47779

i think it's time we start whitelisting the good guys though? GSO and
GRO are hugely important for QUIC servers, and together offer a higher
throughput gain than io_uring alone (rate of data transit
considering), thus io_uring is the lesser performance choice for QUIC
servers at the moment.

RE http://vger.kernel.org/lpc_net2018_talks/willemdebruijn-lpc2018-udpgso-paper-DRAFT-1.pdf,
GSO is about +~63% and GRO +~82%.

this patch closes that loophole.

Victor Stewart (1);
   net/socket.c: add __sys_whitelisted_cmsghdrs()

   net/socket.c | 15 ++++++++++++---
   1 file changed, 12 insertions(+), 3 deletions(-)

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

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 1172 bytes --]

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.

Whitelisting more (or even all) would need more work, but can be done
later.

metze

Am 23.11.20 um 16:29 schrieb Victor Stewart:
> so currently all cmsg headers are disabled through sendmsg and recvmsg
> operations through io_uring because of
> https://www.exploit-db.com/exploits/47779
> 
> i think it's time we start whitelisting the good guys though? GSO and
> GRO are hugely important for QUIC servers, and together offer a higher
> throughput gain than io_uring alone (rate of data transit
> considering), thus io_uring is the lesser performance choice for QUIC
> servers at the moment.
> 
> RE http://vger.kernel.org/lpc_net2018_talks/willemdebruijn-lpc2018-udpgso-paper-DRAFT-1.pdf,
> GSO is about +~63% and GRO +~82%.
> 
> this patch closes that loophole.
> 
> Victor Stewart (1);
>    net/socket.c: add __sys_whitelisted_cmsghdrs()
> 
>    net/socket.c | 15 ++++++++++++---
>    1 file changed, 12 insertions(+), 3 deletions(-)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[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
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?

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.

>
> metze
>
>

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

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 3684 bytes --]

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).

    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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Soheil Hassas Yeganeh]

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
>
>

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

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 797 bytes --]

Hi Soheil,

> 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.

Was there a specific reason why you only added the PROTO_CMSG_DATA_ONLY check
in __sys_recvmsg_sock(), but not in __sys_sendmsg_sock()?

metze




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Soheil Hassas Yeganeh]

On Mon, Nov 30, 2020 at 10:05 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Soheil,
>
> > 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.
>
> Was there a specific reason why you only added the PROTO_CMSG_DATA_ONLY check
> in __sys_recvmsg_sock(), but not in __sys_sendmsg_sock()?

We only needed this for recvmsg(MSG_ERRQUEUE) to support transmit
zerocopy.  So, we took a more conservative approach and didn't add it
for sendmsg().

I believe it should be fine to add it for TCP sendmsg, because for
SO_MARK we check the user's capability:

if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
          return -EPERM;

I believe udp_sendmsg() is sane too and I cannot spot any issue there.

> metze
>
>
>

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

[Victor Stewart]

this being the list of UDP options.. i think we're good here? I'll put
together a new patch.

https://github.com/torvalds/linux/blob/b65054597872ce3aefbc6a666385eabdf9e288da/include/uapi/linux/udp.h#L30

/* UDP socket options */
#define UDP_CORK 1 /* Never send partially complete segments */
#define UDP_ENCAP 100 /* Set the socket to accept encapsulated packets */
#define UDP_NO_CHECK6_TX 101 /* Disable sending checksum for UDP6X */
#define UDP_NO_CHECK6_RX 102 /* Disable accpeting checksum for UDP6 */
#define UDP_SEGMENT 103 /* Set GSO segmentation size */
#define UDP_GRO 104 /* This socket can receive UDP GRO packets */

On Mon, Nov 30, 2020 at 3:15 PM Soheil Hassas Yeganeh <soheil@google.com> wrote:
>
> On Mon, Nov 30, 2020 at 10:05 AM Stefan Metzmacher <metze@samba.org> wrote:
> >
> > Hi Soheil,
> >
> > > 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.
> >
> > Was there a specific reason why you only added the PROTO_CMSG_DATA_ONLY check
> > in __sys_recvmsg_sock(), but not in __sys_sendmsg_sock()?
>
> We only needed this for recvmsg(MSG_ERRQUEUE) to support transmit
> zerocopy.  So, we took a more conservative approach and didn't add it
> for sendmsg().
>
> I believe it should be fine to add it for TCP sendmsg, because for
> SO_MARK we check the user's capability:
>
> if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
>           return -EPERM;
>
> I believe udp_sendmsg() is sane too and I cannot spot any issue there.
>
> > metze
> >
> >
> >

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

[Soheil Hassas Yeganeh]

On Mon, Nov 30, 2020 at 11:17 AM Victor Stewart <v@nametag.social> wrote:
>
> this being the list of UDP options.. i think we're good here? I'll put
> together a new patch.
>
> https://github.com/torvalds/linux/blob/b65054597872ce3aefbc6a666385eabdf9e288da/include/uapi/linux/udp.h#L30
>
> /* UDP socket options */
> #define UDP_CORK 1 /* Never send partially complete segments */
> #define UDP_ENCAP 100 /* Set the socket to accept encapsulated packets */
> #define UDP_NO_CHECK6_TX 101 /* Disable sending checksum for UDP6X */
> #define UDP_NO_CHECK6_RX 102 /* Disable accpeting checksum for UDP6 */
> #define UDP_SEGMENT 103 /* Set GSO segmentation size */
> #define UDP_GRO 104 /* This socket can receive UDP GRO packets */

That is not sufficient proof, because in udp_sendmsg() we also call
ip_cmsg_send() in udp_sendmsg(), and  ip_cmsg_recv_offset() in
udp_recvmsg().  That said, I have audited them and I think they are
sane.

Jann, what do you think?

> On Mon, Nov 30, 2020 at 3:15 PM Soheil Hassas Yeganeh <soheil@google.com> wrote:
> >
> > On Mon, Nov 30, 2020 at 10:05 AM Stefan Metzmacher <metze@samba.org> wrote:
> > >
> > > Hi Soheil,
> > >
> > > > 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.
> > >
> > > Was there a specific reason why you only added the PROTO_CMSG_DATA_ONLY check
> > > in __sys_recvmsg_sock(), but not in __sys_sendmsg_sock()?
> >
> > We only needed this for recvmsg(MSG_ERRQUEUE) to support transmit
> > zerocopy.  So, we took a more conservative approach and didn't add it
> > for sendmsg().
> >
> > I believe it should be fine to add it for TCP sendmsg, because for
> > SO_MARK we check the user's capability:
> >
> > if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
> >           return -EPERM;
> >
> > I believe udp_sendmsg() is sane too and I cannot spot any issue there.
> >
> > > metze
> > >
> > >
> > >