> On Jun 29, 2018, at 4:15 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > On Fri, 2018-06-29 at 16:02 -0400, Chuck Lever wrote: >>> On Jun 29, 2018, at 3:48 PM, Trond Myklebust <trondmy@hammerspace.c >>> om> wrote: >>> >>> On Fri, 2018-06-29 at 15:33 -0400, Chuck Lever wrote: >>>>> On Jun 29, 2018, at 3:29 PM, Trond Myklebust <trondmy@hammerspa >>>>> ce.c >>>>> om> wrote: >>>>> >>>>> On Fri, 2018-06-29 at 14:42 -0400, Steve Dickson wrote: >>>>>> From: Daniel Sands <dnsands@xxxxxxxxxx> >>>>>> >>>>>> The cause is that the xdr_putlong uses a long to store the >>>>>> converted value, then passes it to fwrite as a byte buffer. >>>>>> Only the first 4 bytes are written, which is okay for a LE >>>>>> system after byteswapping, but writes all zeroes on BE >>>>>> systems. >>>>>> >>>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1261738 >>>>>> >>>>>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx> >>>>>> --- >>>>>> v2: Added bounds checking >>>>>> Changed from unsigned to signed >>>>>> >>>>>> src/xdr_stdio.c | 14 +++++++++++--- >>>>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c >>>>>> index 4410262..b902acd 100644 >>>>>> --- a/src/xdr_stdio.c >>>>>> +++ b/src/xdr_stdio.c >>>>>> @@ -103,10 +103,12 @@ xdrstdio_getlong(xdrs, lp) >>>>>> XDR *xdrs; >>>>>> long *lp; >>>>>> { >>>>>> + int32_t mycopy; >>>>>> >>>>>> - if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>>> x_private) >>>>>> >>>>>> != 1) >>>>>> + if (fread(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>>> x_private) != 1) >>>>>> >>>>>> return (FALSE); >>>>>> - *lp = (long)ntohl((u_int32_t)*lp); >>>>>> + >>>>>> + *lp = (long)ntohl(mycopy); >>>>>> return (TRUE); >>>>>> } >>>>>> >>>>>> @@ -115,8 +117,14 @@ xdrstdio_putlong(xdrs, lp) >>>>>> XDR *xdrs; >>>>>> const long *lp; >>>>>> { >>>>>> - long mycopy = (long)htonl((u_int32_t)*lp); >>>>>> + int32_t mycopy; >>>>>> + >>>>>> +#if defined(_LP64) >>>>>> + if ((*lp > INT32_MAX) || (*lp < INT32_MIN)) >>>>>> + return (FALSE); >>>>> >>>>> The will work just fine if your cast is: >>>>> int a; >>>>> >>>>> lp = (long)a; >>>>> >>>>> ...but it won't do the right thing for >>>>> >>>>> unsigned int b = UINT32_MAX; >>>>> >>>>> lp = (long)b; /* set lp to UINT32_MAX */ >>>> >>>> mycopy is an int32_t now. Does that help? >>> >>> Not really. 'mycopy' isn't involved in the check above, and *lp == >>> UINT32_MAX should in either case get cast to the same 32-bit bit >>> pattern whether mycopy is an int32_t or a u_int32_t. >>> It's when you try to cast back from 'mycopy' to a long type that >>> the >>> two can end up differing (an int32_t might get sign extended). >> >> AFAICT we are casting *lp to an int32_t in this function. mycopy >> is not being cast to a long here. >> >> Do you mean xdrstdio_GETlong needs further protection? Could be, >> there is a cast of an int32_t to a long there. >> >> I agree that we should vet xdrstdio_getlong and xdrstdio_putlong for >> sign-extension issues very carefully. >> > > The worry for xdrstdio_putlong() should be how we handle the truncation > when going from (long)-> big endian (int32_t/u_int32_t). We don't want > to silently lose information. To put it in terms of an API contract: If the value in *lp cannot be expressed in 32 bits, xdrstdio_putlong returns FALSE. > AFAICS, when casting from a 'long' type, whether 'mycopy' is signed or > unsigned doesn't really matter when considering what ends up getting > encoded, as long as it is of size 32-bits. The exception would be if > sizeof(long) < 4, but libtirpc is broken for that case anyway... > >>>>> Since there us no dedicated xdrstdio_putulong() to use for >>>>> unsigned >>>>> integers, we need the check to work for both cases. >>>>> For that reason, I think the above check either has to be: >>>>> >>>>> if (*lp == (long)((u_int32_t)*lp)) >>>>> return (FALSE); >>>>> >>>>> ...or something uglier like >>>>> >>>>> if (*lp > UINT32_MAX || *lp < INT32_MIN) >>>>> return (FALSE); On 32 bit systems, long ranges between INT32_MIN and INT32_MAX. The more narrow range seems to me to be the most portable, as it guarantees that this API works for exactly the same set of values on both 32- and 64-bit systems. Thus I would prefer to exclude the range between INT32_MAX and UINT32_MAX. >>>>>> +#endif >>>>>> >>>>>> + mycopy = (int32_t)htonl((int32_t)*lp); >>>>>> if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>>> x_private) != 1) >>>>>> >>>>>> return (FALSE); >>>>>> return (TRUE); >>>>> >>>>> -- >>>>> Trond Myklebust >>>>> CTO, Hammerspace Inc >>>>> 4300 El Camino Real, Suite 105 >>>>> Los Altos, CA 94022 >>>>> www.hammer.space >>>>> >>>>> N‹§ēæėrļ›yúčšØbēXŽķĮ§vØ^–)Þš{.nĮ+‰·ĨŠ{ąû"žØ^n‡rĄöĶzËëh™ĻčÚ& >>>>> Ēø Ū >>>>> GŦéhŪ(階ŠÝĒj"úķm§ĸïęäzđÞ–ŠāþfĢĒ·hšˆ§~ˆm >>>> >>>> -- >>>> Chuck Lever >>>> >>>> >>>> >>> >>> -- >>> Trond Myklebust >>> Linux NFS client maintainer, Hammerspace >>> trond.myklebust@xxxxxxxxxxxxxxx >> >> -- >> Chuck Lever >> >> >> > -- > Trond Myklebust > CTO, Hammerspace Inc > 4300 El Camino Real, Suite 105 > Los Altos, CA 94022 > www.hammer.space -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
