| Message ID | 20180307140903.13278-1-steved@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Steve- > On Mar 7, 2018, at 9:09 AM, Steve Dickson <SteveD@redhat.com> wrote: > > Commit 2936f109590e add free()s on memory that > was allocated from the stack (via alloca()). > That type memory is automatically freed so > those added free()s was causing a double frees. I'm going to split hairs, but that doesn't mean I object to the patch. You know that's the way I roll. My copy of "man alloca(3)" says: "Do not attempt to free(3) space allocated by alloca()!" Since memory returned by alloca(3) is not in the heap, it's an attempt to free memory that was not allocated via malloc(3), which is technically not a double-free. > It was suggested allocating memory from the > stack can be a bit troublesome. So this patch > changes the memory allocation from the stack > to the heap which also eliminates the double frees. My reading of CVE-2016-4429 is that the alloca(3) was the real problem. There is a "goto call_again;" later down in this function that can force this piece of code to be called in a loop, resulting in more than one call to alloca(3) in this function. Stack exhaustion occurs if the loop goto is taken too many times. Using a heap allocation and freeing that memory within the loop is a good fix for this issue, and that's what we end up with after this patch is applied. It would be helpful to replace this second paragraph with a note that states this patch is needed to finally close CVE-2016-4429. Happily, this appears to be the only remaining alloca(3) call site in libtirpc. It would make this code a little easier to understand if the numeric literals (256) were replaced by a macro or enum. That's a nit, but then it becomes more clear that the size of the buffer is made available to recvmsg(2) so that, IIUC, the buffer is never overrun by the incoming message. > Fixes: 2936f109590e ("clnt_dg_call: Fix a buffer overflow (CVE-2016-4429)") > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1552163 > > Signed-off-by: Steve Dickson <steved@redhat.com> Reviewed-by: Chuck Lever <chuck.lever@oracle.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..04a2aba 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
Hey! On 03/07/2018 10:43 AM, Chuck Lever wrote: > Hi Steve- > >> On Mar 7, 2018, at 9:09 AM, Steve Dickson <SteveD@redhat.com> wrote: >> >> Commit 2936f109590e add free()s on memory that >> was allocated from the stack (via alloca()). >> That type memory is automatically freed so >> those added free()s was causing a double frees. > > I'm going to split hairs, but that doesn't mean I > object to the patch. You know that's the way I roll. I know all to well... ;-) > > My copy of "man alloca(3)" says: > > "Do not attempt to free(3) space allocated by alloca()!" > > Since memory returned by alloca(3) is not in the heap, > it's an attempt to free memory that was not allocated > via malloc(3), which is technically not a double-free. > This was definitely brain-fart on my part... I wish I had read that before doing the commit... I was not familiar with the workings of alloca(3) I just assume alloca(3) was like the rest of the [m|c]alloc family but no... it had to be a twisted sister! 8-) > >> It was suggested allocating memory from the >> stack can be a bit troublesome. So this patch >> changes the memory allocation from the stack >> to the heap which also eliminates the double frees. > > My reading of CVE-2016-4429 is that the alloca(3) was the > real problem. There is a "goto call_again;" later down in > this function that can force this piece of code to be > called in a loop, resulting in more than one call to > alloca(3) in this function. Stack exhaustion occurs if the > loop goto is taken too many times. > > Using a heap allocation and freeing that memory within the > loop is a good fix for this issue, and that's what we end > up with after this patch is applied. It would be helpful to > replace this second paragraph with a note that states this > patch is needed to finally close CVE-2016-4429. I see this as the fix for the fix of the CVE. > > Happily, this appears to be the only remaining alloca(3) > call site in libtirpc. Bingo! > > It would make this code a little easier to understand if > the numeric literals (256) were replaced by a macro or enum. > That's a nit, but then it becomes more clear that the size > of the buffer is made available to recvmsg(2) so that, IIUC, > the buffer is never overrun by the incoming message. Let remember this is UDP... which is no longer on by default in the upstream server and an unnamed source as told me that UDP will not be supported in upcoming distro release. So I'm hoping to just fix the CVE and move on... > > >> Fixes: 2936f109590e ("clnt_dg_call: Fix a buffer overflow (CVE-2016-4429)") >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1552163 >> >> Signed-off-by: Steve Dickson <steved@redhat.com> > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com> Added... and Thank you!! steved. > > >> --- >> 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..04a2aba 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/07/2018 09:09 AM, Steve Dickson wrote: > Commit 2936f109590e add free()s on memory that > was allocated from the stack (via alloca()). > That type memory is automatically freed so > those added free()s was causing a double frees. > > It was suggested allocating memory from the > stack can be a bit troublesome. So this patch > changes the memory allocation from the stack > to the heap which also eliminates the double frees. > > Fixes: 2936f109590e ("clnt_dg_call: Fix a buffer overflow (CVE-2016-4429)") > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1552163 > > Signed-off-by: Steve Dickson <steved@redhat.com> Committed... steved. > --- > 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..04a2aba 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 > > -- 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..04a2aba 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
Commit 2936f109590e add free()s on memory that was allocated from the stack (via alloca()). That type memory is automatically freed so those added free()s was causing a double frees. It was suggested allocating memory from the stack can be a bit troublesome. So this patch changes the memory allocation from the stack to the heap which also eliminates the double frees. Fixes: 2936f109590e ("clnt_dg_call: Fix a buffer overflow (CVE-2016-4429)") BugLink: 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(-)