Messages in this thread | | | Date | Wed, 18 May 2011 10:12:54 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] x86, x86_64: Fix checks for userspace address limit |
| |
* Brian Gerst <brgerst@gmail.com> wrote:
> On Mon, May 16, 2011 at 7:42 AM, Ingo Molnar <mingo@elte.hu> wrote: > > > > * Jiri Olsa <jolsa@redhat.com> wrote: > > > >> hi, > >> there seems to be bug in the _copy_to_user and _copy_from_user > >> functions, not allowing access to the last user page. > >> > >> Also I tried to decipher the inline assembly in __range_not_ok, > >> and it seems to work properly, but the macro comment seems to > >> be misleading. > >> > >> wbr, > >> jirka > >> > >> --- > >> As shown in BZ 30352 (https://bugzilla.kernel.org/show_bug.cgi?id=30352) > >> there's an issue with reading last allowed page on x86_64. > >> > >> The _copy_to_user and _copy_from_user functions use following > >> check for address limit: > >> > >> if (buf + size >= limit) > >> fail > >> > >> while it should be: > >> > >> if (buf + size > limit) > >> fail > >> > >> That's because the size represents the number of bytes being > >> read/write from/to buf address AND including the buf address. > >> So the copy function will actually never touch the limit > >> address even if "buf + size == limit". > >> > >> Following program fails to use the last page as buffer > >> due to the wrong limit check. > >> > >> --- > >> #include <sys/mman.h> > >> #include <sys/socket.h> > >> #include <assert.h> > >> > >> #define PAGE_SIZE (4096) > >> #define LAST_PAGE ((void*)(0x7fffffffe000)) > >> > >> int main() > >> { > >> int fds[2], err; > >> void * ptr = mmap(LAST_PAGE, PAGE_SIZE, PROT_READ | PROT_WRITE, > >> MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); > >> assert(ptr == LAST_PAGE); > >> err = socketpair(AF_LOCAL, SOCK_STREAM, 0, fds); > >> assert(err == 0); > >> err = send(fds[0], ptr, PAGE_SIZE, 0); > >> perror("send"); > >> assert(err == PAGE_SIZE); > >> err = recv(fds[1], ptr, PAGE_SIZE, MSG_WAITALL); > >> perror("recv"); > >> assert(err == PAGE_SIZE); > >> return 0; > >> } > >> --- > >> > >> Other place checking the addr limit is access_ok function, > >> which is working properly. There's just misleading comment > >> for the __range_not_ok macro. > >> > >> > >> Signed-off-by: Jiri Olsa <jolsa@redhat.com> > >> --- > >> arch/x86/include/asm/uaccess.h | 2 +- > >> arch/x86/lib/copy_user_64.S | 4 ++-- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > >> index abd3e0e..99f0ad7 100644 > >> --- a/arch/x86/include/asm/uaccess.h > >> +++ b/arch/x86/include/asm/uaccess.h > >> @@ -42,7 +42,7 @@ > >> * Returns 0 if the range is valid, nonzero otherwise. > >> * > >> * This is equivalent to the following test: > >> - * (u33)addr + (u33)size >= (u33)current->addr_limit.seg (u65 for x86_64) > >> + * (u33)addr + (u33)size > (u33)current->addr_limit.seg (u65 for x86_64) > >> * > >> * This needs 33-bit (65-bit for x86_64) arithmetic. We have a carry... > >> */ > >> diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S > >> index 99e4826..a73397f 100644 > >> --- a/arch/x86/lib/copy_user_64.S > >> +++ b/arch/x86/lib/copy_user_64.S > >> @@ -72,7 +72,7 @@ ENTRY(_copy_to_user) > >> addq %rdx,%rcx > >> jc bad_to_user > >> cmpq TI_addr_limit(%rax),%rcx > >> - jae bad_to_user > >> + ja bad_to_user > >> ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string > >> CFI_ENDPROC > >> ENDPROC(_copy_to_user) > >> @@ -85,7 +85,7 @@ ENTRY(_copy_from_user) > >> addq %rdx,%rcx > >> jc bad_from_user > >> cmpq TI_addr_limit(%rax),%rcx > >> - jae bad_from_user > >> + ja bad_from_user > >> ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string > >> CFI_ENDPROC > >> ENDPROC(_copy_from_user) > > > > Hm, something tickles me about this area that we would reintroduce a security > > hole, that we really wanted to treat the last page of user-space as some sort > > of guard page but i cannot quite remember it why ... > > > > IIRC Linus wrote bits of this so i'm Cc:-ing him just in case he remembers. > > > > Thanks, > > > > Ingo > > The guard page is apparently due to an erratum on K8 cpus (#121 > Sequential Execution Across Non-Canonical Boundary > Causes Processor Hang). However, his test code is using the last > valid page before the guard page. The bug is that the last byte > before the guard page can't be read because of the off-by-one error.
Ok, so if you otherwise agree with the change then Jiri please update the changelog with this information and Brian's Acked-by.
Thanks,
Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |