Re: [PATCH] ptrace: add PTRACE_GET_RSEQ_CONFIGURATION request

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Feb 22, 2021, at 6:57 AM, Dmitry V. Levin ldv@altlinux.org wrote:

> On Mon, Feb 22, 2021 at 11:04:43AM +0100, Piotr Figiel wrote:
> [...]
>> --- a/include/uapi/linux/ptrace.h
>> +++ b/include/uapi/linux/ptrace.h
>> @@ -102,6 +102,14 @@ struct ptrace_syscall_info {
>>  	};
>>  };
>>  
>> +#define PTRACE_GET_RSEQ_CONFIGURATION	0x420f
>> +
>> +struct ptrace_rseq_configuration {
>> +	__u64 rseq_abi_pointer;
>> +	__u32 signature;
>> +	__u32 pad;
>> +};
>> +
>>  /*
>>   * These values are stored in task->ptrace_message
>>   * by tracehook_report_syscall_* to describe the current syscall-stop.
>> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
>> index 61db50f7ca86..a936af66cf6f 100644
>> --- a/kernel/ptrace.c
>> +++ b/kernel/ptrace.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/cn_proc.h>
>>  #include <linux/compat.h>
>>  #include <linux/sched/signal.h>
>> +#include <linux/minmax.h>
>>  
>>  #include <asm/syscall.h>	/* for syscall_get_* */
>>  
>> @@ -779,6 +780,22 @@ static int ptrace_peek_siginfo(struct task_struct *child,
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_RSEQ
>> +static long ptrace_get_rseq_configuration(struct task_struct *task,
>> +					  unsigned long size, void __user *data)
>> +{
>> +	struct ptrace_rseq_configuration conf = {
>> +		.rseq_abi_pointer = (u64)(uintptr_t)task->rseq,
>> +		.signature = task->rseq_sig,
>> +	};
>> +
>> +	size = min_t(unsigned long, size, sizeof(conf));
>> +	if (copy_to_user(data, &conf, size))
>> +		return -EFAULT;
>> +	return size;
>> +}
>> +#endif
> 
> From API perspective I suggest for such interfaces to return the amount of
> data that could have been written if there was enough room specified, e.g.
> in this case it's sizeof(conf) instead of size.

Looking at the ptrace(2) man page:

RETURN VALUE
       On success, the PTRACE_PEEK* requests return the  requested  data  (but
       see NOTES), the PTRACE_SECCOMP_GET_FILTER request returns the number of
       instructions in the BPF program, and other requests return zero.

       On error, all requests return  -1,  and  errno  is  set  appropriately.
       Since  the  value  returned by a successful PTRACE_PEEK* request may be
       -1, the caller must clear errno before the call, and then check it  af‐
       terward to determine whether or not an error occurred.

It looks like the usual behavior for ptrace requests would be to return 0 when everything
is OK. Unless there a strong motivation for doing different for this new request, I
would be tempted to use the same expected behavior than other requests on success:
return 0.

Unless there is a strong motivation for returning either size or sizeof(conf) ? If we
return sizeof(conf) to user-space, it means it should check it and deal with the
size mismatch. Is that size ever expected to change ?

Thanks,

Mathieu

> 
> 
> --
> ldv

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com