From owner-svn-src-stable-12@freebsd.org Thu Jul 30 17:10:23 2020 Return-Path: Delivered-To: svn-src-stable-12@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 965BC3AAC97; Thu, 30 Jul 2020 17:10:23 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4BHcNt5LgMz4fNF; Thu, 30 Jul 2020 17:10:22 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 70A4B3C0199; Thu, 30 Jul 2020 17:10:16 +0000 (UTC) Date: Thu, 30 Jul 2020 17:10:16 +0000 From: Brooks Davis To: Rick Macklem Cc: Ian Lepore , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-stable@freebsd.org" , "svn-src-stable-12@freebsd.org" Subject: Re: svn commit: r363625 - stable/12/usr.sbin/mountd Message-ID: <20200730171016.GC94620@spindle.one-eyed-alien.net> References: <202007272318.06RNIFjV005206@repo.freebsd.org> <4d5b871fad9412661c3914a64c8ca0b7a01d1dc6.camel@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LKTjZJSUETSlgu2t" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: 4BHcNt5LgMz4fNF X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of brooks@spindle.one-eyed-alien.net has no SPF policy when checking 199.48.129.229) smtp.mailfrom=brooks@spindle.one-eyed-alien.net X-Spamd-Result: default: False [-2.97 / 15.00]; TO_DN_EQ_ADDR_SOME(0.00)[]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.26)[-0.259]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-0.72)[-0.716]; MIME_GOOD(-0.20)[multipart/signed,text/plain]; DMARC_NA(0.00)[freebsd.org]; AUTH_NA(1.00)[]; RCPT_COUNT_FIVE(0.00)[6]; TO_MATCH_ENVRCPT_SOME(0.00)[]; NEURAL_HAM_SHORT(-1.10)[-1.098]; SIGNED_PGP(-2.00)[]; FORGED_SENDER(0.30)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net]; RCVD_COUNT_ZERO(0.00)[0]; R_SPF_NA(0.00)[no SPF record]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:36236, ipnet:199.48.128.0/22, country:US]; FROM_NEQ_ENVFROM(0.00)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net] X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 17:10:23 -0000 --LKTjZJSUETSlgu2t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 30, 2020 at 03:48:34PM +0000, Rick Macklem wrote: > Rick Macklem wrote: > >Ian Lepore wrote: > >>On Thu, 2020-07-30 at 01:52 +0000, Rick Macklem wrote: > >>> Brooks Davis wrote: > >>> > Author: brooks > >>> > Date: Mon Jul 27 23:18:14 2020 > >>> > New Revision: 363625 > >>> > URL: https://svnweb.freebsd.org/changeset/base/363625 > >>> > > >>> > Log: > >>> > MFC r363439: > >>> > > >>> > Correct a type-mismatch between xdr_long and the variable "bad". > >>> > > >>> > [...] > >>> --> I can't see how the xdr.c code would work for a machine that is > >>> BIG_ENDIAN and where "long" is 64bits, but we don't have any of > >>> those. > >>> > >> > >>mips64 and powerpc64 are both big endian with 64-bit long. > >Oops, I didn't know that. In the past, I've run PowerPC and MIPS, but th= ought > >they both were little endian. (I recall the arches can be run either way= =2E) > > > >Anyhow, take a look at head/lib/libc/xdr/xdr.c and it looks to me like it > >has been broken "forever" (ever since we stopped using a K&R compiler > >that would have always made "long" 32bits). > OK, I took another look at xdr.c and it isn't broken as I thought. >=20 > xdr_long() takes a "long *" argument ("long" in Sun XDR is 32bits), > but then it only passes it as an argument to XDR_PUTLONG(), which is actu= ally > a call to xdrmem_putlong_aligned() or xdrmem_putlong_unaligned(). > For xdrmem_putlong_aligned(), the line is: > *(u_int32_t *)xdrs->x_private =3D htonl((u_int32_t)*lp); > --> where lp is a "long *" >=20 > I'll admit I'm not 100% sure if "(u_int32_t)*lp" gets the correct 32bits = of a 64bit > long pointer for all arches? (I'm not very good at knowing what type cast= s do.) > If this is the equivalent of "u_int32_t t; t =3D *lp; htonl(t); then I th= ink the code is ok? > (At least it makes it clear that it is using 32bits of the value pointed = to by the > argument.) >=20 > For xdrmem_putlong_unaligned(), it does the same thing via: > u_int32_t l; > ?. > l =3D htonl((u_int32_t)*lp); >=20 > --> At least the man page for xdr_long() should be clarified to note it > puts a 32bit quantity on the wire. >=20 > >If anyone has either of these and can set up an NFS server on one of > >them and then try and do an NFSv3 mount that is not allowed, it would > >be interesting to see the packet trace and if the MNT RPC fails, because > >it looks like it will put the high order 32bits on the wire and they'll > >always be 0? > It would still be interesting to test this on a 64bit big endian, but so = long as > the above cast works, it does look like it works for all arches. >=20 > >Just to clarify. The behaviour wasn't broken by this commit. I just > >don't see how the commit fixes anything? > My mistake. Sorry for the noise. >=20 > I now think the commit is correct since it uses "*lp" to get the value be= fore > casting it down to 32bits. Passing in an "int *" was incorrect. >=20 > The code does seem to handle "long *" for 64bit arches, although it > only puts 32bits "on-the-wire". >=20 > rick, who was confused because he knew there was only supposed to be > 32bits go on the wire. Thank you for all the analysis. I'd initially changed all the uses of bad to use xdr_int(), but switched to this "fix" because it's what NetBSD and OpenBSD have been using for over a decade (and there was less churn). I'm happy to flip it the other way if that seems more correct/less confusing. The previous code does in fact cause a 64-bit load of a pointer to an int on 64-bit platforms. I hit this in CheriBSD because that pointer had 4-byte bounds. -- Brooks --LKTjZJSUETSlgu2t Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJfIv72AAoJEKzQXbSebgfAH8EIAIlWUTJIM1GhDrwSAqFMnh38 jJbuQ2WT6LkuO+kghcC+MpOm4pQDEa/0yzmJp3U45LEeTomXi9gm4KkzMRipJzF3 6Lyx3/jaEAI+lf6WoShO9Nx9rR+aLLfY9EkpzzvXoyJg3HC1R6tAx0HIJgKFyDyZ F5rU6blO5XPIJ/iPXNjgoJF+lbXeohfiBobz30vQYR6Vq7fDnCtZh378La0Lce65 4OXAGkfsrGVyq5LAWs52T2MK7G64siw53c2VH/4kXvnTPeLLXGagJZfl7MWPedZY TlJ5obhZ875PWCkPNQsZ7DyQosPjpu4QCu5zMNxkCb62vUsvzxnfCRQthsKEB9s= =i3lH -----END PGP SIGNATURE----- --LKTjZJSUETSlgu2t--