clnt_dg_call: Change the memory allocation

PATCH Superseded Steve Dickson 2 participants 4 messages Mar 6, 2018

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2018-03-06 18:03 UTC #0 lore raw

PATCH 1/1 clnt_dg_call: Change the memory allocation

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

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

src/clnt_dg.c +3 -3

index 884a2db..1d55b1e 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

Reviewed-by on v1 1/1

2018-03-06 18:19 UTC #1 lore raw

Quoted 8 lines · Steve Dickson

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

Quoted 36 lines

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..1d55b1e 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

Steve Dickson

author

2018-03-06 19:10 UTC #2 lore raw

Quoted 7 lines · Chuck Lever

Quoted 6 lines · Steve Dickson
Change the memory allocation from the stack
to the heap by calling calloc() verses alloca

Quoted 6 lines

Quoted 1 line
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.

Quoted 5 lines

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...

Quoted 2 lines

Reviewed-by Chuck Lever <[email protected]>

Thanks!

steved.

Quoted 44 lines

Quoted 38 lines

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..1d55b1e 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

Chuck Lever

2018-03-06 19:22 UTC #3 lore raw

Quoted 19 lines · Steve Dickson

=20
=20
=20
Quoted 12 lines · Chuck Lever
=20
=20
Quoted 4 lines · Steve Dickson
=20
Change the memory allocation from the stack
to the heap by calling calloc() verses alloca
=20
Quoted 1 line
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=3D1552163
=20
IMO, this should be:
=20
Fixes 2936f109590e ("clnt_dg_call: Fix a buffer overflow =

(CVE-2016-4429)")

Quoted 8 lines

Quoted 1 line
BugLink https://bugzilla.redhat.com/show_bug.cgi?id=3D1552163
point.
=20
Quoted 5 lines
=20
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.=20

Quoted 8 lines

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=20 allocating from the heap is probably better than=20 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 ;-)

Quoted 1 line

I'll try to be more more descriptive in the description.

Cool, thanks.

Quoted 12 lines

Why not just remove them, for instance?
I assumed outlen can be variable size, but did not=20 look very hard. I'm just trying to clean up some=20 old bz so the less change I do the better... IMHO...=20
Quoted 1 line
Reviewed-by Chuck Lever <[email protected]>
Thanks!
=20
steved.
=20
Quoted 2 lines
=20
=20

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
Signed-off-by Steve Dickson <[email protected]>
---
src/clnt_dg.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 8 lines
src/clnt_dg.c get_reply: · line 431
Quoted 8 lines
=20
src/clnt_dg.c +0 -0
index 884a2db..1d55b1e 100644
@@ -430,7 +430,7 @@ get_reply:
 	  struct sockaddr_in err_addr;
 	  struct sockaddr_in *sin =3D (struct sockaddr_in =

*)&cu->cu_raddr;

Quoted 9 lines

src/clnt_dg.c get_reply: · line 463

Quoted 9 lines

src/clnt_dg.c +1 -1

 	  struct iovec iov;
-	  char *cbuf =3D (char *) alloca (outlen + 256);
+	  char *cbuf =3D (char *) mem_alloc(outlen + 256);
 	  int ret;

=20

	  if (cbuf =3D=3D NULL)=20

src/clnt_dg.c +0 -0

@@ -462,13 +462,13 @@ get_reply:
 		 cmsg =3D CMSG_NXTHDR (&msg, cmsg))
 	      if (cmsg->cmsg_level =3D=3D SOL_IP && cmsg->cmsg_type =3D=3D=

IP_RECVERR)

Quoted 13 lines

src/clnt_dg.c +2 -2

 		{
-		  free(cbuf);
+		  mem_free(cbuf, (outlen + 256));
 		  e =3D (struct sock_extended_err *) CMSG_DATA(cmsg);
 		  cu->cu_error.re_errno =3D e->ee_errno;
 		  release_fd_lock(cu->cu_fd, mask);
 		  return (cu->cu_error.re_status =3D RPC_CANTRECV);
 		}
-	  free(cbuf);
+	  mem_free(cbuf, (outlen + 256));
 	}
 #endif

=20

Quoted 8 lines

Quoted 8 lines
Quoted 2 lines
--=20
2.14.3
=20
--
Chuck Lever
=20
=20
=20

--
Chuck Lever