clnt_dg_call: Change the memory allocation

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 <[email protected]>

---
src/clnt_dg.c | 6 +++---

1 file changed, 3 insertions(+), 3 deletions(-)

src/clnt_dg.c +3 -3

index 884a2db..04a2aba 100644
@@ -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
 

Signature · 1 line

2.14.3

Hi Steve-

Quoted 6 lines · Steve Dickson

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.

Quoted 5 lines

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.

Quoted 6 lines

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 <[email protected]>

Reviewed-by Chuck Lever <[email protected]>

Quoted 37 lines

---
src/clnt_dg.c | 6 +++---

1 file changed, 3 insertions(+), 3 deletions(-)

src/clnt_dg.c +3 -3

index 884a2db..04a2aba 100644
@@ -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
 

Signature · 1 line

2.14.3

--
Chuck Lever

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 <[email protected]>

---
 src/clnt_dg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

src/clnt_dg.c +3 -3

index 884a2db..04a2aba 100644
@@ -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