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(-)
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
> On Mar 6, 2018, at 1:03 PM, Steve Dickson <[email protected]> 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 <[email protected]>
> Signed-off-by: Steve Dickson <[email protected]>
> ---
> 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
On 03/06/2018 01:19 PM, Chuck Lever wrote:
>
>
>> On Mar 6, 2018, at 1:03 PM, Steve Dickson <[email protected]> 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 <[email protected]>
Thanks!
steved.
>
>
>> Signed-off-by: Steve Dickson <[email protected]>
>> ---
>> 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
>
>
>
> On Mar 6, 2018, at 2:10 PM, Steve Dickson <[email protected]> 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 <[email protected]> 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 <[email protected]>
> Thanks!
>=20
> steved.
>=20
>>=20
>>=20
>>> Signed-off-by: Steve Dickson <[email protected]>
>>> ---
>>> 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