| Summary: | [libc] [patch] select(2) in i386 emulation can overwrite user data | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Peter Jeremy <PeterJeremy> | ||||
| Component: | amd64 | Assignee: | freebsd-amd64 (Nobody) <amd64> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | 8.0-BETA2 | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
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 State Changed From-To: open->closed Different patch was committed to HEAD and stable/8. |
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. 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: 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#