ptrace: Fix PTRACE_GETREGSET/PTRACE_SETREGSET in code documentation

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.

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.

Quoted 4 lines · 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.

Well, it kind of does, here:

* struct iovec iov = { buf, len};

include/uapi/linux/ptrace.h line 51

Quoted 13 lines

include/uapi/linux/ptrace.h +6 -2

@@ -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.

Quoted 5 lines

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.

Quoted 9 lines · Pedro Alves

Quoted 4 lines · 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.

Well, it kind of does, here:

* struct iovec iov = { buf, len};

:) Thats not explicit enough.

Quoted 19 lines

include/uapi/linux/ptrace.h line 51

Quoted 13 lines

include/uapi/linux/ptrace.h +6 -2

@@ -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:

Quoted 13 lines

* 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"

Quoted 3 lines

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.

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;

Quoted 9 lines

Quoted 5 lines

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" ?

Quoted 46 lines · Anshuman Khandual

Quoted 9 lines · Pedro Alves

Quoted 4 lines · 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.

Well, it kind of does, here:

* struct iovec iov = { buf, len};

:) Thats not explicit enough.

Quoted 19 lines

include/uapi/linux/ptrace.h line 51

Quoted 13 lines

include/uapi/linux/ptrace.h +6 -2

@@ -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:

Quoted 13 lines

* 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.

Quoted 2 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 15 lines

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

Quoted 9 lines

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 3 lines · Pedro Alves

Quoted 1 line · Anshuman Khandual

On 05/01/2014 07:43 PM, Pedro Alves wrote:

OK, then this is what I suggest instead:

Quoted 5 lines

Quoted 2 lines

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 ?

From 1237f5ac5896f3910f66df83a5093bb548006188 Mon Sep 17 00:00:00 2001

Quoted 4 lines · Anshuman Khandual

Quoted 3 lines · Pedro Alves

Quoted 1 line · Anshuman Khandual

On 05/01/2014 07:43 PM, Pedro Alves wrote:

OK, then this is what I suggest instead:

...

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines · Anshuman Khandual

Quoted 3 lines · Pedro Alves

Quoted 1 line · Anshuman Khandual

On 05/01/2014 07:43 PM, Pedro Alves wrote:

OK, then this is what I suggest instead:

...

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

Reported-by Anshuman Khandual <[email protected]>

---
 include/uapi/linux/ptrace.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

include/uapi/linux/ptrace.h +1 -3

index cf1019e..30836b9 100644
@@ -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.

Quoted 11 lines

include/uapi/linux/ptrace.h +1 -1

@@ -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);

include/uapi/linux/ptrace.h

Quoted 8 lines

include/uapi/linux/ptrace.h +5 -2

  *
- * 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).

Quoted 11 lines · Anshuman Khandual

Quoted 11 lines

include/uapi/linux/ptrace.h +1 -1

@@ -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);

Quoted 8 lines

include/uapi/linux/ptrace.h

Quoted 8 lines

include/uapi/linux/ptrace.h +5 -2

  *
- * 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.