On 15/11/2022 10:43, Teo Couprie Diaz wrote:
On 11/11/2022 17:49, Kristina Martsenko wrote:
The PTRACE_POKEDATA request writes a word of data to the tracee's memory. In PCuABI the size of the write remains 8 bytes. Currently the kernel erroneously writes 16 bytes, thereby overwriting 8 bytes of unrelated memory. Fix this by restoring the type of the data argument of generic_ptrace_pokedata() to unsigned long.
The fix makes sense by itself, maybe it's my lack of experience with kernel dev but do we always expect it to be 8 bytes of data to trace ?
The ptrace man page states that it's a machine "word", so 8 on 64-bit systems and 4 on 32-bit systems. (It remains 8 on Morello according to the PCuABI spec, there is a new request type PTRACE_POKECAP for writing 16-byte capabilities.)
If so, isn't there some type that could be more specific (and ideally fixed in size) that could be used here as to prevent further confusion later and be sure that the right amount of bytes will always be written ?
Yeah, that would make sense. But the C language doesn't have a type for the machine word size, and I think the kernel just uses "unsigned long" for that. Also note that in this particular case I'm just reverting part of the change made in commit 343ca6c6706e ("kernel/ptrace: Modify ptrace syscall to accept capability arguments"), not introducing a new change.
I'm also a bit confused when looking a the function where it gets called from :
int ptrace_request(struct task_struct *child, long request, user_uintptr_t addr, user_uintptr_t data)
Here `addr` is an `user_uinptr_t`, which gets passed to `generic_ptrace_pokedata()` which will use it to poke said data. But there the type of `addr` is `unsigned long`, shouldn't it be `user_uintptr_t` as well ?
The type in ptrace_request() needs to be user_uintptr_t because ptrace is a multiplexed syscall that can take both pointers and integers in the 'addr' and 'data' arguments. Based on the request type (the big switch statement in ptrace_request()), the argument gets interpreted as either a pointer or integer. In the case of PTRACE_POKEDATA, the PCuABI spec [1] states that 'addr' is interpreted as a 64-bit value, because it holds an address in the tracee's address space (for which the tracer doesn't have a capability). So the call to generic_ptrace_pokedata() implicitly converts it to unsigned long.
It also means that `data` is going through the same kind of type change, but it's probably less of an issue because it's just data to be copied around.
'data' also needs to be interpreted as a 64-bit value for the PTRACE_POKEDATA request type because it needs to be the size of a 'word', as described above.
I'm not sure what I am saying is really relevant, if not please ignore ! :)
Thanks for having a look!
Kristina
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Regards, Téo
Fixes: ("kernel/ptrace: Modify ptrace syscall to accept capability arguments") Signed-off-by: Kristina Martsenko kristina.martsenko@arm.com
include/linux/ptrace.h | 2 +- kernel/ptrace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index a4c84dbbe084..cfdd378636c7 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -108,7 +108,7 @@ static inline void ptrace_unlink(struct task_struct *child) int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr, user_uintptr_t data); int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr, - user_uintptr_t data); + unsigned long data); /** * ptrace_parent - return the task that is tracing the given task diff --git a/kernel/ptrace.c b/kernel/ptrace.c index c278ae0058a6..e5343257131f 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -1350,7 +1350,7 @@ int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr, } int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr, - user_uintptr_t data) + unsigned long data) { int copied;
On 15/11/2022 13:43, Kristina Martsenko wrote:
On 15/11/2022 10:43, Teo Couprie Diaz wrote:
On 11/11/2022 17:49, Kristina Martsenko wrote:
The PTRACE_POKEDATA request writes a word of data to the tracee's memory. In PCuABI the size of the write remains 8 bytes. Currently the kernel erroneously writes 16 bytes, thereby overwriting 8 bytes of unrelated memory. Fix this by restoring the type of the data argument of generic_ptrace_pokedata() to unsigned long.
The fix makes sense by itself, maybe it's my lack of experience with kernel dev but do we always expect it to be 8 bytes of data to trace ?
The ptrace man page states that it's a machine "word", so 8 on 64-bit systems and 4 on 32-bit systems. (It remains 8 on Morello according to the PCuABI spec, there is a new request type PTRACE_POKECAP for writing 16-byte capabilities.)
Ah right, it makes more sense knowing there is a new request type for capabilities.
If so, isn't there some type that could be more specific (and ideally fixed in size) that could be used here as to prevent further confusion later and be sure that the right amount of bytes will always be written ?
Yeah, that would make sense. But the C language doesn't have a type for the machine word size, and I think the kernel just uses "unsigned long" for that.
Noted, I figured the kernel might have its own one as it seems to have a few useful typedefs but if it doesn't then I understand the use of `unsigned long`.
Also note that in this particular case I'm just reverting part of the change made in commit 343ca6c6706e ("kernel/ptrace: Modify ptrace syscall to accept capability arguments"), not introducing a new change.
I did notice, but I figured I would ask if there was another type that existed that could fit better than what was previously there. But I guess if it could, it would already be there ! :)
I'm also a bit confused when looking a the function where it gets called from :
int ptrace_request(struct task_struct *child, long request, user_uintptr_t addr, user_uintptr_t data)
Here `addr` is an `user_uinptr_t`, which gets passed to `generic_ptrace_pokedata()` which will use it to poke said data. But there the type of `addr` is `unsigned long`, shouldn't it be `user_uintptr_t` as well ?
The type in ptrace_request() needs to be user_uintptr_t because ptrace is a multiplexed syscall that can take both pointers and integers in the 'addr' and 'data' arguments. Based on the request type (the big switch statement in ptrace_request()), the argument gets interpreted as either a pointer or integer. In the case of PTRACE_POKEDATA, the PCuABI spec [1] states that 'addr' is interpreted as a 64-bit value, because it holds an address in the tracee's address space (for which the tracer doesn't have a capability). So the call to generic_ptrace_pokedata() implicitly converts it to unsigned long.
Ah, ok, I didn't quite get the relationship between the different pointers at first then ! It does make sense that it would just be used as a raw value.
It also means that `data` is going through the same kind of type change, but it's probably less of an issue because it's just data to be copied around.
'data' also needs to be interpreted as a 64-bit value for the PTRACE_POKEDATA request type because it needs to be the size of a 'word', as described above.
That makes sense, I figured it would be ok in this case.
I'm not sure what I am saying is really relevant, if not please ignore ! :)
Thanks for having a look!
Kristina
No worries, thanks for taking the time to respond with all those details !
Regards, Téo
PS: I did make a mistake and did not reply to the mailing list with my first message, sorry. Thanks for looping it back in correctly !
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Regards, Téo
Fixes: ("kernel/ptrace: Modify ptrace syscall to accept capability arguments") Signed-off-by: Kristina Martsenko kristina.martsenko@arm.com
include/linux/ptrace.h | 2 +- kernel/ptrace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index a4c84dbbe084..cfdd378636c7 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -108,7 +108,7 @@ static inline void ptrace_unlink(struct task_struct *child) int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr, user_uintptr_t data); int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr, - user_uintptr_t data); + unsigned long data); /** * ptrace_parent - return the task that is tracing the given task diff --git a/kernel/ptrace.c b/kernel/ptrace.c index c278ae0058a6..e5343257131f 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -1350,7 +1350,7 @@ int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr, } int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr, - user_uintptr_t data) + unsigned long data) { int copied;
linux-morello@op-lists.linaro.org
-
Kristina Martsenko -
Teo Couprie Diaz