Date: Sun, 30 Aug 2009 17:44:52 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: freebsd-amd64@freebsd.org Cc: freebsd-current@freebsd.org Subject: Re: sshd failing in jail Message-ID: <20090830144452.GK1881@deviant.kiev.zoral.com.ua> In-Reply-To: <20090829233454.GA13036@server.vk2pj.dyndns.org> References: <20090824193344.GA34949@server.vk2pj.dyndns.org> <20090829233454.GA13036@server.vk2pj.dyndns.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--ef8eQmdvO1j1gFMO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, Aug 30, 2009 at 09:34:54AM +1000, Peter Jeremy wrote:
> [Redirected to amd64 because this is an amd64 kernel bug]
>=20
> On 2009-Aug-25 05:33:44 +1000, Peter Jeremy <peterjeremy@optushome.com.au=
> wrote:
> >I am attempting to build an i386 jail on an amd64 box to build
> >packages for my netbook. The host is running -current from just over
> >two weeks ago and the jail is -current from early June. The jail was
> >built by doing a dump|restore of my netbook and then tweaking various
> >config files to give it a new identity. The jail's devfs is using
> >"devfsrules_jail" from /etc/default/devfs.rules.
> >
> >The jail starts OK but when I attempt to ssh into it, I just get
> >"Connection closed by <jail IP address>".
>=20
> Turns out this is a bug in the 32-bit select(2) wrapper on 64-bit
> kernels. The userland fd_set arguments are not wrapped but passed
> directly to kern_select(). Unfortunately, fd_set is (effectively) an
> array of longs which means kern_select() assumes fd_set is a multiple
> of 8-bytes whilst userland assumes it is a multiple of 4 bytes. As a
> result, the kernel can over-write an extra 4 bytes of user memory. In
> the case of sshd, this causes part of the RSA host key to be trashed
> when privilege separation mode is enabled.
>=20
> This bug also affects linux emulation on amd64 and potentially affects
> any other 64-bit kernels with 32-bit emulation modes. I have raised
> amd64/138318 to cover it.
I do not think that we can go the proposed route, since changing the
type of __fd_mask changes the type of fd_set. The later would not
affect the kernel ABI, but definitely changes the ABI of any code that
passes fd_sets.
Also, looking closely at the issue you found, I think that copyin
is the same problematic as copyout, since we can end up reading
one more word then userspace supplied. This is not a problem only
because most user code keeps fd_sets on stack.
Could you test that the patch below fixes real sshd issue. At least,
it passes your select test from the PR.
diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/f=
reebsd32_misc.c
index 466aab4..71b22aa 100644
--- a/sys/compat/freebsd32/freebsd32_misc.c
+++ b/sys/compat/freebsd32/freebsd32_misc.c
@@ -589,7 +589,8 @@ freebsd32_select(struct thread *td, struct freebsd32_se=
lect_args *uap)
* XXX big-endian needs to convert the fd_sets too.
* XXX Do pointers need PTRIN()?
*/
- return (kern_select(td, uap->nd, uap->in, uap->ou, uap->ex, tvp));
+ return (kern_select(td, uap->nd, uap->in, uap->ou, uap->ex, tvp,
+ sizeof(int32_t) * 8));
}
=20
/*
diff --git a/sys/compat/linux/linux_misc.c b/sys/compat/linux/linux_misc.c
index 267da07..1d5eaf8 100644
--- a/sys/compat/linux/linux_misc.c
+++ b/sys/compat/linux/linux_misc.c
@@ -522,7 +522,7 @@ linux_select(struct thread *td, struct linux_select_arg=
s *args)
tvp =3D NULL;
=20
error =3D kern_select(td, args->nfds, args->readfds, args->writefds,
- args->exceptfds, tvp);
+ args->exceptfds, tvp, sizeof(l_int) * 8);
=20
#ifdef DEBUG
if (ldebug(select))
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index bd0f279..6831fe8 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -774,12 +774,13 @@ select(td, uap)
} else
tvp =3D NULL;
=20
- return (kern_select(td, uap->nd, uap->in, uap->ou, uap->ex, tvp));
+ return (kern_select(td, uap->nd, uap->in, uap->ou, uap->ex, tvp,
+ NFDBITS));
}
=20
int
kern_select(struct thread *td, int nd, fd_set *fd_in, fd_set *fd_ou,
- fd_set *fd_ex, struct timeval *tvp)
+ fd_set *fd_ex, struct timeval *tvp, int abi_nfdbits)
{
struct filedesc *fdp;
/*
@@ -792,7 +793,7 @@ kern_select(struct thread *td, int nd, fd_set *fd_in, f=
d_set *fd_ou,
fd_mask *ibits[3], *obits[3], *selbits, *sbp;
struct timeval atv, rtv, ttv;
int error, timo;
- u_int nbufbytes, ncpbytes, nfdbits;
+ u_int nbufbytes, ncpbytes, ncpubytes, nfdbits;
=20
if (nd < 0)
return (EINVAL);
@@ -806,6 +807,7 @@ kern_select(struct thread *td, int nd, fd_set *fd_in, f=
d_set *fd_ou,
*/
nfdbits =3D roundup(nd, NFDBITS);
ncpbytes =3D nfdbits / NBBY;
+ ncpubytes =3D roundup(nd, abi_nfdbits) / NBBY;
nbufbytes =3D 0;
if (fd_in !=3D NULL)
nbufbytes +=3D 2 * ncpbytes;
@@ -832,9 +834,11 @@ kern_select(struct thread *td, int nd, fd_set *fd_in, =
fd_set *fd_ou,
ibits[x] =3D sbp + nbufbytes / 2 / sizeof *sbp; \
obits[x] =3D sbp; \
sbp +=3D ncpbytes / sizeof *sbp; \
- error =3D copyin(name, ibits[x], ncpbytes); \
+ error =3D copyin(name, ibits[x], ncpubytes); \
if (error !=3D 0) \
goto done; \
+ bzero((char *)ibits[x] + ncpubytes, \
+ ncpbytes - ncpubytes); \
} \
} while (0)
getbits(fd_in, 0);
@@ -888,7 +892,7 @@ done:
if (error =3D=3D EWOULDBLOCK)
error =3D 0;
#define putbits(name, x) \
- if (name && (error2 =3D copyout(obits[x], name, ncpbytes))) \
+ if (name && (error2 =3D copyout(obits[x], name, ncpubytes))) \
error =3D error2;
if (error =3D=3D 0) {
int error2;
diff --git a/sys/sys/syscallsubr.h b/sys/sys/syscallsubr.h
index d0f209c..e1c83cc 100644
--- a/sys/sys/syscallsubr.h
+++ b/sys/sys/syscallsubr.h
@@ -170,7 +170,7 @@ int kern_sched_rr_get_interval(struct thread *td, pid_t=
pid,
int kern_semctl(struct thread *td, int semid, int semnum, int cmd,
union semun *arg, register_t *rval);
int kern_select(struct thread *td, int nd, fd_set *fd_in, fd_set *fd_ou,
- fd_set *fd_ex, struct timeval *tvp);
+ fd_set *fd_ex, struct timeval *tvp, int abi_nfdbits);
int kern_sendfile(struct thread *td, struct sendfile_args *uap,
struct uio *hdr_uio, struct uio *trl_uio, int compat);
int kern_sendit(struct thread *td, int s, struct msghdr *mp, int flags,
--ef8eQmdvO1j1gFMO
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkqakGMACgkQC3+MBN1Mb4hPJgCgzkRRw85CqSG0dRODxYD4h6HE
bkcAn1M/oT7H9vmpIJHOTd7++i7VhKt+
=NGrs
-----END PGP SIGNATURE-----
--ef8eQmdvO1j1gFMO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090830144452.GK1881>