[WTF?] random test in netlink_sendmsg()

[WTF?] random test in netlink_sendmsg()

[Al Viro]

	In netlink_sendmsg() we have the following:

        if (netlink_tx_is_mmaped(sk) &&
            msg->msg_iov->iov_base == NULL) {
                err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
                                           siocb);
                goto out;
        }

Now, suppose sendmsg(2) is called with msg.msg_iovlen == 0.  We'll have
->msg_iov in kernel-side copy pointing at the uninitialized array in
stack frame of ___sys_sendmsg() - neither new nor old code touches elements
past the first msg_iovlen ones.  So in that case it checks if an
uninitialized word on stack is zero.

	What is that check trying to do?  Is that simply missing
"(msg->msg_iovlen > 0) &&"?  And why on the Earth didn't it simply use
zero msg_iovlen as the indicator, instead of messing with iovec contents?
Obviously too late to change, but... ouch.

Patrick, it had been that way since your commit last year ("netlink: implement
memory mapped sendmsg()"); could you explain what's the intended ABI?

Incidentally, WTF is "atomic_read(&nlk->mapped) > 1" part of check in
netlink_mmap_sendmsg() trying to achieve?  AFAICS, ->mapped tries to
keep track of the number of VMAs, right?  If so, it's bloody pointless -
one can have memory accessible in more than one process without any
extra VMAs.  Just clone(2) with CLONE_VM.  Voila - child shares the
entire address space.  No extra VMAs or calls of ->open() in sight...

Re: [WTF?] random test in netlink_sendmsg()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, 28 Nov 2014 06:23:15 +0000

> 	In netlink_sendmsg() we have the following:
> 
>         if (netlink_tx_is_mmaped(sk) &&
>             msg->msg_iov->iov_base == NULL) {
>                 err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
>                                            siocb);
>                 goto out;
>         }
> 
> Now, suppose sendmsg(2) is called with msg.msg_iovlen == 0.  We'll have
> ->msg_iov in kernel-side copy pointing at the uninitialized array in
> stack frame of ___sys_sendmsg() - neither new nor old code touches elements
> past the first msg_iovlen ones.  So in that case it checks if an
> uninitialized word on stack is zero.
> 
> 	What is that check trying to do?  Is that simply missing
> "(msg->msg_iovlen > 0) &&"?  And why on the Earth didn't it simply use
> zero msg_iovlen as the indicator, instead of messing with iovec contents?
> Obviously too late to change, but... ouch.

I think it's simply trying to say: if nothing in the given iovec, use
the mmap() netlink area for the data.

I cannot vouch for the correctness of this test.

If we take the netlink_mmap_sendmsg() path, msg->msg_iov is not
accessed at all, so it cannot be a huge problem.

Re: [WTF?] random test in netlink_sendmsg()

[Al Viro]

On Fri, Dec 12, 2014 at 03:34:33PM -0500, David Miller wrote:
> > 	What is that check trying to do?  Is that simply missing
> > "(msg->msg_iovlen > 0) &&"?  And why on the Earth didn't it simply use
> > zero msg_iovlen as the indicator, instead of messing with iovec contents?
> > Obviously too late to change, but... ouch.
> 
> I think it's simply trying to say: if nothing in the given iovec, use
> the mmap() netlink area for the data.
> 
> I cannot vouch for the correctness of this test.
> 
> If we take the netlink_mmap_sendmsg() path, msg->msg_iov is not
> accessed at all, so it cannot be a huge problem.

Yes, but decision whether to take that path or not is random in case of
msg_iovlen being 0...

What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
to it and msg.msg_iovlen is 0?  Userland ABI question - not a "do we have
a security hole there" one...  As it is, it might decide to do
netlink_mmap_sendmsg() or it might decide to act as if we hadn't done
NETLINK_TX_RING at all.  In _both_ cases it won't try to dereference
kernel-side ->msg_iov[0].iov_base - the former won't look at msg_iov at all,
the latter will ask to copy 0 bytes from it, which will succeed and do nothing
whatsoever.  Which path is taken depends on whether an uninitialized array
on stack frame of ___sys_sendmsg() happens to begin with NULL.

IMO that's bogus - behaviour ought to be at least deterministic...

Re: [WTF?] random test in netlink_sendmsg()

[Florian Westphal]

Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Fri, Dec 12, 2014 at 03:34:33PM -0500, David Miller wrote:
> > > 	What is that check trying to do?  Is that simply missing
> > > "(msg->msg_iovlen > 0) &&"?  And why on the Earth didn't it simply use
> > > zero msg_iovlen as the indicator, instead of messing with iovec contents?
> > > Obviously too late to change, but... ouch.
> > 
> > I think it's simply trying to say: if nothing in the given iovec, use
> > the mmap() netlink area for the data.
> > 
> > I cannot vouch for the correctness of this test.
> > 
> > If we take the netlink_mmap_sendmsg() path, msg->msg_iov is not
> > accessed at all, so it cannot be a huge problem.
> 
> Yes, but decision whether to take that path or not is random in case of
> msg_iovlen being 0...
> 
> What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
> that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
> to it and msg.msg_iovlen is 0?  Userland ABI question

IIRC userland, after filling txring with at least one new netlink
message, needs to call this to tell kernel to start processing the
messages in the tx ring.

> a security hole there" one...  As it is, it might decide to do
> netlink_mmap_sendmsg() or it might decide to act as if we hadn't done
> NETLINK_TX_RING at all.

It should definitely be netlink_mmap_sendmsg() to commence messsage
processing.

Re: [WTF?] random test in netlink_sendmsg()

[Al Viro]

On Fri, Dec 12, 2014 at 10:50:17PM +0100, Florian Westphal wrote:

> > What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
> > that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
> > to it and msg.msg_iovlen is 0?  Userland ABI question
> 
> IIRC userland, after filling txring with at least one new netlink
> message, needs to call this to tell kernel to start processing the
> messages in the tx ring.

AFAICS, the suggested way to initiate transmission is sendto(fd, NULL, 0, ...),
not sendmsg(2).  _That_ is equivalent to msg.msg_iovlen = 1, msg.msg_iov =
&(struct iovec){NULL, 0} (and that explains the origin of the odd check,
I suppose).  The case where it goes random is different - msg.msg_iovlen = 0,
msg.msg_iov - anything, will be ignored by sys_sendmsg().

Re: [WTF?] random test in netlink_sendmsg()

[Florian Westphal]

Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Fri, Dec 12, 2014 at 10:50:17PM +0100, Florian Westphal wrote:
> 
> > > What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
> > > that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
> > > to it and msg.msg_iovlen is 0?  Userland ABI question
> > 
> > IIRC userland, after filling txring with at least one new netlink
> > message, needs to call this to tell kernel to start processing the
> > messages in the tx ring.
> 
> AFAICS, the suggested way to initiate transmission is sendto(fd, NULL, 0, ...),
> not sendmsg(2).

Indeed, you're right.

Re: [WTF?] random test in netlink_sendmsg()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, 12 Dec 2014 21:32:43 +0000

> What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
> that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
> to it and msg.msg_iovlen is 0?

We had a similar issue with msg_name/msg_namelen and we ended up saying
that if msg_namelen is zero then we force msg_name to NULL.

Re: [WTF?] random test in netlink_sendmsg()

[Al Viro]

On Fri, Dec 12, 2014 at 08:07:58PM -0500, David Miller wrote:
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Fri, 12 Dec 2014 21:32:43 +0000
> 
> > What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
> > that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
> > to it and msg.msg_iovlen is 0?
> 
> We had a similar issue with msg_name/msg_namelen and we ended up saying
> that if msg_namelen is zero then we force msg_name to NULL.

Hmm...  The thing is, there might be legitimate users with empty payload,
making this call for the sake of SCM_CREDENTIALS.  IOW, what should happen
if we have
	msg_iovlen = 0
	msg_iov = <anything>
	msg_control = &cmsg
	msg_controllen = cmsg_len
Sure, both paths will pass creds, but what about the payload?  And the number
of datagram actually transmitted, for that matter?

Re: [WTF?] random test in netlink_sendmsg()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 13 Dec 2014 01:54:15 +0000

> On Fri, Dec 12, 2014 at 08:07:58PM -0500, David Miller wrote:
>> From: Al Viro <viro@ZenIV.linux.org.uk>
>> Date: Fri, 12 Dec 2014 21:32:43 +0000
>> 
>> > What do we want sendmsg(fd, &msg, 0) to do when fd is AF_NETLINK socket
>> > that had setsockopt(fd, SOL_NETLINK, NETLINK_TX_RING, ...) successfully done
>> > to it and msg.msg_iovlen is 0?
>> 
>> We had a similar issue with msg_name/msg_namelen and we ended up saying
>> that if msg_namelen is zero then we force msg_name to NULL.
> 
> Hmm...  The thing is, there might be legitimate users with empty payload,
> making this call for the sake of SCM_CREDENTIALS.  IOW, what should happen
> if we have
> 	msg_iovlen = 0
> 	msg_iov = <anything>
> 	msg_control = &cmsg
> 	msg_controllen = cmsg_len
> Sure, both paths will pass creds, but what about the payload?  And the number
> of datagram actually transmitted, for that matter?

Ok, so we just adjust the AF_PACKET check to test msg_iovlen==1 as
well, and that takes care of that case.

Right?

Re: [WTF?] random test in netlink_sendmsg()

[Al Viro]

On Fri, Dec 12, 2014 at 09:33:13PM -0500, David Miller wrote:

> Ok, so we just adjust the AF_PACKET check to test msg_iovlen==1 as
> well, and that takes care of that case.

AF_NETLINK, I suppose?  AF_PACKET is avoiding all those contortions - they
simply do
        if (po->tx_ring.pg_vec)
                return tpacket_snd(po, msg);
        else
                return packet_snd(sock, msg, len);
IOW, if you have done PACKET_TX_RING, you get msg_iov completely ignored and
tx_ring used as data source, otherwise you get data coming from msg_iov.

For AF_NETLINK what you suggest would work, AFAICS.  It's still bloody weird,
(if we want to be able to mix from-msg_iov and from-tx_ring sends on the same
socket, I'd probably go with MSG_<something> in flags), but existing userland
ABI is existing userland ABI...

So in term of msg_iter, it turns into
	/* It's a really convoluted way for userland to ask for mmaped
	 * sendmsg(), but that's what we've got...  */
	if (netlink_tx_is_mmaped(sk) &&
	    msg->msg_iter.type == KVEC_ITER &&
	    msg->msg_iter.nr_segs == 1 &&
	    msg->msg_iter.iov->iov_base == NULL) {
		err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
					   siocb);
                goto out;
	}

OK, that works...

Next fun place: AF_CAIF/SOCK_SEQPACKET has sendmsg() treating NULL
msg_iov[0].iov_base as EINVAL.  No idea why - without that check zero-length
sendmsg() with such msg_iov would work and non-zero-length one would
fail with EFAULT instead.  And this check is just as random in case of
msg_iovlen being 0.  Could CAIF folks explain what's going on there?
My preference would be to remove that check completely...

Re: [WTF?] random test in netlink_sendmsg()

[Al Viro]

On Sat, Dec 13, 2014 at 03:25:00AM +0000, Al Viro wrote:
> 	    msg->msg_iter.type == KVEC_ITER &&

ITER_IOVEC, that is.  And that way it even works...  Are you OK with the
commit below?

netlink: make the check for "send from tx_ring" deterministic
    
As it is, zero msg_iovlen means that the first iovec in the kernel
array of iovecs is left uninitialized, so checking if its ->iov_base
is NULL is random.  Since the real users of that thing are doing
sendto(fd, NULL, 0, ...), they are getting msg_iovlen = 1 and
msg_iov[0] = {NULL, 0}, which is what this test is trying to catch.
As suggested by davem, let's just check that msg_iovlen was 1 and
msg_iov[0].iov_base was NULL - _that_ is well-defined and it catches
what we want to catch.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index cc9bcf0..5bcb58c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2304,7 +2304,11 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 			goto out;
 	}
 
+        /* It's a really convoluted way for userland to ask for mmaped
+	 * sendmsg(), but that's what we've got...  */
 	if (netlink_tx_is_mmaped(sk) &&
+	    msg->msg_iter.type == ITER_IOVEC &&
+	    msg->msg_iter.nr_segs == 1 &&
 	    msg->msg_iter.iov->iov_base == NULL) {
 		err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
 					   siocb);

Re: [WTF?] random test in netlink_sendmsg()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 13 Dec 2014 04:51:33 +0000

> On Sat, Dec 13, 2014 at 03:25:00AM +0000, Al Viro wrote:
>> 	    msg->msg_iter.type == KVEC_ITER &&
> 
> ITER_IOVEC, that is.  And that way it even works...  Are you OK with the
> commit below?

No objection, looks perfectly fine.