Re: perf_copy_attr pointer arithmetic weirdness

On Fri, 2009-09-18 at 21:26 +0200, Ian Schram wrote:
There is some -to me at least- weird code in per_copy_attr. Which supposedly
checks that all bytes trailing a struct are zero.

It doesn't seem to get pointer arithmetic right. Since it increments
an iterating pointer by sizeof(unsigned long) rather than 1.

I believe this has an impact on the exploitability of the recent buffer overflow
in the perf_copy_attr function. I'm pretty sure I'm not the only one who noticed
this, but i couldn't find it being mentioned. For some reason people prefer
mmaping something at zero these days?

I have appended a patch locating the issue. The PTR_ALIGN stuff right above it
doesn't seem to take any boundary conditions into account which is probably not
a good thing either.

sizeof(struct perf_counter_attr) should always be a multiple of u64, and
we can indeed read beyond the tail boundary, but that should be ok,
worst that can happen is that we fail the read..

Ugh on the ptr arith, one wonders how many stupid bugs one can make in
such a piece of code... :/

signed-of-by Ian Schram <ischram@xxxxxxxxxx>

Acked-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>

diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index 8cb94a5..9c7590e 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -4208,7 +4208,7 @@ static int perf_copy_attr(struct perf_counter_attr __user *uattr,
end = PTR_ALIGN((void __user *)uattr + size,
sizeof(unsigned long));

- for (; addr < end; addr += sizeof(unsigned long)) {
+ for (; addr < end; ++addr) {
ret = get_user(val, addr);
if (ret)
return ret;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • perf_copy_attr pointer arithmetic weirdness
    ... checks that all bytes trailing a struct are zero. ... Since it increments ... an iterating pointer by sizeofrather than 1. ... ret = get_user; ...
    (Linux-Kernel)
  • Re: perf_copy_attr pointer arithmetic weirdness
    ... checks that all bytes trailing a struct are zero. ... Since it increments ... an iterating pointer by sizeofrather than 1. ... doesn't seem to take any boundary conditions into account which is probably not ...
    (Linux-Kernel)
  • Re: orthogonality of complex (or trigonometric) sinusoids...
    ... of their product gives zero. ... That is only true if the integral of integration is T, ... along with the boundary conditions and any ... And all that with the assumption that the basis functions ...
    (comp.dsp)
  • Re: [Stable-review] [104/145] netfilter: xt_recent: fix false match
    ... A rule with a zero hit_count will always match. ... ret =!ret; ... but decided to go with the simplest patch since 'hits' is local to ... result of the test for further passes of the loop. ...
    (Linux-Kernel)
  • Re: IIR to TIIR
    ... That's why people talk about the periodic nature of the dft. ... The DFT has periodic boundary conditions. ... the derivative goes to zero ... For sound waves we usually measure the pressure: ...
    (comp.dsp)