From owner-freebsd-amd64@FreeBSD.ORG Mon Aug 31 12:23:59 2009 Return-Path: Delivered-To: freebsd-amd64@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 64D94106566C; Mon, 31 Aug 2009 12:23:59 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 252AB8FC19; Mon, 31 Aug 2009 12:23:59 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id CA16E46B29; Mon, 31 Aug 2009 08:23:58 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 8F4DF8A040; Mon, 31 Aug 2009 08:23:57 -0400 (EDT) From: John Baldwin To: freebsd-amd64@freebsd.org, Peter Jeremy Date: Mon, 31 Aug 2009 08:23:27 -0400 User-Agent: KMail/1.9.7 References: <200908292303.n7TN3WLe081443@server.vk2pj.dyndns.org> In-Reply-To: <200908292303.n7TN3WLe081443@server.vk2pj.dyndns.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200908310823.27390.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Mon, 31 Aug 2009 08:23:57 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.3 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: FreeBSD-gnats-submit@freebsd.org Subject: Re: amd64/138318: [patch] select(2) in i386 emulation can overwrite user data X-BeenThere: freebsd-amd64@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to the AMD64 platform List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 31 Aug 2009 12:23:59 -0000 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 > #include > #include > #include > > 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 > #include > > -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