[PATCH] clnt_dg_call: Change the memory allocation

[PATCH] clnt_dg_call: Change the memory allocation

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

Re: [PATCH] clnt_dg_call: Change the memory allocation

[Chuck Lever]

> 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

Re: [PATCH] clnt_dg_call: Change the memory allocation

[Steve Dickson]

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

Re: [PATCH] clnt_dg_call: Change the memory allocation

[Chuck Lever]

> On Mar 6, 2018, at 2:10 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>=20
>=20
>=20
> On 03/06/2018 01:19 PM, Chuck Lever wrote:
>>=20
>>=20
>>> On Mar 6, 2018, at 1:03 PM, Steve Dickson <steved@redhat.com> wrote:
>>>=20
>>> Change the memory allocation from the stack
>>> to the heap by calling calloc() verses alloca
>>=20
>>> 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)")
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=3D1552163
> point.
>=20
>>=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
> 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 ;-)


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

>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> Thanks!
>=20
> steved.
>=20
>>=20
>>=20
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>> src/clnt_dg.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>=20
>>> 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 =3D (struct sockaddr_in =
*)&cu->cu_raddr;
>>> 	  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
>>> @@ -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)
>>> 		{
>>> -		  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
>>> --=20
>>> 2.14.3
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20

--
Chuck Lever