[PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Anshuman Khandual]

The current documentation is bit misleading and does not explicitly
specify that iov.len need to be initialized failing which kernel
may just ignore the ptrace request and never read from/write into
the user specified buffer. This patch fixes the documentation.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 include/uapi/linux/ptrace.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index cf1019e..e9d6b37 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -43,8 +43,12 @@
  *
  *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
  *
- * On the successful completion, iov.len will be updated by the kernel,
- * specifying how much the kernel has written/read to/from the user's iov.buf.
+ * A non-zero value upto the max size of data expected to be written/read by the
+ * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
+ * before initiating the ptrace call. If iov.len is 0, then kernel will neither
+ * read from or write into the user buffer specified. On successful completion,
+ * iov.len will be updated by the kernel, specifying how much the kernel has
+ * written/read to/from the user's iov.buf.
  */
 #define PTRACE_GETREGSET	0x4204
 #define PTRACE_SETREGSET	0x4205
-- 
1.7.11.7

Re: [PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Pedro Alves]

On 04/28/2014 12:00 PM, Anshuman Khandual wrote:
> The current documentation is bit misleading and does not explicitly
> specify that iov.len need to be initialized failing which kernel
> may just ignore the ptrace request and never read from/write into
> the user specified buffer. This patch fixes the documentation.

Well, it kind of does, here:

*      struct iovec iov = { buf, len};

> @@ -43,8 +43,12 @@
>   *
>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>   *
> - * On the successful completion, iov.len will be updated by the kernel,
> - * specifying how much the kernel has written/read to/from the user's iov.buf.
> + * A non-zero value upto the max size of data expected to be written/read by the
> + * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
> + * before initiating the ptrace call. If iov.len is 0, then kernel will neither
> + * read from or write into the user buffer specified. On successful completion,
> + * iov.len will be updated by the kernel, specifying how much the kernel has
> + * written/read to/from the user's iov.buf.

I really appreciate that you're trying to make this clearer, but I
find the new sentence very hard to read/reason.  :-/

I suggest:

 * This interface usage is as follows:
- *      struct iovec iov = { buf, len};
+ *      struct iovec iov = { buf, len };
 *
 *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
 *
- * On the successful completion, iov.len will be updated by the kernel,
- * specifying how much the kernel has written/read to/from the user's iov.buf.
+ * On entry, iov describes the buffer's address and length.  The buffer's
+ * length must be equal to or shorter than the size of the NT_XXX_TYPE regset.
+ * On successful completion, iov.len is updated by the kernel, specifying how
+ * much the kernel has written/read to/from the user's iov.buf.

I'm not sure I understood what you're saying correctly, though.  Specifically,
I don't know whether the buffer's length must really be shorter than the
size of the NT_XXX_TYPE regset.

> The current documentation is bit misleading and does not explicitly
> specify that iov.len need to be initialized failing which kernel
> may just ignore the ptrace request and never read from/write into
> the user specified buffer.

You're saying that if iov.len is larger than the NT_XXX_TYPE regset,
then the kernel returns _success_, but actually doesn't fill the
buffer?  That sounds like a bug to me.

-- 
Pedro Alves

Re: [PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Anshuman Khandual]

On 05/01/2014 07:43 PM, Pedro Alves wrote:
> On 04/28/2014 12:00 PM, Anshuman Khandual wrote:
>> The current documentation is bit misleading and does not explicitly
>> specify that iov.len need to be initialized failing which kernel
>> may just ignore the ptrace request and never read from/write into
>> the user specified buffer. This patch fixes the documentation.
> 
> Well, it kind of does, here:
> 
> *      struct iovec iov = { buf, len};

:) Thats not explicit enough.

> 
>> @@ -43,8 +43,12 @@
>>   *
>>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>   *
>> - * On the successful completion, iov.len will be updated by the kernel,
>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>> + * A non-zero value upto the max size of data expected to be written/read by the
>> + * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
>> + * before initiating the ptrace call. If iov.len is 0, then kernel will neither
>> + * read from or write into the user buffer specified. On successful completion,
>> + * iov.len will be updated by the kernel, specifying how much the kernel has
>> + * written/read to/from the user's iov.buf.
> 
> I really appreciate that you're trying to make this clearer, but I
> find the new sentence very hard to read/reason.  :-/
> 
> I suggest:
> 
>  * This interface usage is as follows:
> - *      struct iovec iov = { buf, len};
> + *      struct iovec iov = { buf, len };
>  *
>  *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>  *
> - * On the successful completion, iov.len will be updated by the kernel,
> - * specifying how much the kernel has written/read to/from the user's iov.buf.
> + * On entry, iov describes the buffer's address and length.  The buffer's
> + * length must be equal to or shorter than the size of the NT_XXX_TYPE regset.
> + * On successful completion, iov.len is updated by the kernel, specifying how
> + * much the kernel has written/read to/from the user's iov.buf.
> 

Yeah, sounds better. I may add "If the length is zero, the kernel will neither read
from or write into the buffer"

> I'm not sure I understood what you're saying correctly, though.  Specifically,
> I don't know whether the buffer's length must really be shorter than the
> size of the NT_XXX_TYPE regset.

No, it does not have to. From the code snippet below (ptrace_regset function)
the buffer length has to be multiple of regset->size for the given NT_XXX_TYPE
upto the max regset size for the user to see any valid data. The problem what I
faced was when you use any iovec structure with the length parameter uninitialized,
the kernel simply ignores and does not return anything.

        if (!regset || (kiov->iov_len % regset->size) != 0)
                return -EINVAL;
 
> 
>> The current documentation is bit misleading and does not explicitly
>> specify that iov.len need to be initialized failing which kernel
>> may just ignore the ptrace request and never read from/write into
>> the user specified buffer.
> 
> You're saying that if iov.len is larger than the NT_XXX_TYPE regset,
> then the kernel returns _success_, but actually doesn't fill the
> buffer?  That sounds like a bug to me.

No, I am not saying that. The kernel takes care of that situation by capping
the length to regset size of the NT_XXX_TYPE.

 kiov->iov_len = min(kiov->iov_len,
                            (__kernel_size_t) (regset->n * regset->size));


Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
moving myself as "Reported-by" ?

Re: [PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Pedro Alves]

On 05/05/14 05:10, Anshuman Khandual wrote:
> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>> On 04/28/2014 12:00 PM, Anshuman Khandual wrote:
>>> The current documentation is bit misleading and does not explicitly
>>> specify that iov.len need to be initialized failing which kernel
>>> may just ignore the ptrace request and never read from/write into
>>> the user specified buffer. This patch fixes the documentation.
>>
>> Well, it kind of does, here:
>>
>> *      struct iovec iov = { buf, len};
> 
> :) Thats not explicit enough.
> 
>>
>>> @@ -43,8 +43,12 @@
>>>   *
>>>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>>   *
>>> - * On the successful completion, iov.len will be updated by the kernel,
>>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>>> + * A non-zero value upto the max size of data expected to be written/read by the
>>> + * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
>>> + * before initiating the ptrace call. If iov.len is 0, then kernel will neither
>>> + * read from or write into the user buffer specified. On successful completion,
>>> + * iov.len will be updated by the kernel, specifying how much the kernel has
>>> + * written/read to/from the user's iov.buf.
>>
>> I really appreciate that you're trying to make this clearer, but I
>> find the new sentence very hard to read/reason.  :-/
>>
>> I suggest:
>>
>>  * This interface usage is as follows:
>> - *      struct iovec iov = { buf, len};
>> + *      struct iovec iov = { buf, len };
>>  *
>>  *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>  *
>> - * On the successful completion, iov.len will be updated by the kernel,
>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>> + * On entry, iov describes the buffer's address and length.  The buffer's
>> + * length must be equal to or shorter than the size of the NT_XXX_TYPE regset.
>> + * On successful completion, iov.len is updated by the kernel, specifying how
>> + * much the kernel has written/read to/from the user's iov.buf.
>>
> 
> Yeah, sounds better. I may add "If the length is zero, the kernel will neither read
> from or write into the buffer"

Well, I think that much should be obvious.  What's not obvious is
whether that is considered success or error (what is the return code?)
I suspect and expect success return if the regset type is known, and
error otherwise.  So that could be used as a way to probe for support
for a given regset without using stack or heap space, if it ever matters.
The kernel never reads/writes beyond iov.len, so better say that, and
then it automatically gets the 0 case handled too, right?

>> I'm not sure I understood what you're saying correctly, though.  Specifically,
>> I don't know whether the buffer's length must really be shorter than the
>> size of the NT_XXX_TYPE regset.
> 
> No, it does not have to. From the code snippet below (ptrace_regset function)
> the buffer length has to be multiple of regset->size for the given NT_XXX_TYPE
> upto the max regset size for the user to see any valid data.

Ah, I guess one could call it a bug.  If the passed in
len is bigger than the whole register set size, then there seems
to be no point in validating whether the length is multiple of
a single register's size.  That unnecessarily prevents coming up
with a register set in the future that has registers of
different sizes...

But given that that's how things are today, I suppose we should
document it...

 The problem what I
> faced was when you use any iovec structure with the length parameter uninitialized,
> the kernel simply ignores and does not return anything.

Ah.  Well, saying "does not return anything" is quite confusing.  It does
return something -- -EINVAL.

> 
>         if (!regset || (kiov->iov_len % regset->size) != 0)
>                 return -EINVAL;

>  
>>
>>> The current documentation is bit misleading and does not explicitly
>>> specify that iov.len need to be initialized failing which kernel
>>> may just ignore the ptrace request and never read from/write into
>>> the user specified buffer.
>>
>> You're saying that if iov.len is larger than the NT_XXX_TYPE regset,
>> then the kernel returns _success_, but actually doesn't fill the
>> buffer?  That sounds like a bug to me.
> 
> No, I am not saying that. The kernel takes care of that situation by capping
> the length to regset size of the NT_XXX_TYPE.
> 
>  kiov->iov_len = min(kiov->iov_len,
>                             (__kernel_size_t) (regset->n * regset->size));
> 
> 

OK, then this is what I suggest instead:

   * This interface usage is as follows:
 - *      struct iovec iov = { buf, len};
 + *      struct iovec iov = { buf, len };
   *
   *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
   *
   * On the successful completion, iov.len will be updated by the kernel,
 - * specifying how much the kernel has written/read to/from the user's iov.buf.
 + * On entry, iov describes the buffer's address and length.  The buffer's
 + * length must be a multiple of the size of a single register in the register set.
 + * The kernel never reads or writes more than iov.len, and caps the buffer
 + * length to the register set's size.  In other words, the kernel reads or
 + * writes min(iov.len, regset size).
 + * On successful completion, iov.len is updated by the kernel, specifying how
 + * much the kernel has read from / written to the user's iov.buf.

> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
> moving myself as "Reported-by" ?

No idea of the actual policy to follow.  Feel free to do that if that's the
standard procedure.

-- 
Pedro Alves

Re: [PATCH] ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

[Anshuman Khandual]

On 05/13/2014 11:39 PM, Pedro Alves wrote:
> On 05/05/14 05:10, Anshuman Khandual wrote:
>> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>>> On 04/28/2014 12:00 PM, Anshuman Khandual wrote:
>>>> The current documentation is bit misleading and does not explicitly
>>>> specify that iov.len need to be initialized failing which kernel
>>>> may just ignore the ptrace request and never read from/write into
>>>> the user specified buffer. This patch fixes the documentation.
>>>
>>> Well, it kind of does, here:
>>>
>>> *      struct iovec iov = { buf, len};
>>
>> :) Thats not explicit enough.
>>
>>>
>>>> @@ -43,8 +43,12 @@
>>>>   *
>>>>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>>>   *
>>>> - * On the successful completion, iov.len will be updated by the kernel,
>>>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>>>> + * A non-zero value upto the max size of data expected to be written/read by the
>>>> + * kernel in response to any NT_XXX_TYPE request type must be assigned to iov.len
>>>> + * before initiating the ptrace call. If iov.len is 0, then kernel will neither
>>>> + * read from or write into the user buffer specified. On successful completion,
>>>> + * iov.len will be updated by the kernel, specifying how much the kernel has
>>>> + * written/read to/from the user's iov.buf.
>>>
>>> I really appreciate that you're trying to make this clearer, but I
>>> find the new sentence very hard to read/reason.  :-/
>>>
>>> I suggest:
>>>
>>>  * This interface usage is as follows:
>>> - *      struct iovec iov = { buf, len};
>>> + *      struct iovec iov = { buf, len };
>>>  *
>>>  *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>>>  *
>>> - * On the successful completion, iov.len will be updated by the kernel,
>>> - * specifying how much the kernel has written/read to/from the user's iov.buf.
>>> + * On entry, iov describes the buffer's address and length.  The buffer's
>>> + * length must be equal to or shorter than the size of the NT_XXX_TYPE regset.
>>> + * On successful completion, iov.len is updated by the kernel, specifying how
>>> + * much the kernel has written/read to/from the user's iov.buf.
>>>
>>
>> Yeah, sounds better. I may add "If the length is zero, the kernel will neither read
>> from or write into the buffer"
> 
> Well, I think that much should be obvious.  What's not obvious is
> whether that is considered success or error (what is the return code?)
> I suspect and expect success return if the regset type is known, and
> error otherwise.  So that could be used as a way to probe for support
> for a given regset without using stack or heap space, if it ever matters.
> The kernel never reads/writes beyond iov.len, so better say that, and
> then it automatically gets the 0 case handled too, right?
> 
>>> I'm not sure I understood what you're saying correctly, though.  Specifically,
>>> I don't know whether the buffer's length must really be shorter than the
>>> size of the NT_XXX_TYPE regset.
>>
>> No, it does not have to. From the code snippet below (ptrace_regset function)
>> the buffer length has to be multiple of regset->size for the given NT_XXX_TYPE
>> upto the max regset size for the user to see any valid data.
> 
> Ah, I guess one could call it a bug.  If the passed in
> len is bigger than the whole register set size, then there seems
> to be no point in validating whether the length is multiple of
> a single register's size.  That unnecessarily prevents coming up
> with a register set in the future that has registers of
> different sizes...
> 
> But given that that's how things are today, I suppose we should
> document it...
> 
>  The problem what I
>> faced was when you use any iovec structure with the length parameter uninitialized,
>> the kernel simply ignores and does not return anything.
> 
> Ah.  Well, saying "does not return anything" is quite confusing.  It does
> return something -- -EINVAL.
> 
>>
>>         if (!regset || (kiov->iov_len % regset->size) != 0)
>>                 return -EINVAL;
> 
>>  
>>>
>>>> The current documentation is bit misleading and does not explicitly
>>>> specify that iov.len need to be initialized failing which kernel
>>>> may just ignore the ptrace request and never read from/write into
>>>> the user specified buffer.
>>>
>>> You're saying that if iov.len is larger than the NT_XXX_TYPE regset,
>>> then the kernel returns _success_, but actually doesn't fill the
>>> buffer?  That sounds like a bug to me.
>>
>> No, I am not saying that. The kernel takes care of that situation by capping
>> the length to regset size of the NT_XXX_TYPE.
>>
>>  kiov->iov_len = min(kiov->iov_len,
>>                             (__kernel_size_t) (regset->n * regset->size));
>>
>>
> 
> OK, then this is what I suggest instead:
> 
>    * This interface usage is as follows:
>  - *      struct iovec iov = { buf, len};
>  + *      struct iovec iov = { buf, len };
>    *
>    *      ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>    *
>    * On the successful completion, iov.len will be updated by the kernel,
>  - * specifying how much the kernel has written/read to/from the user's iov.buf.
>  + * On entry, iov describes the buffer's address and length.  The buffer's
>  + * length must be a multiple of the size of a single register in the register set.
>  + * The kernel never reads or writes more than iov.len, and caps the buffer
>  + * length to the register set's size.  In other words, the kernel reads or
>  + * writes min(iov.len, regset size).
>  + * On successful completion, iov.len is updated by the kernel, specifying how
>  + * much the kernel has read from / written to the user's iov.buf.
> 
>> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
>> moving myself as "Reported-by" ?
> 
> No idea of the actual policy to follow.  Feel free to do that if that's the
> standard procedure.

Even I am not sure about this, so to preserve the correct authorship, would you
mind sending this patch ?

[PATCH v2] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET, documentation in uapi header

[Pedro Alves]

On 05/14/14 08:10, Anshuman Khandual wrote:
> On 05/13/2014 11:39 PM, Pedro Alves wrote:
>> On 05/05/14 05:10, Anshuman Khandual wrote:
>>> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>> OK, then this is what I suggest instead:
...
>>> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
>>> moving myself as "Reported-by" ?
>>
>> No idea of the actual policy to follow.  Feel free to do that if that's the
>> standard procedure.
> 
> Even I am not sure about this, so to preserve the correct authorship, would you
> mind sending this patch ?

Here you go.  This is against current Linus'.  Please take it from
here if necessary.

8<------------------------------------------
>From 1237f5ac5896f3910f66df83a5093bb548006188 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 14 May 2014 11:05:07 +0100
Subject: [PATCH] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET
 documentation in uapi header

The current comments don't explicitly state in plain words that
iov.len must be set to the buffer's length prior to the ptrace call.
A user might get confused and leave that uninitialized.

In the ptrace_regset function (snippet below) we see that the buffer
length has to be a multiple of the slot/register size for the given
NT_XXX_TYPE:

        if (!regset || (kiov->iov_len % regset->size) != 0)
                return -EINVAL;

Note regset->size is the size of each slot/register in the set, not
the size of the whole set.

And then, we see here:

 kiov->iov_len = min(kiov->iov_len,
                            (__kernel_size_t) (regset->n * regset->size));

that the kernel takes care of capping the requested length to the size
of the whole regset.

Signed-off-by: Pedro Alves <palves@redhat.com>
Reported-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 include/uapi/linux/ptrace.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index cf1019e..30836b9 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -39,12 +39,17 @@
  * payload are exactly the same layout.
  *
  * This interface usage is as follows:
- *	struct iovec iov = { buf, len};
+ *	struct iovec iov = { buf, len };
  *
  *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
  *
- * On the successful completion, iov.len will be updated by the kernel,
- * specifying how much the kernel has written/read to/from the user's iov.buf.
+ * On entry, iov describes the buffer's address and length.  The buffer's length
+ * must be a multiple of the size of a single register in the register set.  The
+ * kernel never reads or writes more than iov.len, and caps the buffer length to
+ * the register set's size.  In other words, the kernel reads or writes
+ * min(iov.len, regset size).  On successful completion, iov.len is updated by
+ * the kernel, specifying how much the kernel has read from / written to the
+ * user's iov.buf.
  */
 #define PTRACE_GETREGSET	0x4204
 #define PTRACE_SETREGSET	0x4205
-- 
1.9.0

Re: [PATCH v2] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET, documentation in uapi header

[Anshuman Khandual]

On 05/14/2014 04:24 PM, Pedro Alves wrote:
> On 05/14/14 08:10, Anshuman Khandual wrote:
>> On 05/13/2014 11:39 PM, Pedro Alves wrote:
>>> On 05/05/14 05:10, Anshuman Khandual wrote:
>>>> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>>> OK, then this is what I suggest instead:
> ...
>>>> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
>>>> moving myself as "Reported-by" ?
>>>
>>> No idea of the actual policy to follow.  Feel free to do that if that's the
>>> standard procedure.
>>
>> Even I am not sure about this, so to preserve the correct authorship, would you
>> mind sending this patch ?
> 
> Here you go.  This is against current Linus'.  Please take it from
> here if necessary.

Thanks Pedro for the patch. I would assume that the ptrace maintainer (Roland or Oleg as
mentioned in the MAINTAINERS file) will pick it from here and merge mainline. Please do
let me know if the process is otherwise different. Thanks.

Re: [PATCH v2] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET, documentation in uapi header

[Anshuman Khandual]

On 05/14/2014 04:24 PM, Pedro Alves wrote:
> On 05/14/14 08:10, Anshuman Khandual wrote:
>> On 05/13/2014 11:39 PM, Pedro Alves wrote:
>>> On 05/05/14 05:10, Anshuman Khandual wrote:
>>>> On 05/01/2014 07:43 PM, Pedro Alves wrote:
>>> OK, then this is what I suggest instead:
> ...
>>>> Shall I resend the patch with the your proposed changes and your "Signed-off-by" and
>>>> moving myself as "Reported-by" ?
>>>
>>> No idea of the actual policy to follow.  Feel free to do that if that's the
>>> standard procedure.
>>
>> Even I am not sure about this, so to preserve the correct authorship, would you
>> mind sending this patch ?
> 
> Here you go.  This is against current Linus'.  Please take it from
> here if necessary.
> 
> 8<------------------------------------------
> From 1237f5ac5896f3910f66df83a5093bb548006188 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 14 May 2014 11:05:07 +0100
> Subject: [PATCH] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET
>  documentation in uapi header
> 
> The current comments don't explicitly state in plain words that
> iov.len must be set to the buffer's length prior to the ptrace call.
> A user might get confused and leave that uninitialized.
> 
> In the ptrace_regset function (snippet below) we see that the buffer
> length has to be a multiple of the slot/register size for the given
> NT_XXX_TYPE:
> 
>         if (!regset || (kiov->iov_len % regset->size) != 0)
>                 return -EINVAL;
> 
> Note regset->size is the size of each slot/register in the set, not
> the size of the whole set.
> 
> And then, we see here:
> 
>  kiov->iov_len = min(kiov->iov_len,
>                             (__kernel_size_t) (regset->n * regset->size));
> 
> that the kernel takes care of capping the requested length to the size
> of the whole regset.
> 
> Signed-off-by: Pedro Alves <palves@redhat.com>
> Reported-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
>  include/uapi/linux/ptrace.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index cf1019e..30836b9 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -39,12 +39,17 @@
>   * payload are exactly the same layout.
>   *
>   * This interface usage is as follows:
> - *	struct iovec iov = { buf, len};
> + *	struct iovec iov = { buf, len };
>   *
>   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>   *
> - * On the successful completion, iov.len will be updated by the kernel,
> - * specifying how much the kernel has written/read to/from the user's iov.buf.
> + * On entry, iov describes the buffer's address and length.  The buffer's length
> + * must be a multiple of the size of a single register in the register set.  The
> + * kernel never reads or writes more than iov.len, and caps the buffer length to
> + * the register set's size.  In other words, the kernel reads or writes
> + * min(iov.len, regset size).  On successful completion, iov.len is updated by
> + * the kernel, specifying how much the kernel has read from / written to the
> + * user's iov.buf.
>   */
>  #define PTRACE_GETREGSET	0x4204
>  #define PTRACE_SETREGSET	0x4205

Hey Peter/Oleg,

The above patch is a documentation fix which we discussed sometime back. Could you please
kindly review and consider merging. Thank you.

Re: [PATCH v2] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET, documentation in uapi header

[Oleg Nesterov]

On 06/12, Anshuman Khandual wrote:
>
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -39,12 +39,17 @@
> >   * payload are exactly the same layout.
> >   *
> >   * This interface usage is as follows:
> > - *	struct iovec iov = { buf, len};
> > + *	struct iovec iov = { buf, len };
> >   *
> >   *	ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
> >   *
> > - * On the successful completion, iov.len will be updated by the kernel,
> > - * specifying how much the kernel has written/read to/from the user's iov.buf.
> > + * On entry, iov describes the buffer's address and length.  The buffer's length
> > + * must be a multiple of the size of a single register in the register set.  The
> > + * kernel never reads or writes more than iov.len, and caps the buffer length to
> > + * the register set's size.  In other words, the kernel reads or writes
> > + * min(iov.len, regset size).

I think this should be self-obvious ;) otherwise why do we need to pass
the length of the buffer?

But of course I won't argue with the additional documentation, perhaps you
can re-send this patch to akpm? Usually ptrace changes are routed via -mm
tree.

But if we update the kernel header, then probably it would be more important
to update the man-page. To me the description of GETREGSET looks good, but
probably it could also mention that buflen should be multiple of regsize (as
you did above). Add Michael.

Oleg.

Re: [PATCH v2] ptrace: Clarify PTRACE_GETREGSET/PTRACE_SETREGSET, documentation in uapi header

[Michael Kerrisk (man-pages)]

On Thu, Jun 12, 2014 at 8:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/12, Anshuman Khandual wrote:
>>
>> > --- a/include/uapi/linux/ptrace.h
>> > +++ b/include/uapi/linux/ptrace.h
>> > @@ -39,12 +39,17 @@
>> >   * payload are exactly the same layout.
>> >   *
>> >   * This interface usage is as follows:
>> > - * struct iovec iov = { buf, len};
>> > + * struct iovec iov = { buf, len };
>> >   *
>> >   * ret = ptrace(PTRACE_GETREGSET/PTRACE_SETREGSET, pid, NT_XXX_TYPE, &iov);
>> >   *
>> > - * On the successful completion, iov.len will be updated by the kernel,
>> > - * specifying how much the kernel has written/read to/from the user's iov.buf.
>> > + * On entry, iov describes the buffer's address and length.  The buffer's length
>> > + * must be a multiple of the size of a single register in the register set.  The
>> > + * kernel never reads or writes more than iov.len, and caps the buffer length to
>> > + * the register set's size.  In other words, the kernel reads or writes
>> > + * min(iov.len, regset size).
>
> I think this should be self-obvious ;) otherwise why do we need to pass
> the length of the buffer?
>
> But of course I won't argue with the additional documentation, perhaps you
> can re-send this patch to akpm? Usually ptrace changes are routed via -mm
> tree.
>
> But if we update the kernel header, then probably it would be more important
> to update the man-page. To me the description of GETREGSET looks good, but
> probably it could also mention that buflen should be multiple of regsize (as
> you did above). Add Michael.

Can someone send a patch, then?

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/