[ariel-miculas]

1. As I said in the original thread:

  There is already a check at the beginning of the function:
  if (index > PT_FPSCR)
     return -EIO;

2. fpscr will actually be written to by this line of code:

  +       ((unsigned int \*)child->thread.fp_state.fpr)
  +               [FPRINDEX(index)] = data;

3. This is a nitpick.

So please tell me how my patch was buggy.

[kazinator]

1. The index > PT_FPSCR test isn't good enough:

  int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data)
  {
          unsigned int fpidx = index - PT_FPR0;

          if (index > PT_FPSCR)
                  return -EIO;

          flush_fp_to_thread(child);
          if (fpidx < (PT_FPSCR - PT_FPR0))
                  memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long));
          else
                  *data = child->thread.fp_state.fpscr;

          return 0;
  }

It is testing the original signed value of type int, which could be negative, and therefore convert to a huge unsigned int value manifested in fpidx.

(Also, the function risks integer subtraction underflow in "index - PT_FPR0". If we pass in the value INT_MIN, subtracting a positive value from it is undefined behavior.)

It looks like the original author knew that the -EIO test is too weak, so the converted unsigned value is tested again.

Regarding 2, accessing fpscr using fpr[32] is undefined behavior (out of bounds array access), which is probably why the original code is refraining from that, but testing for that value and separately accessing the member.

However is also this bit of uncertainty (I have not fully drilled into). On 32 bit PPC, that PT_FPSCR is defined with an extra offset of 1:

   #define PT_FPSCR (PT_FPR0 + 2*32 + 1)                                                               

I suspect that this is due to PPC being big endian; the + 1 displaces the index to the lower half of a 64 bit word. This also tells me that this fpscr register must be 32 bits on PPC32?

Thus, even with the range check in place, out-of-bounds access past the array is possible if user space requests the [64] index since the range check is testing for below 65. The [64] index isn't fpscr, but the upper half of u64 fpscr.

There is also the aspect that when we assign to fpscr, the whole 64 bit value is assigned, not just half of it.