Date: Sun, 30 Aug 2009 09:03:32 +1000 (EST) From: Peter Jeremy <peterjeremy@optushome.com.au> To: FreeBSD-gnats-submit@FreeBSD.org Subject: amd64/138318: [patch] select(2) in i386 emulation can overwrite user data Message-ID: <200908292303.n7TN3WLe081443@server.vk2pj.dyndns.org> Resent-Message-ID: <200908292310.n7TNA2Ld075688@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200908292303.n7TN3WLe081443>