Skip to contentSkip to conversation

rpcbind: fix attempt to free non-dynamic memory

PATCH New Ed Blake 2 participants 4 messages Jan 17, 2018 – Jan 18, 2018

lore
Share & tools
lore

Ed Blake

author
#0 lore raw

PATCH 1/1 rpcbind: fix attempt to free non-dynamic memory

Commit 954509f added a security fix for CVE-2017-8779, involving pairing all svc_getargs() calls with svc_freeargs() to avoid a memory leak. This included adding a call to svc_freeargs() to rpcbproc_callit_com().

However, rpcbproc_callit_com() allocates memory for args.rmt_args.args itself, either dynamically (sendsz > RPC_BUF_MAX) or else on the stack, rather than having the memory allocated in svc_getargs().

The call to svc_freeargs() results in an attempt to free the memory allocated by rpcbproc_callit_com(), which if on the stack results in undefined behaviour.

Fix this by removing the svc_freeargs() call, which is not required as rpcbproc_callit_com() allocates (and correctly frees) memory itself.

Change-Id I7fc34efd58408ec5e626da8edd08aa697ed8b936
Signed-off-by Ed Blake <[email protected]>
---
 ...pair-all-svc_getargs-calls-with-svc_freeargs.patch | 19 -------------------
 1 file changed, 19 deletions(-)
package/rpcbind/0004-rpcbind-pair-all-svc_getargs-calls-with-svc_freeargs.patch +0 -19
index 130e5d77c..91fe20830 100644
@@ -207,25 +207,6 @@ index b673452..3e37b54 100644
  	}
  
  	if (rqstp->rq_proc == RPCBPROC_SET
-diff --git a/src/rpcb_svc_com.c b/src/rpcb_svc_com.c
-index ff9ce6b..98ede61 100644
---- a/src/rpcb_svc_com.c
-+++ b/src/rpcb_svc_com.c
-@@ -931,6 +931,14 @@ error:
- 	if (call_msg.rm_xid != 0)
- 		(void) free_slot_by_xid(call_msg.rm_xid);
- out:
-+	if (!svc_freeargs(transp, (xdrproc_t) xdr_rmtcall_args, (char *) &a)) {
-+		if (debugging) {
-+			(void) xlog(LOG_DEBUG, "unable to free arguments\n");
-+			if (doabort) {
-+				rpcbind_abort();
-+			}
-+		}
-+	}
- 	if (local_uaddr)
- 		free(local_uaddr);
- 	if (buf_alloc)
 -- 
 2.11.0
Signature · 1 line
2.11.0
#1 lore raw

PATCH 1/1 rpcbind: fix attempt to free non-dynamic memory

Hello,

Quoted 13 lines · Ed Blake

Commit 954509f added a security fix for CVE-2017-8779, involving pairing all svc_getargs() calls with svc_freeargs() to avoid a memory leak. This included adding a call to svc_freeargs() to rpcbproc_callit_com().

However, rpcbproc_callit_com() allocates memory for args.rmt_args.args itself, either dynamically (sendsz > RPC_BUF_MAX) or else on the stack, rather than having the memory allocated in svc_getargs().

The call to svc_freeargs() results in an attempt to free the memory allocated by rpcbproc_callit_com(), which if on the stack results in undefined behaviour.

Quoted 5 lines

Fix this by removing the svc_freeargs() call, which is not required as rpcbproc_callit_com() allocates (and correctly frees) memory itself.

Change-Id I7fc34efd58408ec5e626da8edd08aa697ed8b936
Signed-off-by Ed Blake <[email protected]>

Thanks. Is this fix-for-the-fix in the upstream rpcbind project ? If not, did you submit it ?

I think we'd prefer to keep the existing 0004-rpcbind-pair-all-svc_getargs-calls-with-svc_freeargs.patch unchanged, so that it matches the upstream commit, and add an additional patch that fixes the commit. Just to be inline with what upstream has.

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#2 lore raw

PATCH 1/1 rpcbind: fix attempt to free non-dynamic memory

Quoted 16 lines · Thomas Petazzoni

Hello,

Quoted 13 lines · Ed Blake

Commit 954509f added a security fix for CVE-2017-8779, involving pairing all svc_getargs() calls with svc_freeargs() to avoid a memory leak. This included adding a call to svc_freeargs() to rpcbproc_callit_com().

However, rpcbproc_callit_com() allocates memory for args.rmt_args.args itself, either dynamically (sendsz > RPC_BUF_MAX) or else on the stack, rather than having the memory allocated in svc_getargs().

The call to svc_freeargs() results in an attempt to free the memory allocated by rpcbproc_callit_com(), which if on the stack results in undefined behaviour.

Quoted 7 lines
Quoted 5 lines

Fix this by removing the svc_freeargs() call, which is not required as rpcbproc_callit_com() allocates (and correctly frees) memory itself.

Change-Id I7fc34efd58408ec5e626da8edd08aa697ed8b936
Signed-off-by Ed Blake <[email protected]>

Thanks. Is this fix-for-the-fix in the upstream rpcbind project ? If not, did you submit it ?

So it looks like this was already fixed here:

http://git.linux-nfs.org/?p=steved/rpcbind.git;a=commit;h=7c7590ad536c0e24bef790cb1e65702fc54db566

So I should just submit that as a patch file to buildroot?

I guess we should also apply this at the same time:

http://git.linux-nfs.org/?p=steved/rpcbind.git;a=commit;h=c49a7ea639eb700823e174fd605bbbe183e229aa

Quoted 9 lines

I think we'd prefer to keep the existing 0004-rpcbind-pair-all-svc_getargs-calls-with-svc_freeargs.patch unchanged, so that it matches the upstream commit, and add an additional patch that fixes the commit. Just to be inline with what upstream has.

Best regards,

Thomas

Signature · 1 line
Ed
#3 lore raw

PATCH 1/1 rpcbind: fix attempt to free non-dynamic memory

Hello,

Quoted 10 lines · Ed Blake
Quoted 4 lines
Quoted 2 lines
Change-Id I7fc34efd58408ec5e626da8edd08aa697ed8b936
Signed-off-by Ed Blake <[email protected]>

Thanks. Is this fix-for-the-fix in the upstream rpcbind project ? If not, did you submit it ?

So it looks like this was already fixed here:

http://git.linux-nfs.org/?p=steved/rpcbind.git;a=commit;h=7c7590ad536c0e24bef790cb1e65702fc54db566

So I should just submit that as a patch file to buildroot?

Yes. git format-patch this commit, adds your Signed-off-by, and put the patch in package/rpcbind/.

Quoted 3 lines

I guess we should also apply this at the same time:

http://git.linux-nfs.org/?p=steved/rpcbind.git;a=commit;h=c49a7ea639eb700823e174fd605bbbe183e229aa

Indeed.

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com