svn commit: r363625 - stable/12/usr.sbin/mountd

[Brooks Davis]

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 thought
> >they both were little endian. (I recall the arches can be run either way.)
> >
> >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.
> 
> 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 actually
> a call to xdrmem_putlong_aligned() or xdrmem_putlong_unaligned().
> For xdrmem_putlong_aligned(), the line is:
>        *(u_int32_t *)xdrs->x_private = htonl((u_int32_t)*lp);
>       --> where lp is a "long *"
> 
> 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 casts do.)
> If this is the equivalent of "u_int32_t t; t = *lp; htonl(t); then I think the code is ok?
> (At least it makes it clear that it is using 32bits of the value pointed to by the
>  argument.)
> 
> For xdrmem_putlong_unaligned(), it does the same thing via:
>         u_int32_t l;
>         ?.
>         l = htonl((u_int32_t)*lp);
> 
> --> At least the man page for xdr_long() should be clarified to note it
>       puts a 32bit quantity on the wire.
> 
> >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.
> 
> >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.
> 
> I now think the commit is correct since it uses "*lp" to get the value before
> casting it down to 32bits. Passing in an "int *" was incorrect.
> 
> The code does seem to handle "long *" for 64bit arches, although it
> only puts 32bits "on-the-wire".
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20200730/06ce046e/attachment.sig>