Re: svn commit: r363625 - stable/12/usr.sbin/mountd
On 30 Jul 2020, at 23:38, Rick Macklem wrote:
> Brooks Davis wrote:
>> On Thu, Jul 30, 2020 at 03:48:34PM +, Rick Macklem wrote:
>>> Rick Macklem wrote:
Ian Lepore wrote:
> On Thu, 2020-07-30 at 01:52 +, 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.
> I think I will try and come up with a man page patch, noting that xdr_long()
> always puts 32bits on the wire, even if long is 64bits for the arch.
>
>>>
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.
> I think your current patch is fine. The confusion is w.r.t. what xdr_long()
> does
> for a 64bit long and I think a man page update may be the way to go.
> --> If you look in xdr.c, xdr_int() assigns the value to a long and then ends
> up truncating it back down, similar to xdr_long().
> --> Some of the stuff in xdr.c is pretty scary for 64bit longs, but it
> all
> seems to work, once you look at it for a while.;-)
>
>> 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.
> Yes. The first time I looked at the code (it was late evening), I misread
>((u_int32_t)*lp) as *((u_int32_t *)lp) and that was why I thought your
> patch
>was broken.
>
> Thanks for doing this and sorry about the noise, rick
> ps: Personally, I've never understood why ANSI C allowed "long" to be 64bits
> on some arches. I still bump into hassles because the old K&R code was
> written assuming long to be 32bits.
Yeah, c.f. XChangeProperty(3) which has a similarly weird interface
(but slightly more so) that seriously confused me a few years ago. If
y
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
Brooks Davis wrote:
>On Thu, Jul 30, 2020 at 03:48:34PM +, Rick Macklem wrote:
>> Rick Macklem wrote:
>> >Ian Lepore wrote:
>> >>On Thu, 2020-07-30 at 01:52 +, 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.
I think I will try and come up with a man page patch, noting that xdr_long()
always puts 32bits on the wire, even if long is 64bits for the arch.
>>
>> >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.
I think your current patch is fine. The confusion is w.r.t. what xdr_long() does
for a 64bit long and I think a man page update may be the way to go.
--> If you look in xdr.c, xdr_int() assigns the value to a long and then ends
up truncating it back down, similar to xdr_long().
--> Some of the stuff in xdr.c is pretty scary for 64bit longs, but it all
seems to work, once you look at it for a while.;-)
>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.
Yes. The first time I looked at the code (it was late evening), I misread
((u_int32_t)*lp) as *((u_int32_t *)lp) and that was why I thought your
patch
was broken.
Thanks for doing this and sorry about the noise, rick
ps: Personally, I've never understood why ANSI C allowed "long" to be 64bits
on some arches. I still bump into hassles because the old K&R code was
written assuming long to be 32bits.
-- Brooks
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
On Thu, Jul 30, 2020 at 03:48:34PM +, Rick Macklem wrote:
> Rick Macklem wrote:
> >Ian Lepore wrote:
> >>On Thu, 2020-07-30 at 01:52 +, 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
signature.asc
Description: PGP signature
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
Rick Macklem wrote:
>Ian Lepore wrote:
>>On Thu, 2020-07-30 at 01:52 +, 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.
-- Ian
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
Ian Lepore wrote: >On Thu, 2020-07-30 at 01:52 +, 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). 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? Just to clarify. The behaviour wasn't broken by this commit. I just don't see how the commit fixes anything? rick, who doesn't have these arches to test on. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
On Thu, 2020-07-30 at 01:52 +, 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. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r363625 - stable/12/usr.sbin/mountd
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".
>
> Way back in r28911 (August 1997, CVS rev 1.22) we imported a NetBSD
> information leak fix via OpenBSD. Unfortunatly we failed to track the
> followup commit that fixed the type of the error code. Apply the change
> from int to long now.
I don't think this is correct.
RFC-1813 defines the error return for a MNT RPC as a 32bit quantity.
Way back when this stuff was written it was in K&R days and "long" was
always a 32bit integer.
If you look at head/lib/libc/xdr/xdr.c you'll see "long" used to refer to 32bit
numbers throughout it. Look near the end, where it does a "longlong" (64bits)
using 2 longs.
The good news w.r.t. this ancient code is that XDR_PUTLONG() assumes 32bits.
Also, note that xdr_int() and xdr_long() do exactly the same thing.
I support int32_t would be preferred to "int" to make sure "bad" is 32bits
and then you can use xdr_int32_t(), which does exactly the same thing as
xdr_int() and about the same thing as xdr_long(). { They all assume a "long"
is 32bits. Scary to look at now that "long" isn't always 32bits. }
--> 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.
I don't think "int bad" was wrong and "long bad" definitely seems wrong for
64bit systems, although the xdr.c code simply ends up putting the low order
32bits on the wire, I think?
rick
Reviewed by: emaste
Reported by: CHERI
Obtained from:CheriBSD
Sponsored by: DARPA
Differential Revision:https://reviews.freebsd.org/D25779
Modified:
stable/12/usr.sbin/mountd/mountd.c
Directory Properties:
stable/12/ (props changed)
Modified: stable/12/usr.sbin/mountd/mountd.c
==
--- stable/12/usr.sbin/mountd/mountd.c Mon Jul 27 21:19:41 2020
(r363624)
+++ stable/12/usr.sbin/mountd/mountd.c Mon Jul 27 23:18:14 2020
(r363625)
@@ -1087,7 +1087,8 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *transp)
struct sockaddr *saddr;
u_short sport;
char rpcpath[MNTPATHLEN + 1], dirpath[MAXPATHLEN];
- int bad = 0, defset, hostset;
+ int defset, hostset;
+ long bad = 0;
sigset_t sighup_mask;
int numsecflavors, *secflavorsp;
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r363625 - stable/12/usr.sbin/mountd
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". Way back in r28911 (August 1997, CVS rev 1.22) we imported a NetBSD information leak fix via OpenBSD. Unfortunatly we failed to track the followup commit that fixed the type of the error code. Apply the change from int to long now. Reviewed by: emaste Reported by: CHERI Obtained from:CheriBSD Sponsored by: DARPA Differential Revision:https://reviews.freebsd.org/D25779 Modified: stable/12/usr.sbin/mountd/mountd.c Directory Properties: stable/12/ (props changed) Modified: stable/12/usr.sbin/mountd/mountd.c == --- stable/12/usr.sbin/mountd/mountd.c Mon Jul 27 21:19:41 2020 (r363624) +++ stable/12/usr.sbin/mountd/mountd.c Mon Jul 27 23:18:14 2020 (r363625) @@ -1087,7 +1087,8 @@ mntsrv(struct svc_req *rqstp, SVCXPRT *transp) struct sockaddr *saddr; u_short sport; char rpcpath[MNTPATHLEN + 1], dirpath[MAXPATHLEN]; - int bad = 0, defset, hostset; + int defset, hostset; + long bad = 0; sigset_t sighup_mask; int numsecflavors, *secflavorsp; ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
