* perf_copy_attr pointer arithmetic weirdness @ 2009-09-18 19:26 Ian Schram 2009-09-18 19:57 ` Peter Zijlstra 2009-09-19 18:04 ` [tip:perfcounters/core] perf_counter: Fix perf_copy_attr() pointer arithmetic tip-bot for Ian Schram 0 siblings, 2 replies; 5+ messages in thread From: Ian Schram @ 2009-09-18 19:26 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: xiaoguangrong 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. (I'm not subscribed, please add me in CC.) signed-of-by Ian Schram <ischram@telenet.be> 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; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: perf_copy_attr pointer arithmetic weirdness 2009-09-18 19:26 perf_copy_attr pointer arithmetic weirdness Ian Schram @ 2009-09-18 19:57 ` Peter Zijlstra 2009-09-19 8:06 ` Ingo Molnar 2009-09-19 18:04 ` [tip:perfcounters/core] perf_counter: Fix perf_copy_attr() pointer arithmetic tip-bot for Ian Schram 1 sibling, 1 reply; 5+ messages in thread From: Peter Zijlstra @ 2009-09-18 19:57 UTC (permalink / raw) To: Ian Schram Cc: Linux Kernel Mailing List, xiaoguangrong, Ingo Molnar, Paul Mackerras 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@telenet.be> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: perf_copy_attr pointer arithmetic weirdness 2009-09-18 19:57 ` Peter Zijlstra @ 2009-09-19 8:06 ` Ingo Molnar 2009-09-19 12:30 ` Ian Schram 0 siblings, 1 reply; 5+ messages in thread From: Ingo Molnar @ 2009-09-19 8:06 UTC (permalink / raw) To: Peter Zijlstra Cc: Ian Schram, Linux Kernel Mailing List, xiaoguangrong, Paul Mackerras * Peter Zijlstra <peterz@infradead.org> wrote: > 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@telenet.be> > > Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Ian, you meant Signed-off-by, not signed-of-by, right? Ingo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: perf_copy_attr pointer arithmetic weirdness 2009-09-19 8:06 ` Ingo Molnar @ 2009-09-19 12:30 ` Ian Schram 0 siblings, 0 replies; 5+ messages in thread From: Ian Schram @ 2009-09-19 12:30 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Linux Kernel Mailing List, xiaoguangrong, Paul Mackerras Ingo Molnar wrote: > * Peter Zijlstra <peterz@infradead.org> wrote: > >> 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 I don't think this matters since the starting address is not 'forced' to be aligned. In which case some bytes in the middle would be unchecked. All in all seems like an undesirable situation. I'll verify this and try my hand at fixing it properly. Unless somebody who actually understands the purpose of these checks wants to have a go.. >> 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@telenet.be> >> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > > Ian, you meant Signed-off-by, not signed-of-by, right? > exactly right, *shame*, apologies for the extra work for this one line drive-by patch. > Ingo > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:perfcounters/core] perf_counter: Fix perf_copy_attr() pointer arithmetic 2009-09-18 19:26 perf_copy_attr pointer arithmetic weirdness Ian Schram 2009-09-18 19:57 ` Peter Zijlstra @ 2009-09-19 18:04 ` tip-bot for Ian Schram 1 sibling, 0 replies; 5+ messages in thread From: tip-bot for Ian Schram @ 2009-09-19 18:04 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, acme, paulus, hpa, mingo, a.p.zijlstra, efault, fweisbec, ischram, tglx, mingo Commit-ID: cdf8073d6b2c6c5a3cd6ce0e6c1297157f7f99ba Gitweb: http://git.kernel.org/tip/cdf8073d6b2c6c5a3cd6ce0e6c1297157f7f99ba Author: Ian Schram <ischram@telenet.be> AuthorDate: Fri, 18 Sep 2009 21:26:26 +0200 Committer: Ingo Molnar <mingo@elte.hu> CommitDate: Sat, 19 Sep 2009 19:32:55 +0200 perf_counter: Fix perf_copy_attr() pointer arithmetic There is still some 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. Signed-off-by: Ian Schram <ischram@telenet.be> [ v2: clean up the messy PTR_ALIGN logic as well. ] Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Mike Galbraith <efault@gmx.de> Cc: Paul Mackerras <paulus@samba.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: <stable@kernel.org> # for v2.6.31.x LKML-Reference: <4AB3DEE2.3030600@telenet.be> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- kernel/perf_counter.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-) diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c index d5899b6..cc768ab 100644 --- a/kernel/perf_counter.c +++ b/kernel/perf_counter.c @@ -4208,8 +4208,8 @@ done: static int perf_copy_attr(struct perf_counter_attr __user *uattr, struct perf_counter_attr *attr) { - int ret; u32 size; + int ret; if (!access_ok(VERIFY_WRITE, uattr, PERF_ATTR_SIZE_VER0)) return -EFAULT; @@ -4234,19 +4234,19 @@ static int perf_copy_attr(struct perf_counter_attr __user *uattr, /* * If we're handed a bigger struct than we know of, - * ensure all the unknown bits are 0. + * ensure all the unknown bits are 0 - i.e. new + * user-space does not rely on any kernel feature + * extensions we dont know about yet. */ if (size > sizeof(*attr)) { - unsigned long val; - unsigned long __user *addr; - unsigned long __user *end; + unsigned char __user *addr; + unsigned char __user *end; + unsigned char val; - addr = PTR_ALIGN((void __user *)uattr + sizeof(*attr), - sizeof(unsigned long)); - end = PTR_ALIGN((void __user *)uattr + size, - sizeof(unsigned long)); + addr = (void __user *)uattr + sizeof(*attr); + end = (void __user *)uattr + size; - for (; addr < end; addr += sizeof(unsigned long)) { + for (; addr < end; addr++) { ret = get_user(val, addr); if (ret) return ret; ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-09-19 18:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-09-18 19:26 perf_copy_attr pointer arithmetic weirdness Ian Schram 2009-09-18 19:57 ` Peter Zijlstra 2009-09-19 8:06 ` Ingo Molnar 2009-09-19 12:30 ` Ian Schram 2009-09-19 18:04 ` [tip:perfcounters/core] perf_counter: Fix perf_copy_attr() pointer arithmetic tip-bot for Ian Schram
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.