> On Jun 29, 2018, at 3:48 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > On Fri, 2018-06-29 at 15:33 -0400, Chuck Lever wrote: >>> On Jun 29, 2018, at 3:29 PM, Trond Myklebust <trondmy@hammerspace.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. >>> 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); >>> >>>> +#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 -- 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
