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

From: Victor Stewart <v@nametag.social>
To: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Stefan Metzmacher <metze@samba.org>,
	io-uring <io-uring@vger.kernel.org>,
	Luke Hsiao <lukehsiao@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jann Horn <jannh@google.com>, Arjun Roy <arjunroy@google.com>,
	netdev <netdev@vger.kernel.org>, Jens Axboe <axboe@kernel.dk>
Subject: Re: [RFC 0/1] whitelisting UDP GSO and GRO cmsgs
Date: Mon, 30 Nov 2020 16:17:22 +0000	[thread overview]
Message-ID: <CAM1kxwgaWxhJ7RQT3rMaRow8yUQjM_5=rZkv88+-heaiB_2hjA@mail.gmail.com> (raw)
In-Reply-To: <CACSApvboyVGOmFKdQLpJd+0fnOAfMvgUwpzRXqLbdSJWMQYmyg@mail.gmail.com>

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