Re: ptrace PT_IO write bug

[Mathieu -]

Mark Kettenis wrote:
> > > The documentation may have been wrong for some time on some archs, it
> > > feels like making PT_WRITE_D and PT_WRITE_I equivalent was deemed
> > > useful at one point.  Given that Free and NetBSD document the same
> > > guarantee, I personally don't feel comfortable changing that, but YMMV.
> > 
> > This is a trap designed to make code work on amd64 and fail on hppa.
> > But if kettenis cares more about hppa than most people, maybe we should let
> > him be the one to decide. :)
> 
> I'm not really worried about this; ptrace(2) is only used by gdb and
> people writing exploits ;).

Hi guys,

I see Ted did commit an update to the docs, the root issue is still
present though and is adressed in the latest version of my patch, what's
the status on that one?

Re: ptrace PT_IO write bug

[Mark Kettenis]

> From: "Ted Unangst" 
> Date: Tue, 31 May 2016 13:57:04 -0400
> 
> Jeremie Courreges-Anglas wrote:
> > >> 
> > >> Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
> > >> since rev. 1.1, I doubt that such an optimization is a good idea.
> > >
> > > A clear case where the documentation is wrong.
> > 
> > 
> > The documentation may have been wrong for some time on some archs, it
> > feels like making PT_WRITE_D and PT_WRITE_I equivalent was deemed
> > useful at one point.  Given that Free and NetBSD document the same
> > guarantee, I personally don't feel comfortable changing that, but YMMV.
> 
> This is a trap designed to make code work on amd64 and fail on hppa.
> But if kettenis cares more about hppa than most people, maybe we should let
> him be the one to decide. :)

I'm not really worried about this; ptrace(2) is only used by gdb and
people writing exploits ;).

Re: ptrace PT_IO write bug

[Ted Unangst]

Jeremie Courreges-Anglas wrote:
> >> 
> >> Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
> >> since rev. 1.1, I doubt that such an optimization is a good idea.
> >
> > A clear case where the documentation is wrong.
> 
> 
> The documentation may have been wrong for some time on some archs, it
> feels like making PT_WRITE_D and PT_WRITE_I equivalent was deemed
> useful at one point.  Given that Free and NetBSD document the same
> guarantee, I personally don't feel comfortable changing that, but YMMV.

This is a trap designed to make code work on amd64 and fail on hppa.
But if kettenis cares more about hppa than most people, maybe we should let
him be the one to decide. :)

Re: ptrace PT_IO write bug

[Jeremie Courreges-Anglas]

Mark Kettenis  writes:

>> From: Jeremie Courreges-Anglas 
>> Date: Mon, 30 May 2016 19:30:27 +0200
>> 
>> Mark Kettenis  writes:
>> 
>> > Mathieu - schreef op 2016-05-28 13:05:
>> >> Martin Natano wrote:
>> >>> The diff reads fine to me, however it is incomplete. There are some
>> >>> callers of process_domem() in arch/. They will need to be changed too.
>> >>> req seems to be in sync with uio_rw in all the cases, so just removing
>> >>> the last argument should do it.
>> >>>
>> >>
>> >> Thanks for the feedback. The missing callers where an overlook on my
>> >> part, sorry for that.
>> >>
>> >> Here is a regenerated diff including all the call site. As a side note,
>> >> obviously every one of them was using PT_WRITE_I, that's why it went
>> >> unnoticed.
>> >
>> > The thing you guys are missing is that on some architectures making
>> > changes to instructions (PT_WRITE_I) requires some additional operations
>> > to guarantee that the CPU actually sees those updated instructions.
>> > Typically this is the case on architectures with separate data and
>> > instruction caches, where the instruction cache doesn't snoop the data
>> > cache.  On such architectures (powerpc and arm are examples) you need to
>> > flush the data cache and invalidate the instruction cache.  That may be
>> > a somewhat expensive operation.
>> > As you probably guessed, pmap_proc_iflush() is the function that takes
>> > care of this.  Since you still call pmap_proc_iflush(), the diff isn't
>> > wrong from a correctness point of view, but I think we should keep the
>> > optimization of not calling pmap_proc_iflush() for PT_WRITE_D.
>> 
>> Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
>> since rev. 1.1, I doubt that such an optimization is a good idea.
>
> A clear case where the documentation is wrong.

It is wrong since the change that happened in rev. 1.33; before,
procfs_domem() used:

error = uvm_io(>p_vmspace->vm_map, uio,
uio->uio_rw == UIO_WRITE ? UVM_IO_FIXPROT : 0);
uvmspace_free(p->p_vmspace);

if (error == 0 && uio->uio_rw == UIO_WRITE)
pmap_proc_iflush(p, addr, len);

Looks like you are the one who added the pmap_proc_iflush call, btw.

The documentation may have been wrong for some time on some archs, it
feels like making PT_WRITE_D and PT_WRITE_I equivalent was deemed
useful at one point.  Given that Free and NetBSD document the same
guarantee, I personally don't feel comfortable changing that, but YMMV.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ptrace PT_IO write bug

[Mark Kettenis]

> From: Jeremie Courreges-Anglas 
> Date: Mon, 30 May 2016 19:30:27 +0200
> 
> Mark Kettenis  writes:
> 
> > Mathieu - schreef op 2016-05-28 13:05:
> >> Martin Natano wrote:
> >>> The diff reads fine to me, however it is incomplete. There are some
> >>> callers of process_domem() in arch/. They will need to be changed too.
> >>> req seems to be in sync with uio_rw in all the cases, so just removing
> >>> the last argument should do it.
> >>>
> >>
> >> Thanks for the feedback. The missing callers where an overlook on my
> >> part, sorry for that.
> >>
> >> Here is a regenerated diff including all the call site. As a side note,
> >> obviously every one of them was using PT_WRITE_I, that's why it went
> >> unnoticed.
> >
> > The thing you guys are missing is that on some architectures making
> > changes to instructions (PT_WRITE_I) requires some additional operations
> > to guarantee that the CPU actually sees those updated instructions.
> > Typically this is the case on architectures with separate data and
> > instruction caches, where the instruction cache doesn't snoop the data
> > cache.  On such architectures (powerpc and arm are examples) you need to
> > flush the data cache and invalidate the instruction cache.  That may be
> > a somewhat expensive operation.
> > As you probably guessed, pmap_proc_iflush() is the function that takes
> > care of this.  Since you still call pmap_proc_iflush(), the diff isn't
> > wrong from a correctness point of view, but I think we should keep the
> > optimization of not calling pmap_proc_iflush() for PT_WRITE_D.
> 
> Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
> since rev. 1.1, I doubt that such an optimization is a good idea.

A clear case where the documentation is wrong.

Re: ptrace PT_IO write bug

[Jeremie Courreges-Anglas]

Mark Kettenis  writes:

> Mathieu - schreef op 2016-05-28 13:05:
>> Martin Natano wrote:
>>> The diff reads fine to me, however it is incomplete. There are some
>>> callers of process_domem() in arch/. They will need to be changed too.
>>> req seems to be in sync with uio_rw in all the cases, so just removing
>>> the last argument should do it.
>>>
>>
>> Thanks for the feedback. The missing callers where an overlook on my
>> part, sorry for that.
>>
>> Here is a regenerated diff including all the call site. As a side note,
>> obviously every one of them was using PT_WRITE_I, that's why it went
>> unnoticed.
>
> The thing you guys are missing is that on some architectures making
> changes to instructions (PT_WRITE_I) requires some additional operations
> to guarantee that the CPU actually sees those updated instructions.
> Typically this is the case on architectures with separate data and
> instruction caches, where the instruction cache doesn't snoop the data
> cache.  On such architectures (powerpc and arm are examples) you need to
> flush the data cache and invalidate the instruction cache.  That may be
> a somewhat expensive operation.
> As you probably guessed, pmap_proc_iflush() is the function that takes
> care of this.  Since you still call pmap_proc_iflush(), the diff isn't
> wrong from a correctness point of view, but I think we should keep the
> optimization of not calling pmap_proc_iflush() for PT_WRITE_D.

Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
since rev. 1.1, I doubt that such an optimization is a good idea.

> As for the original issue.  Adding UVM_IO_FIXPROT for PT_WRITE_D as
> well, means that it will now be able to make changes to read-only data.
> That is probably corrrect.
>
> So I think the only thing that should be changed is the following bit:
>
>> @@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p,
>> struct uio *uio, int req)
>>  vm->vm_refcnt++;
>>
>>  error = uvm_io(>vm_map, uio,
>> -(req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
>> +(uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
>>
>
> Is that indeed enough to fix the original problem?

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ptrace PT_IO write bug

[Mathieu -]

Mark Kettenis wrote:
> The thing you guys are missing is that on some architectures making changes
> to instructions (PT_WRITE_I) requires some additional operations to
> guarantee that the CPU actually sees those updated instructions.  Typically
> this is the case on architectures with separate data and instruction caches,
> where the instruction cache doesn't snoop the data cache.  On such
> architectures (powerpc and arm are examples) you need to flush the data
> cache and invalidate the instruction cache.  That may be a somewhat
> expensive operation.
> As you probably guessed, pmap_proc_iflush() is the function that takes care
> of this.  Since you still call pmap_proc_iflush(), the diff isn't wrong from
> a correctness point of view, but I think we should keep the optimization of
> not calling pmap_proc_iflush() for PT_WRITE_D.

Well his makes sense. I pondered while making this change whether
or not I should change the second condition (the iflush one), obviously
I shouldn't have. Heh, I pay my lack of knowledge on !x86 arch.

Thanks for the input!

> As for the original issue.  Adding UVM_IO_FIXPROT for PT_WRITE_D as well,
> means that it will now be able to make changes to read-only data.  That is
> probably corrrect.
> 
> So I think the only thing that should be changed is the following bit:
> 
> >@@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p,
> >struct uio *uio, int req)
> > vm->vm_refcnt++;
> >
> > error = uvm_io(>vm_map, uio,
> >-(req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
> >+(uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
> >
> 
> Is that indeed enough to fix the original problem?

Yes this is indeed enough, and I just confirmed it with my test program.
So here is (hopefully) the latest version of this diff, which is now way
shorter.

diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 60ec50e..09e56b9 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -734,7 +734,7 @@ process_domem(struct proc *curp, struct proc *p, struct uio 
*uio, int req)
vm->vm_refcnt++;
 
error = uvm_io(>vm_map, uio,
-   (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
+   (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
 
uvmspace_free(vm);
 

Re: ptrace PT_IO write bug

[Mark Kettenis]

Mathieu - schreef op 2016-05-28 13:05:

Martin Natano wrote:

The diff reads fine to me, however it is incomplete. There are some
callers of process_domem() in arch/. They will need to be changed too.
req seems to be in sync with uio_rw in all the cases, so just removing
the last argument should do it.



Thanks for the feedback. The missing callers where an overlook on my
part, sorry for that.

Here is a regenerated diff including all the call site. As a side note,
obviously every one of them was using PT_WRITE_I, that's why it went
unnoticed.


The thing you guys are missing is that on some architectures making 
changes to instructions (PT_WRITE_I) requires some additional operations 
to guarantee that the CPU actually sees those updated instructions.  
Typically this is the case on architectures with separate data and 
instruction caches, where the instruction cache doesn't snoop the data 
cache.  On such architectures (powerpc and arm are examples) you need to 
flush the data cache and invalidate the instruction cache.  That may be 
a somewhat expensive operation.
As you probably guessed, pmap_proc_iflush() is the function that takes 
care of this.  Since you still call pmap_proc_iflush(), the diff isn't 
wrong from a correctness point of view, but I think we should keep the 
optimization of not calling pmap_proc_iflush() for PT_WRITE_D.


As for the original issue.  Adding UVM_IO_FIXPROT for PT_WRITE_D as 
well, means that it will now be able to make changes to read-only data.  
That is probably corrrect.


So I think the only thing that should be changed is the following bit:


@@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p,
struct uio *uio, int req)
vm->vm_refcnt++;

error = uvm_io(>vm_map, uio,
-   (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
+   (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);



Is that indeed enough to fix the original problem?

Re: ptrace PT_IO write bug

[Jeremie Courreges-Anglas]

Mathieu -  writes:

> Martin Natano wrote:
>> The diff reads fine to me, however it is incomplete. There are some
>> callers of process_domem() in arch/. They will need to be changed too.
>> req seems to be in sync with uio_rw in all the cases, so just removing
>> the last argument should do it.
>> 

Thanks, well spotted Martin.

> Thanks for the feedback. The missing callers where an overlook on my
> part, sorry for that.
>
> Here is a regenerated diff including all the call site. As a side note,
> obviously every one of them was using PT_WRITE_I, that's why it went
> unnoticed.

Looks even more correct. :)

ok / objections?

> Mathieu-
>
>
> diff --git a/sys/arch/alpha/alpha/process_machdep.c 
> b/sys/arch/alpha/alpha/process_machdep.c
> index 6fe711e..291e06e 100644
> --- a/sys/arch/alpha/alpha/process_machdep.c
> +++ b/sys/arch/alpha/alpha/process_machdep.c
> @@ -181,7 +181,7 @@ ptrace_read_int(struct proc *p, vaddr_t addr, u_int32_t 
> *v)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_READ;
>   uio.uio_procp = p;
> - return process_domem(curproc, p, , PT_READ_I);
> + return process_domem(curproc, p, );
>  }
>  
>  int
> @@ -199,7 +199,7 @@ ptrace_write_int(struct proc *p, vaddr_t addr, u_int32_t 
> v)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_WRITE;
>   uio.uio_procp = p;
> - return process_domem(curproc, p, , PT_WRITE_I);
> + return process_domem(curproc, p, );
>  }
>  
>  u_int64_t
> diff --git a/sys/arch/hppa/hppa/trap.c b/sys/arch/hppa/hppa/trap.c
> index e86e636..805a924 100644
> --- a/sys/arch/hppa/hppa/trap.c
> +++ b/sys/arch/hppa/hppa/trap.c
> @@ -690,7 +690,7 @@ ss_get_value(struct proc *p, vaddr_t addr, u_int *value)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_READ;
>   uio.uio_procp = curproc;
> - return (process_domem(curproc, p, , PT_READ_I));
> + return (process_domem(curproc, p, ));
>  }
>  
>  int
> @@ -708,7 +708,7 @@ ss_put_value(struct proc *p, vaddr_t addr, u_int value)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_WRITE;
>   uio.uio_procp = curproc;
> - return (process_domem(curproc, p, , PT_WRITE_I));
> + return (process_domem(curproc, p, ));
>  }
>  
>  void
> diff --git a/sys/arch/m88k/m88k/trap.c b/sys/arch/m88k/m88k/trap.c
> index d11f8ca..734743d 100644
> --- a/sys/arch/m88k/m88k/trap.c
> +++ b/sys/arch/m88k/m88k/trap.c
> @@ -1447,7 +1447,7 @@ ss_get_value(struct proc *p, vaddr_t addr, u_int *value)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_READ;
>   uio.uio_procp = curproc;
> - return (process_domem(curproc, p, , PT_READ_I));
> + return (process_domem(curproc, p, ));
>  }
>  
>  int
> @@ -1465,7 +1465,7 @@ ss_put_value(struct proc *p, vaddr_t addr, u_int value)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_WRITE;
>   uio.uio_procp = curproc;
> - return (process_domem(curproc, p, , PT_WRITE_I));
> + return (process_domem(curproc, p, ));
>  }
>  
>  /*
> diff --git a/sys/arch/mips64/mips64/trap.c b/sys/arch/mips64/mips64/trap.c
> index 0bd71e5..9e81952 100644
> --- a/sys/arch/mips64/mips64/trap.c
> +++ b/sys/arch/mips64/mips64/trap.c
> @@ -1021,7 +1021,7 @@ ptrace_read_insn(struct proc *p, vaddr_t va, uint32_t 
> *insn)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_READ;
>   uio.uio_procp = p;
> - return process_domem(p, p, , PT_READ_I);
> + return process_domem(p, p, );
>  }
>  
>  int
> @@ -1039,7 +1039,7 @@ ptrace_write_insn(struct proc *p, vaddr_t va, uint32_t 
> insn)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = UIO_WRITE;
>   uio.uio_procp = p;
> - return process_domem(p, p, , PT_WRITE_I);
> + return process_domem(p, p, );
>  }
>  
>  /*
> diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
> index 60ec50e..4d589e7 100644
> --- a/sys/kern/sys_process.c
> +++ b/sys/kern/sys_process.c
> @@ -368,8 +368,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
>   uio.uio_segflg = UIO_SYSSPACE;
>   uio.uio_rw = write ? UIO_WRITE : UIO_READ;
>   uio.uio_procp = p;
> - error = process_domem(p, t, , write ? PT_WRITE_I :
> - PT_READ_I);
> + error = process_domem(p, t, );
>   if (write == 0)
>   *retval = temp;
>   return (error);
> @@ -387,23 +386,14 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
>   uio.uio_procp = p;
>   switch (piod.piod_op) {
>   case PIOD_READ_I:
> - req = PT_READ_I;
> - uio.uio_rw = UIO_READ;
> - break;
>   case PIOD_READ_D:
> - req = PT_READ_D;
>   uio.uio_rw = UIO_READ;
>   break;
>   case PIOD_WRITE_I:
> - req = PT_WRITE_I;
> -

Re: ptrace PT_IO write bug

[Mathieu -]

Martin Natano wrote:
> The diff reads fine to me, however it is incomplete. There are some
> callers of process_domem() in arch/. They will need to be changed too.
> req seems to be in sync with uio_rw in all the cases, so just removing
> the last argument should do it.
> 

Thanks for the feedback. The missing callers where an overlook on my
part, sorry for that.

Here is a regenerated diff including all the call site. As a side note,
obviously every one of them was using PT_WRITE_I, that's why it went
unnoticed.

Mathieu-


diff --git a/sys/arch/alpha/alpha/process_machdep.c 
b/sys/arch/alpha/alpha/process_machdep.c
index 6fe711e..291e06e 100644
--- a/sys/arch/alpha/alpha/process_machdep.c
+++ b/sys/arch/alpha/alpha/process_machdep.c
@@ -181,7 +181,7 @@ ptrace_read_int(struct proc *p, vaddr_t addr, u_int32_t *v)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_READ;
uio.uio_procp = p;
-   return process_domem(curproc, p, , PT_READ_I);
+   return process_domem(curproc, p, );
 }
 
 int
@@ -199,7 +199,7 @@ ptrace_write_int(struct proc *p, vaddr_t addr, u_int32_t v)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_WRITE;
uio.uio_procp = p;
-   return process_domem(curproc, p, , PT_WRITE_I);
+   return process_domem(curproc, p, );
 }
 
 u_int64_t
diff --git a/sys/arch/hppa/hppa/trap.c b/sys/arch/hppa/hppa/trap.c
index e86e636..805a924 100644
--- a/sys/arch/hppa/hppa/trap.c
+++ b/sys/arch/hppa/hppa/trap.c
@@ -690,7 +690,7 @@ ss_get_value(struct proc *p, vaddr_t addr, u_int *value)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_READ;
uio.uio_procp = curproc;
-   return (process_domem(curproc, p, , PT_READ_I));
+   return (process_domem(curproc, p, ));
 }
 
 int
@@ -708,7 +708,7 @@ ss_put_value(struct proc *p, vaddr_t addr, u_int value)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_WRITE;
uio.uio_procp = curproc;
-   return (process_domem(curproc, p, , PT_WRITE_I));
+   return (process_domem(curproc, p, ));
 }
 
 void
diff --git a/sys/arch/m88k/m88k/trap.c b/sys/arch/m88k/m88k/trap.c
index d11f8ca..734743d 100644
--- a/sys/arch/m88k/m88k/trap.c
+++ b/sys/arch/m88k/m88k/trap.c
@@ -1447,7 +1447,7 @@ ss_get_value(struct proc *p, vaddr_t addr, u_int *value)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_READ;
uio.uio_procp = curproc;
-   return (process_domem(curproc, p, , PT_READ_I));
+   return (process_domem(curproc, p, ));
 }
 
 int
@@ -1465,7 +1465,7 @@ ss_put_value(struct proc *p, vaddr_t addr, u_int value)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_WRITE;
uio.uio_procp = curproc;
-   return (process_domem(curproc, p, , PT_WRITE_I));
+   return (process_domem(curproc, p, ));
 }
 
 /*
diff --git a/sys/arch/mips64/mips64/trap.c b/sys/arch/mips64/mips64/trap.c
index 0bd71e5..9e81952 100644
--- a/sys/arch/mips64/mips64/trap.c
+++ b/sys/arch/mips64/mips64/trap.c
@@ -1021,7 +1021,7 @@ ptrace_read_insn(struct proc *p, vaddr_t va, uint32_t 
*insn)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_READ;
uio.uio_procp = p;
-   return process_domem(p, p, , PT_READ_I);
+   return process_domem(p, p, );
 }
 
 int
@@ -1039,7 +1039,7 @@ ptrace_write_insn(struct proc *p, vaddr_t va, uint32_t 
insn)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = UIO_WRITE;
uio.uio_procp = p;
-   return process_domem(p, p, , PT_WRITE_I);
+   return process_domem(p, p, );
 }
 
 /*
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 60ec50e..4d589e7 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -368,8 +368,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
uio.uio_procp = p;
-   error = process_domem(p, t, , write ? PT_WRITE_I :
-   PT_READ_I);
+   error = process_domem(p, t, );
if (write == 0)
*retval = temp;
return (error);
@@ -387,23 +386,14 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_procp = p;
switch (piod.piod_op) {
case PIOD_READ_I:
-   req = PT_READ_I;
-   uio.uio_rw = UIO_READ;
-   break;
case PIOD_READ_D:
-   req = PT_READ_D;
uio.uio_rw = UIO_READ;
break;
case PIOD_WRITE_I:
-   req = PT_WRITE_I;
-   uio.uio_rw = UIO_WRITE;
-   break;
case PIOD_WRITE_D:
-   req = PT_WRITE_D;
uio.uio_rw = UIO_WRITE;
break;
case PIOD_READ_AUXV:
-   

Re: ptrace PT_IO write bug

[Martin Natano]

On Fri, May 27, 2016 at 10:15:39PM +0200, Jeremie Courreges-Anglas wrote:
> Mathieu -  writes:
> 
> > Hello everyone,
> >
> > While playing a bit with ptrace to do some debugging I stumbled upon
> > something that looks like a bug.
> > While trying to write to the ptrace'd process using PT_IO in combinaison
> > with PIOD_WRITE_D I kept getting EFAULTs.
> > PIOD_READ_D would work fine on the same address though, and even more
> > weirdly PIOD_WRITE_I would also work fine on the same address.
> > Even more strange, PT_WRITE_D works fine too on the same address.
> > So in effect, only PT_IO + PIOD_WRITE_D would EFAULT on this address.
> >
> > If this is the expected behavior (I really doubt it), then the man page
> > is wrong because it states clearly that *_I and *_D are equivalent.
> >
> > Digging a bit on the implementation I traced it back to rev 1.33 of
> > sys_process.c.
> > The old implementation used procfs_domem to do the uvm_io call, and
> > based the decision as to use UVM_IO_FIXPROT or not on the uio.uio_rw
> > field (UIO_WRITE meant FIXPROT vs UIO_READ).
> > However the new implementation, in process_domem takes a third
> > parameter, req, the ptrace request and would use UVM_IO_FIXPROT only in
> > the PT_WRITE_I case (rings any bell?).
> > That's why PT_WRITE_D will EFAULT in any case.
> > Oh and PT_WRITE_I and PT_WRITE_D work because they both use PT_WRITE_I
> > as the req parameter :
> >  process_domem(p, t, , write ? PT_WRITE_I : 
> >
> > So I came up with the following diff (kind of the big hammer approach),
> > which gets rid of the req parameters and base the UVM_IO_FIXPROT
> > decision on the uio.uio_rw field as the previous code (10 years ago!)
> > was doing.
> 
> This looks correct to me.  ok / can I get another review?
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

The diff reads fine to me, however it is incomplete. There are some
callers of process_domem() in arch/. They will need to be changed too.
req seems to be in sync with uio_rw in all the cases, so just removing
the last argument should do it.

natano

Re: ptrace PT_IO write bug

[Jeremie Courreges-Anglas]

Mathieu -  writes:

> Hello everyone,
>
> While playing a bit with ptrace to do some debugging I stumbled upon
> something that looks like a bug.
> While trying to write to the ptrace'd process using PT_IO in combinaison
> with PIOD_WRITE_D I kept getting EFAULTs.
> PIOD_READ_D would work fine on the same address though, and even more
> weirdly PIOD_WRITE_I would also work fine on the same address.
> Even more strange, PT_WRITE_D works fine too on the same address.
> So in effect, only PT_IO + PIOD_WRITE_D would EFAULT on this address.
>
> If this is the expected behavior (I really doubt it), then the man page
> is wrong because it states clearly that *_I and *_D are equivalent.
>
> Digging a bit on the implementation I traced it back to rev 1.33 of
> sys_process.c.
> The old implementation used procfs_domem to do the uvm_io call, and
> based the decision as to use UVM_IO_FIXPROT or not on the uio.uio_rw
> field (UIO_WRITE meant FIXPROT vs UIO_READ).
> However the new implementation, in process_domem takes a third
> parameter, req, the   ptrace request and would use UVM_IO_FIXPROT only in
> the PT_WRITE_I case (rings any bell?).
> That's why PT_WRITE_D will EFAULT in any case.
> Oh and PT_WRITE_I and PT_WRITE_D work because they both use PT_WRITE_I
> as the req parameter :
>  process_domem(p, t, , write ? PT_WRITE_I : 
>
> So I came up with the following diff (kind of the big hammer approach),
> which gets rid of the req parameters and base the UVM_IO_FIXPROT
> decision on the uio.uio_rw field as the previous code (10 years ago!)
> was doing.

This looks correct to me.  ok / can I get another review?

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

ptrace PT_IO write bug

[Mathieu -]

Hello everyone,

While playing a bit with ptrace to do some debugging I stumbled upon
something that looks like a bug.
While trying to write to the ptrace'd process using PT_IO in combinaison
with PIOD_WRITE_D I kept getting EFAULTs.
PIOD_READ_D would work fine on the same address though, and even more
weirdly PIOD_WRITE_I would also work fine on the same address.
Even more strange, PT_WRITE_D works fine too on the same address.
So in effect, only PT_IO + PIOD_WRITE_D would EFAULT on this address.

If this is the expected behavior (I really doubt it), then the man page
is wrong because it states clearly that *_I and *_D are equivalent.

Digging a bit on the implementation I traced it back to rev 1.33 of
sys_process.c.
The old implementation used procfs_domem to do the uvm_io call, and
based the decision as to use UVM_IO_FIXPROT or not on the uio.uio_rw
field (UIO_WRITE meant FIXPROT vs UIO_READ).
However the new implementation, in process_domem takes a third
parameter, req, the ptrace request and would use UVM_IO_FIXPROT only in
the PT_WRITE_I case (rings any bell?).
That's why PT_WRITE_D will EFAULT in any case.
Oh and PT_WRITE_I and PT_WRITE_D work because they both use PT_WRITE_I
as the req parameter :
 process_domem(p, t, , write ? PT_WRITE_I : 

So I came up with the following diff (kind of the big hammer approach),
which gets rid of the req parameters and base the UVM_IO_FIXPROT
decision on the uio.uio_rw field as the previous code (10 years ago!)
was doing.

nota: I was curious as to how gdb worked with this, but turns out they
just bruteforce both PIOD_WRITE_I and PIOD_WRITE_D flag until it works
:).

diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 60ec50e..4d589e7 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -368,8 +368,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_segflg = UIO_SYSSPACE;
uio.uio_rw = write ? UIO_WRITE : UIO_READ;
uio.uio_procp = p;
-   error = process_domem(p, t, , write ? PT_WRITE_I :
-   PT_READ_I);
+   error = process_domem(p, t, );
if (write == 0)
*retval = temp;
return (error);
@@ -387,23 +386,14 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
uio.uio_procp = p;
switch (piod.piod_op) {
case PIOD_READ_I:
-   req = PT_READ_I;
-   uio.uio_rw = UIO_READ;
-   break;
case PIOD_READ_D:
-   req = PT_READ_D;
uio.uio_rw = UIO_READ;
break;
case PIOD_WRITE_I:
-   req = PT_WRITE_I;
-   uio.uio_rw = UIO_WRITE;
-   break;
case PIOD_WRITE_D:
-   req = PT_WRITE_D;
uio.uio_rw = UIO_WRITE;
break;
case PIOD_READ_AUXV:
-   req = PT_READ_D;
uio.uio_rw = UIO_READ;
temp = tr->ps_emul->e_arglen * sizeof(char *);
if (uio.uio_offset > temp)
@@ -418,7 +408,7 @@ sys_ptrace(struct proc *p, void *v, register_t *retval)
default:
return (EINVAL);
}
-   error = process_domem(p, t, , req);
+   error = process_domem(p, t, );
piod.piod_len -= uio.uio_resid;
(void) copyout(, SCARG(uap, addr), sizeof(piod));
return (error);
@@ -711,7 +701,7 @@ process_checkioperm(struct proc *p, struct process *tr)
 }
 
 int
-process_domem(struct proc *curp, struct proc *p, struct uio *uio, int req)
+process_domem(struct proc *curp, struct proc *p, struct uio *uio)
 {
struct vmspace *vm;
int error;
@@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p, struct 
uio *uio, int req)
vm->vm_refcnt++;
 
error = uvm_io(>vm_map, uio,
-   (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
+   (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
 
uvmspace_free(vm);
 
-   if (error == 0 && req == PT_WRITE_I)
+   if (error == 0 && uio->uio_rw == UIO_WRITE)
pmap_proc_iflush(p, addr, len);
 
return (error);
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index 3c8fda3..b8b76a0 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
@@ -116,7 +116,7 @@ int process_write_fpregs(struct proc *p, struct fpreg 
*regs);
 #endif
 intprocess_write_regs(struct proc *p, struct reg *regs);
 intprocess_checkioperm(struct proc *, struct process *);
-intprocess_domem(struct proc *, struct proc *, struct uio *, int);
+intprocess_domem(struct proc *, struct proc *, struct uio *);
 
 #ifndef FIX_SSTEP
 #define FIX_SSTEP(p)