> On Mar 6, 2018, at 2:10 PM, Steve Dickson <SteveD@xxxxxxxxxx> wrote: > > > > On 03/06/2018 01:19 PM, Chuck Lever wrote: >> >> >>> On Mar 6, 2018, at 1:03 PM, Steve Dickson <steved@xxxxxxxxxx> wrote: >>> >>> Change the memory allocation from the stack >>> to the heap by calling calloc() verses alloca >> >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1552163 >> >> IMO, this should be: >> >> Fixes: 2936f109590e ("clnt_dg_call: Fix a buffer overflow (CVE-2016-4429)") >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1552163 > point. > >> >> Can the patch description explain the problem and why >> this is the correct fix? It looks like 2936f109590e >> added some free(3) call sites that were perhaps >> unneeded. > Yeah alloca() allocates from the stack and the memory > is automatically freed when routine returns > (something that was pointed out to me by IRC people > when their UDP mounts broke ;-) So this CVE is basically bogus! > But it was also point out (by the IRC people) that > allocating from the heap is probably better than > scribbling on stack and I agree. Yeah, I prefer to avoid alloca too. As a generic comment, any new code needs to be careful about bounds checking when writing into cbuf, even if cbuf is on the heap instead of the stack. I didn't look carefully at that, but sounds like I will get a second shot to review ;-) > I'll try to be more more descriptive in the description. Cool, thanks. > Why not just remove them, for instance? > I assumed outlen can be variable size, but did not > look very hard. I'm just trying to clean up some > old bz so the less change I do the better... IMHO... >> Reviewed-by: Chuck Lever <chuck.lever@xxxxxxxxxx> > Thanks! > > steved. > >> >> >>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx> >>> --- >>> src/clnt_dg.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/clnt_dg.c b/src/clnt_dg.c >>> index 884a2db..1d55b1e 100644 >>> --- a/src/clnt_dg.c >>> +++ b/src/clnt_dg.c >>> @@ -430,7 +430,7 @@ get_reply: >>> struct sockaddr_in err_addr; >>> struct sockaddr_in *sin = (struct sockaddr_in *)&cu->cu_raddr; >>> struct iovec iov; >>> - char *cbuf = (char *) alloca (outlen + 256); >>> + char *cbuf = (char *) mem_alloc(outlen + 256); >>> int ret; >>> >>> if (cbuf == NULL) >>> @@ -462,13 +462,13 @@ get_reply: >>> cmsg = CMSG_NXTHDR (&msg, cmsg)) >>> if (cmsg->cmsg_level == SOL_IP && cmsg->cmsg_type == IP_RECVERR) >>> { >>> - free(cbuf); >>> + mem_free(cbuf, (outlen + 256)); >>> e = (struct sock_extended_err *) CMSG_DATA(cmsg); >>> cu->cu_error.re_errno = e->ee_errno; >>> release_fd_lock(cu->cu_fd, mask); >>> return (cu->cu_error.re_status = RPC_CANTRECV); >>> } >>> - free(cbuf); >>> + mem_free(cbuf, (outlen + 256)); >>> } >>> #endif >>> >>> -- >>> 2.14.3 >> >> -- >> Chuck Lever >> >> >> -- 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
