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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 Nov 2020 11:52:08 +0100
From:   Stefan Metzmacher <metze@...ba.org>
To:     Victor Stewart <v@...etag.social>
Cc:     io-uring <io-uring@...r.kernel.org>,
        Luke Hsiao <lukehsiao@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jann Horn <jannh@...gle.com>, Arjun Roy <arjunroy@...gle.com>,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        netdev <netdev@...r.kernel.org>, Jens Axboe <axboe@...nel.dk>
Subject: Re: [RFC 0/1] whitelisting UDP GSO and GRO cmsgs

Am 28.11.20 um 20:03 schrieb Victor Stewart:
> On Thu, Nov 26, 2020 at 7:36 AM Stefan Metzmacher <metze@...ba.org> wrote:
>>
>> Am 23.11.20 um 17:29 schrieb Victor Stewart:
>>> On Mon, Nov 23, 2020 at 4:13 PM Stefan Metzmacher <metze@...ba.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@...r.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@...gle.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@...gle.com>
    Signed-off-by: Arjun Roy <arjunroy@...gle.com>
    Acked-by: Eric Dumazet <edumazet@...gle.com>
    Reviewed-by: Jann Horn <jannh@...gle.com>
    Reviewed-by: Jens Axboe <axboe@...nel.dk>
    Signed-off-by: Luke Hsiao <lukehsiao@...gle.com>
    Signed-off-by: David S. Miller <davem@...emloft.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




Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)