Re: io_uring, IORING_OP_RECVMSG and ancillary data

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jens Axboe]

On 4/25/20 11:29 AM, Andreas Smas wrote:
> Hi,
> 
> Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
> particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
> 
> These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
> 
> The following hack fixes the problem for me and I get valid timestamps
> back. Not suggesting this is the real fix as I'm not sure what the
> implications of this is.
> 
> Any insight into this would be much appreciated.

It was originally disabled because of a security issue, but I do think
it's safe to enable again.

Adding the io-uring list and Jann as well, leaving patch intact below.

> diff --git a/net/socket.c b/net/socket.c
> index 2dd739fba866..689f41f4156e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
> struct msghdr *msg,
>                         struct user_msghdr __user *umsg,
>                         struct sockaddr __user *uaddr, unsigned int flags)
>  {
> -       /* disallow ancillary data requests from this path */
> -       if (msg->msg_control || msg->msg_controllen)
> -               return -EINVAL;
> -
>         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
>  }
> 


-- 
Jens Axboe

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Andreas Smas]

On Sat, Apr 25, 2020 at 1:23 PM Jens Axboe <axboe@kernel.dk> wrote:
>
>
> It was originally disabled because of a security issue, but I do think
> it's safe to enable again.
>
> Adding the io-uring list and Jann as well, leaving patch intact below.
>

Ok thanks,

Should I prepare a new real patch or will this trickle on?

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jann Horn]

On Sat, Apr 25, 2020 at 10:23 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 4/25/20 11:29 AM, Andreas Smas wrote:
> > Hi,
> >
> > Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
> > particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
> >
> > These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
> >
> > The following hack fixes the problem for me and I get valid timestamps
> > back. Not suggesting this is the real fix as I'm not sure what the
> > implications of this is.
> >
> > Any insight into this would be much appreciated.
>
> It was originally disabled because of a security issue, but I do think
> it's safe to enable again.
>
> Adding the io-uring list and Jann as well, leaving patch intact below.
>
> > diff --git a/net/socket.c b/net/socket.c
> > index 2dd739fba866..689f41f4156e 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
> > struct msghdr *msg,
> >                         struct user_msghdr __user *umsg,
> >                         struct sockaddr __user *uaddr, unsigned int flags)
> >  {
> > -       /* disallow ancillary data requests from this path */
> > -       if (msg->msg_control || msg->msg_controllen)
> > -               return -EINVAL;
> > -
> >         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
> >  }

I think that's hard to get right. In particular, unix domain sockets
can currently pass file descriptors in control data - so you'd need to
set the file_table flag for recvmsg and sendmsg. And I'm not sure
whether, to make this robust, there should be a whitelist of types of
control messages that are permitted to be used with io_uring, or
something like that...

I think of ancillary buffers as being kind of like ioctl handlers in
this regard.

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jens Axboe]

On 4/27/20 1:20 PM, Jann Horn wrote:
> On Sat, Apr 25, 2020 at 10:23 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 4/25/20 11:29 AM, Andreas Smas wrote:
>>> Hi,
>>>
>>> Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
>>> particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
>>>
>>> These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
>>>
>>> The following hack fixes the problem for me and I get valid timestamps
>>> back. Not suggesting this is the real fix as I'm not sure what the
>>> implications of this is.
>>>
>>> Any insight into this would be much appreciated.
>>
>> It was originally disabled because of a security issue, but I do think
>> it's safe to enable again.
>>
>> Adding the io-uring list and Jann as well, leaving patch intact below.
>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index 2dd739fba866..689f41f4156e 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
>>> struct msghdr *msg,
>>>                         struct user_msghdr __user *umsg,
>>>                         struct sockaddr __user *uaddr, unsigned int flags)
>>>  {
>>> -       /* disallow ancillary data requests from this path */
>>> -       if (msg->msg_control || msg->msg_controllen)
>>> -               return -EINVAL;
>>> -
>>>         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
>>>  }
> 
> I think that's hard to get right. In particular, unix domain sockets
> can currently pass file descriptors in control data - so you'd need to
> set the file_table flag for recvmsg and sendmsg. And I'm not sure
> whether, to make this robust, there should be a whitelist of types of
> control messages that are permitted to be used with io_uring, or
> something like that...
> 
> I think of ancillary buffers as being kind of like ioctl handlers in
> this regard.

Good point. I'll send out something that hopefully will be enough to
be useful, whole not allowing anything randomly.

-- 
Jens Axboe

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jens Axboe]

On 4/27/20 1:29 PM, Jens Axboe wrote:
> On 4/27/20 1:20 PM, Jann Horn wrote:
>> On Sat, Apr 25, 2020 at 10:23 PM Jens Axboe <axboe@kernel.dk> wrote:
>>> On 4/25/20 11:29 AM, Andreas Smas wrote:
>>>> Hi,
>>>>
>>>> Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
>>>> particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
>>>>
>>>> These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
>>>>
>>>> The following hack fixes the problem for me and I get valid timestamps
>>>> back. Not suggesting this is the real fix as I'm not sure what the
>>>> implications of this is.
>>>>
>>>> Any insight into this would be much appreciated.
>>>
>>> It was originally disabled because of a security issue, but I do think
>>> it's safe to enable again.
>>>
>>> Adding the io-uring list and Jann as well, leaving patch intact below.
>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index 2dd739fba866..689f41f4156e 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
>>>> struct msghdr *msg,
>>>>                         struct user_msghdr __user *umsg,
>>>>                         struct sockaddr __user *uaddr, unsigned int flags)
>>>>  {
>>>> -       /* disallow ancillary data requests from this path */
>>>> -       if (msg->msg_control || msg->msg_controllen)
>>>> -               return -EINVAL;
>>>> -
>>>>         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
>>>>  }
>>
>> I think that's hard to get right. In particular, unix domain sockets
>> can currently pass file descriptors in control data - so you'd need to
>> set the file_table flag for recvmsg and sendmsg. And I'm not sure
>> whether, to make this robust, there should be a whitelist of types of
>> control messages that are permitted to be used with io_uring, or
>> something like that...
>>
>> I think of ancillary buffers as being kind of like ioctl handlers in
>> this regard.
> 
> Good point. I'll send out something that hopefully will be enough to
> be useful, whole not allowing anything randomly.

That things is a bit of a mess... How about something like this for
starters?


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 084dfade5cda..40aa5b38367e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3570,6 +3570,37 @@ static int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	return ret;
 }
 
+static bool __io_net_allow_cmsg(struct cmsghdr *cmsg)
+{
+	switch (cmsg->cmsg_level) {
+	case SOL_SOCKET:
+		if (cmsg->cmsg_type != SCM_RIGHTS &&
+		    cmsg->cmsg_type != SCM_CREDENTIALS)
+			return true;
+		return false;
+	case SOL_IP:
+	case SOL_TCP:
+	case SOL_IPV6:
+	case SOL_ICMPV6:
+	case SOL_SCTP:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool io_net_allow_cmsg(struct msghdr *msg)
+{
+	struct cmsghdr *cmsg;
+
+	for_each_cmsghdr(cmsg, msg) {
+		if (!__io_net_allow_cmsg(cmsg))
+			return false;
+	}
+
+	return true;
+}
+
 static int io_sendmsg(struct io_kiocb *req, bool force_nonblock)
 {
 	struct io_async_msghdr *kmsg = NULL;
@@ -3604,6 +3635,11 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock)
 				return ret;
 		}
 
+		if (!io_net_allow_cmsg(&kmsg->msg)) {
+			ret = -EINVAL;
+			goto err;
+		}
+
 		flags = req->sr_msg.msg_flags;
 		if (flags & MSG_DONTWAIT)
 			req->flags |= REQ_F_NOWAIT;
@@ -3617,6 +3653,7 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock)
 			ret = -EINTR;
 	}
 
+err:
 	if (kmsg && kmsg->iov != kmsg->fast_iov)
 		kfree(kmsg->iov);
 	req->flags &= ~REQ_F_NEED_CLEANUP;
@@ -3840,6 +3877,11 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock)
 				return ret;
 		}
 
+		if (!io_net_allow_cmsg(&kmsg->msg)) {
+			ret = -EINVAL;
+			goto err;
+		}
+
 		kbuf = io_recv_buffer_select(req, &cflags, !force_nonblock);
 		if (IS_ERR(kbuf)) {
 			return PTR_ERR(kbuf);
@@ -3863,6 +3905,7 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock)
 			ret = -EINTR;
 	}
 
+err:
 	if (kmsg && kmsg->iov != kmsg->fast_iov)
 		kfree(kmsg->iov);
 	req->flags &= ~REQ_F_NEED_CLEANUP;
diff --git a/net/socket.c b/net/socket.c
index 2dd739fba866..78cdf9a8cf73 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2425,10 +2425,6 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
 long __sys_sendmsg_sock(struct socket *sock, struct msghdr *msg,
 			unsigned int flags)
 {
-	/* disallow ancillary data requests from this path */
-	if (msg->msg_control || msg->msg_controllen)
-		return -EINVAL;
-
 	return ____sys_sendmsg(sock, msg, flags, NULL, 0);
 }
 
@@ -2637,10 +2633,6 @@ long __sys_recvmsg_sock(struct socket *sock, struct msghdr *msg,
 			struct user_msghdr __user *umsg,
 			struct sockaddr __user *uaddr, unsigned int flags)
 {
-	/* disallow ancillary data requests from this path */
-	if (msg->msg_control || msg->msg_controllen)
-		return -EINVAL;
-
 	return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
 }

-- 
Jens Axboe

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jann Horn]

On Mon, Apr 27, 2020 at 9:53 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 4/27/20 1:29 PM, Jens Axboe wrote:
> > On 4/27/20 1:20 PM, Jann Horn wrote:
> >> On Sat, Apr 25, 2020 at 10:23 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>> On 4/25/20 11:29 AM, Andreas Smas wrote:
> >>>> Hi,
> >>>>
> >>>> Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
> >>>> particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
> >>>>
> >>>> These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
> >>>>
> >>>> The following hack fixes the problem for me and I get valid timestamps
> >>>> back. Not suggesting this is the real fix as I'm not sure what the
> >>>> implications of this is.
> >>>>
> >>>> Any insight into this would be much appreciated.
> >>>
> >>> It was originally disabled because of a security issue, but I do think
> >>> it's safe to enable again.
> >>>
> >>> Adding the io-uring list and Jann as well, leaving patch intact below.
> >>>
> >>>> diff --git a/net/socket.c b/net/socket.c
> >>>> index 2dd739fba866..689f41f4156e 100644
> >>>> --- a/net/socket.c
> >>>> +++ b/net/socket.c
> >>>> @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
> >>>> struct msghdr *msg,
> >>>>                         struct user_msghdr __user *umsg,
> >>>>                         struct sockaddr __user *uaddr, unsigned int flags)
> >>>>  {
> >>>> -       /* disallow ancillary data requests from this path */
> >>>> -       if (msg->msg_control || msg->msg_controllen)
> >>>> -               return -EINVAL;
> >>>> -
> >>>>         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
> >>>>  }
> >>
> >> I think that's hard to get right. In particular, unix domain sockets
> >> can currently pass file descriptors in control data - so you'd need to
> >> set the file_table flag for recvmsg and sendmsg. And I'm not sure
> >> whether, to make this robust, there should be a whitelist of types of
> >> control messages that are permitted to be used with io_uring, or
> >> something like that...
> >>
> >> I think of ancillary buffers as being kind of like ioctl handlers in
> >> this regard.
> >
> > Good point. I'll send out something that hopefully will be enough to
> > be useful, whole not allowing anything randomly.
>
> That things is a bit of a mess... How about something like this for
> starters?
[...]
> +static bool io_net_allow_cmsg(struct msghdr *msg)
> +{
> +       struct cmsghdr *cmsg;
> +
> +       for_each_cmsghdr(cmsg, msg) {

Isn't this going to dereference a userspace pointer? ->msg_control has
not been copied into the kernel at this point, right?

> +               if (!__io_net_allow_cmsg(cmsg))
> +                       return false;
> +       }
[...]
> @@ -3604,6 +3635,11 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock)
[...]
> +               if (!io_net_allow_cmsg(&kmsg->msg)) {
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
[...]
> @@ -3840,6 +3877,11 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock)
[...]
> +               if (!io_net_allow_cmsg(&kmsg->msg)) {
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
> +

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jens Axboe]

On 4/27/20 2:03 PM, Jann Horn wrote:
> On Mon, Apr 27, 2020 at 9:53 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 4/27/20 1:29 PM, Jens Axboe wrote:
>>> On 4/27/20 1:20 PM, Jann Horn wrote:
>>>> On Sat, Apr 25, 2020 at 10:23 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 4/25/20 11:29 AM, Andreas Smas wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Tried to use io_uring with OP_RECVMSG with ancillary buffers (for my
>>>>>> particular use case I'm using SO_TIMESTAMP for incoming UDP packets).
>>>>>>
>>>>>> These submissions fail with EINVAL due to the check in __sys_recvmsg_sock().
>>>>>>
>>>>>> The following hack fixes the problem for me and I get valid timestamps
>>>>>> back. Not suggesting this is the real fix as I'm not sure what the
>>>>>> implications of this is.
>>>>>>
>>>>>> Any insight into this would be much appreciated.
>>>>>
>>>>> It was originally disabled because of a security issue, but I do think
>>>>> it's safe to enable again.
>>>>>
>>>>> Adding the io-uring list and Jann as well, leaving patch intact below.
>>>>>
>>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>>> index 2dd739fba866..689f41f4156e 100644
>>>>>> --- a/net/socket.c
>>>>>> +++ b/net/socket.c
>>>>>> @@ -2637,10 +2637,6 @@ long __sys_recvmsg_sock(struct socket *sock,
>>>>>> struct msghdr *msg,
>>>>>>                         struct user_msghdr __user *umsg,
>>>>>>                         struct sockaddr __user *uaddr, unsigned int flags)
>>>>>>  {
>>>>>> -       /* disallow ancillary data requests from this path */
>>>>>> -       if (msg->msg_control || msg->msg_controllen)
>>>>>> -               return -EINVAL;
>>>>>> -
>>>>>>         return ____sys_recvmsg(sock, msg, umsg, uaddr, flags, 0);
>>>>>>  }
>>>>
>>>> I think that's hard to get right. In particular, unix domain sockets
>>>> can currently pass file descriptors in control data - so you'd need to
>>>> set the file_table flag for recvmsg and sendmsg. And I'm not sure
>>>> whether, to make this robust, there should be a whitelist of types of
>>>> control messages that are permitted to be used with io_uring, or
>>>> something like that...
>>>>
>>>> I think of ancillary buffers as being kind of like ioctl handlers in
>>>> this regard.
>>>
>>> Good point. I'll send out something that hopefully will be enough to
>>> be useful, whole not allowing anything randomly.
>>
>> That things is a bit of a mess... How about something like this for
>> starters?
> [...]
>> +static bool io_net_allow_cmsg(struct msghdr *msg)
>> +{
>> +       struct cmsghdr *cmsg;
>> +
>> +       for_each_cmsghdr(cmsg, msg) {
> 
> Isn't this going to dereference a userspace pointer? ->msg_control has
> not been copied into the kernel at this point, right?

Possibly... Totally untested, maybe I forgot to mention that :-)
I'll check.

The question was more "in principle" if this was a viable approach. The
whole cmsg_type and cmsg_level is really a mess.

-- 
Jens Axboe

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Christoph Hellwig]

On Mon, Apr 27, 2020 at 02:08:25PM -0600, Jens Axboe wrote:
> Possibly... Totally untested, maybe I forgot to mention that :-)
> I'll check.
> 
> The question was more "in principle" if this was a viable approach. The
> whole cmsg_type and cmsg_level is really a mess.

FYI, I have a series in the works to sort out the set_fs and
casting to/from __user mess around msg_control.  It needs a little
more work, but hopefully I can find some time the next days.

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Jens Axboe]

On 4/27/20 2:10 PM, Christoph Hellwig wrote:
> On Mon, Apr 27, 2020 at 02:08:25PM -0600, Jens Axboe wrote:
>> Possibly... Totally untested, maybe I forgot to mention that :-)
>> I'll check.
>>
>> The question was more "in principle" if this was a viable approach. The
>> whole cmsg_type and cmsg_level is really a mess.
> 
> FYI, I have a series in the works to sort out the set_fs and
> casting to/from __user mess around msg_control.  It needs a little
> more work, but hopefully I can find some time the next days.

That'd be great - looking at it right now, I'd need to refactor
____sys_sendmsg (and ditto receive side) to poke at cmsg before
allowing us to proceed. I'll wait for you to post your series and
base it on top of that, if appropriate.

-- 
Jens Axboe

Re: io_uring, IORING_OP_RECVMSG and ancillary data

[Andreas Smas]

On Mon, Apr 27, 2020 at 1:10 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> FYI, I have a series in the works to sort out the set_fs and
> casting to/from __user mess around msg_control.  It needs a little
> more work, but hopefully I can find some time the next days.

Hi,

Just wanted to check in on if these patches saw the light of day?

Thanks,
Andreas