Date: Mon, 31 Aug 2009 08:23:27 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-amd64@freebsd.org, Peter Jeremy <peterjeremy@optushome.com.au> Cc: FreeBSD-gnats-submit@freebsd.org Subject: Re: amd64/138318: [patch] select(2) in i386 emulation can overwrite user data Message-ID: <200908310823.27390.jhb@freebsd.org> In-Reply-To: <200908292303.n7TN3WLe081443@server.vk2pj.dyndns.org> References: <200908292303.n7TN3WLe081443@server.vk2pj.dyndns.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 29 August 2009 7:03:32 pm Peter Jeremy wrote:
>
> >Number: 138318
> >Category: amd64
> >Synopsis: [patch] select(2) in i386 emulation can overwrite user data
> >Confidential: no
> >Severity: critical
> >Priority: high
> >Responsible: freebsd-amd64
> >State: open
> >Quarter:
> >Keywords:
> >Date-Required:
> >Class: sw-bug
> >Submitter-Id: current-users
> >Arrival-Date: Sat Aug 29 23:10:01 UTC 2009
> >Closed-Date:
> >Last-Modified:
> >Originator: Peter Jeremy
> >Release: FreeBSD 8.0-BETA2 amd64
> >Organization:
> n/a
> >Environment:
> System: FreeBSD server.vk2pj.dyndns.org 8.0-BETA2 FreeBSD 8.0-BETA2 #8: Sat
Aug 8 21:54:17 EST 2009
root@server.vk2pj.dyndns.org:/var/obj/usr/src/sys/server amd64
>
> Code inspection shows that this bug still exists in 9-current.
>
> >Description:
> The select() wrapper for freebsd32 and linux32 emulation does not
> wrap the fd_set arguments. fd_set is an array of fd_mask - which
> is 'long' on all architectures. This means that kern_select() on
> 64-bit kernels expects that the fd_set arguments are arrays of
> 8-byte objects whilst 32-bit code passes arrays of 4-byte objects.
> As a result, the kernel can overwrite 4-bytes more than userland
> expects.
>
> This obviously breaks 32-bit sshd with PrivilegeSeparation enabled
> but may have other less-obvious breakage.
>
> >How-To-Repeat:
>
> Run a FreeBSD/i386 sshd on FreeBSD/amd64:
>
> server# file /tank/aspire/usr/sbin/sshd
> /tank/aspire/usr/sbin/sshd: ELF 32-bit LSB executable, Intel 80386, version
1 (FreeBSD), dynamically linked (uses shared libs), for FreeBSD 8.0 (800096),
stripped
> server# /tank/aspire/usr/sbin/sshd -p 8022 -d -o UsePrivilegeSeparation=yes
> debug1: sshd version OpenSSH_5.1p1 FreeBSD-20080801
> ...
> debug1: SSH2_MSG_KEX_DH_GEX_REQUEST received
> debug1: SSH2_MSG_KEX_DH_GEX_GROUP sent
> debug1: expecting SSH2_MSG_KEX_DH_GEX_INIT
> buffer_put_bignum2_ret: BN too small
> buffer_put_bignum2: buffer error
> debug1: do_cleanup
> debug1: do_cleanup
> server#
>
> As a more contrived (but more obvious) example, compile the following
> code on i386 and run it on amd64:
>
> ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ----
> #include <sys/select.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int main(void)
> {
> fd_set *fd, *rd, *wr, *ex;
> int r;
> fd = malloc(sizeof(fd_mask) * 3 * 4);
> memset(fd, 0xa5, sizeof(fd_mask) * 3 * 4);
> rd = (fd_set *)&fd->fds_bits[1];
> wr = (fd_set *)&fd->fds_bits[5];
> ex = (fd_set *)&fd->fds_bits[9];
> rd->fds_bits[0] = wr->fds_bits[0] = ex->fds_bits[0] = 0;
> FD_SET(0, rd);
> FD_SET(1, wr);
> FD_SET(2, wr);
> FD_SET(0, ex);
> FD_SET(1, ex);
> FD_SET(2, ex);
> printf("read: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[0], fd->fds_bits[1], fd->fds_bits[2], fd->fds_bits[3]);
> printf("write: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[4], fd->fds_bits[5], fd->fds_bits[6], fd->fds_bits[7]);
> printf("except: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[8], fd->fds_bits[9], fd->fds_bits[10], fd->fds_bits[11]);
> r = select(3, rd, wr, ex, NULL);
> printf("select returns %d:\n", r);
> printf("read: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[0], fd->fds_bits[1], fd->fds_bits[2], fd->fds_bits[3]);
> printf("write: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[4], fd->fds_bits[5], fd->fds_bits[6], fd->fds_bits[7]);
> printf("except: %08lx %08lx %08lx %08lx\n",
> fd->fds_bits[8], fd->fds_bits[9], fd->fds_bits[10], fd->fds_bits[11]);
> return 0;
> }
> ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ---- 8-< ----
> server# /tank/aspire/root/seltest
> read: a5a5a5a5 00000001 a5a5a5a5 a5a5a5a5
> write: a5a5a5a5 00000006 a5a5a5a5 a5a5a5a5
> except: a5a5a5a5 00000007 a5a5a5a5 a5a5a5a5
> read: a5a5a5a5 00000000 00000000 a5a5a5a5
> write: a5a5a5a5 00000006 00000000 a5a5a5a5
> except: a5a5a5a5 00000000 00000000 a5a5a5a5
> server#
>
> >Fix:
> Either:
> 1) Change the definition of fd_mask from ulong to uint32 (at least within
> the kernel)
> 2) Wrap the fd_set arguments on freebsd32 and linux for 64-bit kernels.
>
> The latter may appear stylistically cleaner but requires significantly
> more effort because the fd_set copyin()s are all currently done within
> kern_select() and are non-trivial blocks of code to optimise performance
> whilst minimising kvm usage. The attached patch therefore implements
> the former behaviour:
> Index: select.h
> ===================================================================
> RCS file: /usr/ncvs/src/sys/sys/select.h,v
> retrieving revision 1.20
> diff -u -r1.20 select.h
> --- select.h 6 Jan 2006 22:12:46 -0000 1.20
> +++ select.h 29 Aug 2009 23:00:08 -0000
> @@ -39,7 +39,7 @@
> #include <sys/_timeval.h>
> #include <sys/timespec.h>
>
> -typedef unsigned long __fd_mask;
> +typedef __uint32_t __fd_mask;
> #if __BSD_VISIBLE
> typedef __fd_mask fd_mask;
> #endif
I think this would break the ABI of select() for old binaries (compiled with
fd_mask == long) on 64-bit big-endian archs (i.e. sparc64). I think you
could manage 2) by having kern_select() accept an 'int nfdbits' parameter
that replaces 'NFDBITS' when computing nfdbits. That will work fine for now
as all our COMPAT32 archs are little-endian. If we wanted to support
COMPAT32 on big endian then you could pass an operations vector to
kern_select() that has wrappers for copying in/out fd_set lists similar to
what is done with kern_kevent().
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200908310823.27390.jhb>