Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 31 Aug 2009 12:30:09 GMT
From:      John Baldwin <jhb@freebsd.org>
To:        freebsd-amd64@FreeBSD.org
Subject:   Re: amd64/138318: [patch] select(2) in i386 emulation can overwrite user data
Message-ID:  <200908311230.n7VCU97q056712@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help
The following reply was made to PR amd64/138318; it has been noted by GNATS.

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
Date: Mon, 31 Aug 2009 08:23:27 -0400

 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?200908311230.n7VCU97q056712>