| Message ID | 20180306180323.12131-1-steved@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Mar 6, 2018, at 1:03 PM, Steve Dickson <steved@redhat.com> 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 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. Why not just remove them, for instance? Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Steve Dickson <steved@redhat.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/2018 01:19 PM, Chuck Lever wrote: > > >> On Mar 6, 2018, at 1:03 PM, Steve Dickson <steved@redhat.com> 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. I'll try to be more more descriptive in the description. 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@oracle.com> Thanks! steved. > > >> Signed-off-by: Steve Dickson <steved@redhat.com> >> --- >> 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 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 6, 2018, at 2:10 PM, Steve Dickson <SteveD@RedHat.com> wrote: > > > > On 03/06/2018 01:19 PM, Chuck Lever wrote: >> >> >>> On Mar 6, 2018, at 1:03 PM, Steve Dickson <steved@redhat.com> 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@oracle.com> > Thanks! > > steved. > >> >> >>> Signed-off-by: Steve Dickson <steved@redhat.com> >>> --- >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 Signed-off-by: Steve Dickson <steved@redhat.com> --- src/clnt_dg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)