* [PATCH] clnt_dg_call: Change the memory allocation @ 2018-03-06 18:03 Steve Dickson 2018-03-06 18:19 ` Chuck Lever 0 siblings, 1 reply; 4+ messages in thread From: Steve Dickson @ 2018-03-06 18:03 UTC (permalink / raw) To: Libtirpc-devel Mailing List; +Cc: Linux NFS Mailing list 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] clnt_dg_call: Change the memory allocation 2018-03-06 18:03 [PATCH] clnt_dg_call: Change the memory allocation Steve Dickson @ 2018-03-06 18:19 ` Chuck Lever 2018-03-06 19:10 ` Steve Dickson 0 siblings, 1 reply; 4+ messages in thread From: Chuck Lever @ 2018-03-06 18:19 UTC (permalink / raw) To: Steve Dickson; +Cc: libtirpc List, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clnt_dg_call: Change the memory allocation 2018-03-06 18:19 ` Chuck Lever @ 2018-03-06 19:10 ` Steve Dickson 2018-03-06 19:21 ` Chuck Lever 0 siblings, 1 reply; 4+ messages in thread From: Steve Dickson @ 2018-03-06 19:10 UTC (permalink / raw) To: Chuck Lever; +Cc: libtirpc List, Linux NFS Mailing List 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] clnt_dg_call: Change the memory allocation 2018-03-06 19:10 ` Steve Dickson @ 2018-03-06 19:21 ` Chuck Lever 0 siblings, 0 replies; 4+ messages in thread From: Chuck Lever @ 2018-03-06 19:21 UTC (permalink / raw) To: Steve Dickson; +Cc: libtirpc List, Linux NFS Mailing List > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-06 19:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-06 18:03 [PATCH] clnt_dg_call: Change the memory allocation Steve Dickson 2018-03-06 18:19 ` Chuck Lever 2018-03-06 19:10 ` Steve Dickson 2018-03-06 19:21 ` Chuck Lever
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox