[PATCH] fix undefined behavior in ptrace

[PATCH] fix undefined behavior in ptrace

[Alexander Monakov]

---
 src/linux/ptrace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/linux/ptrace.c b/src/linux/ptrace.c
index 83b8022b..ab7fcda3 100644
--- a/src/linux/ptrace.c
+++ b/src/linux/ptrace.c
@@ -7,14 +7,17 @@ long ptrace(int req, ...)
 {
 	va_list ap;
 	pid_t pid;
-	void *addr, *data, *addr2;
+	void *addr, *data, *addr2 = 0;
 	long ret, result;
 
 	va_start(ap, req);
 	pid = va_arg(ap, pid_t);
 	addr = va_arg(ap, void *);
 	data = va_arg(ap, void *);
+	/* PTRACE_{READ,WRITE}{DATA,TEXT} are specific to SPARC. */
+#ifdef PTRACE_READTEXT
 	addr2 = va_arg(ap, void *);
+#endif
 	va_end(ap);
 
 	if (req-1U < 3) data = &result;
-- 
2.11.0

Re: [PATCH] fix undefined behavior in ptrace

[Rich Felker]

On Wed, Jun 28, 2017 at 04:25:13PM +0300, Alexander Monakov wrote:
> ---
>  src/linux/ptrace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/linux/ptrace.c b/src/linux/ptrace.c
> index 83b8022b..ab7fcda3 100644
> --- a/src/linux/ptrace.c
> +++ b/src/linux/ptrace.c
> @@ -7,14 +7,17 @@ long ptrace(int req, ...)
>  {
>  	va_list ap;
>  	pid_t pid;
> -	void *addr, *data, *addr2;
> +	void *addr, *data, *addr2 = 0;
>  	long ret, result;
>  
>  	va_start(ap, req);
>  	pid = va_arg(ap, pid_t);
>  	addr = va_arg(ap, void *);
>  	data = va_arg(ap, void *);
> +	/* PTRACE_{READ,WRITE}{DATA,TEXT} are specific to SPARC. */
> +#ifdef PTRACE_READTEXT
>  	addr2 = va_arg(ap, void *);
> +#endif

I think there's still UB here, reading more args than were passed.
These calls to va_arg should probably be dependent on the particular
req; I don't see any reason for it to be compile-time dependent on the
presence of one particular req value.

Otherwise yes it's an improvement.

Rich

Re: [PATCH] fix undefined behavior in ptrace

[Alexander Monakov]

On Wed, 28 Jun 2017, Rich Felker wrote:
> I think there's still UB here, reading more args than were passed.
> These calls to va_arg should probably be dependent on the particular
> req; I don't see any reason for it to be compile-time dependent on the
> presence of one particular req value.

I raised that last year but there was no response:
http://www.openwall.com/lists/musl/2016/05/04/18

Alexander

Re: [PATCH] fix undefined behavior in ptrace

[Rich Felker]

On Wed, Jun 28, 2017 at 06:22:12PM +0300, Alexander Monakov wrote:
> On Wed, 28 Jun 2017, Rich Felker wrote:
> > I think there's still UB here, reading more args than were passed.
> > These calls to va_arg should probably be dependent on the particular
> > req; I don't see any reason for it to be compile-time dependent on the
> > presence of one particular req value.
> 
> I raised that last year but there was no response:
> http://www.openwall.com/lists/musl/2016/05/04/18

I'm sorry for overlooking it then. Could you submit a fleshed-out
patch that uses that approach? Assuming the arg types are always the
same, and only the number of args varies by command, a nicer approach
than nested if's with a bunch of equality comparisons each might be a
switch statement to map commands to # of args, then

	if (nargs>=1) pid = va_arg(ap, pid_t);
	if (nargs>=2) addr = va_arg(ap, void *);
	...

Thoughts?

Rich

Re: [PATCH] fix undefined behavior in ptrace

[Alexander Monakov]

---
On Tue, 4 Jul 2017, Rich Felker wrote:
> Thoughts?

I'm not convinced it's a good idea, given that it's a Linux specific interface,
and the manpage rather explicitly discourages passing fewer than four
arguments.

Plus, handling SPARC-specific differences of argument counts for
PTRACE_{GET,SET}{FP,}REGS would be annoying.

What makes sense is to retrieve the fifth argument only when needed:

 src/linux/ptrace.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/linux/ptrace.c b/src/linux/ptrace.c
index 83b8022b..a3f393d9 100644
--- a/src/linux/ptrace.c
+++ b/src/linux/ptrace.c
@@ -7,14 +7,18 @@ long ptrace(int req, ...)
 {
 	va_list ap;
 	pid_t pid;
-	void *addr, *data, *addr2;
+	void *addr, *data, *addr2 = 0;
 	long ret, result;
 
 	va_start(ap, req);
 	pid = va_arg(ap, pid_t);
 	addr = va_arg(ap, void *);
 	data = va_arg(ap, void *);
-	addr2 = va_arg(ap, void *);
+	/* PTRACE_{READ,WRITE}{DATA,TEXT} (16...19) are specific to SPARC. */
+#ifdef PTRACE_READDATA
+	if ((unsigned)req - PTRACE_READDATA < 4)
+		addr2 = va_arg(ap, void *);
+#endif
 	va_end(ap);
 
 	if (req-1U < 3) data = &result;
-- 
2.11.0

Re: [PATCH] fix undefined behavior in ptrace

[Rich Felker]

On Wed, Jul 05, 2017 at 12:51:05AM +0300, Alexander Monakov wrote:
> ---
> On Tue, 4 Jul 2017, Rich Felker wrote:
> > Thoughts?
> 
> I'm not convinced it's a good idea, given that it's a Linux specific interface,
> and the manpage rather explicitly discourages passing fewer than four
> arguments.
> 
> Plus, handling SPARC-specific differences of argument counts for
> PTRACE_{GET,SET}{FP,}REGS would be annoying.
> 
> What makes sense is to retrieve the fifth argument only when needed:

Ah, okay. I'm fine with that. Thanks for clarifying.

>  src/linux/ptrace.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/linux/ptrace.c b/src/linux/ptrace.c
> index 83b8022b..a3f393d9 100644
> --- a/src/linux/ptrace.c
> +++ b/src/linux/ptrace.c
> @@ -7,14 +7,18 @@ long ptrace(int req, ...)
>  {
>  	va_list ap;
>  	pid_t pid;
> -	void *addr, *data, *addr2;
> +	void *addr, *data, *addr2 = 0;
>  	long ret, result;
>  
>  	va_start(ap, req);
>  	pid = va_arg(ap, pid_t);
>  	addr = va_arg(ap, void *);
>  	data = va_arg(ap, void *);
> -	addr2 = va_arg(ap, void *);
> +	/* PTRACE_{READ,WRITE}{DATA,TEXT} (16...19) are specific to SPARC. */
> +#ifdef PTRACE_READDATA
> +	if ((unsigned)req - PTRACE_READDATA < 4)
> +		addr2 = va_arg(ap, void *);
> +#endif
>  	va_end(ap);
>  
>  	if (req-1U < 3) data = &result;
> -- 
> 2.11.0

Applying.

Rich