[PATCH] x86, x86_64: Fix checks for userspace address limit

[PATCH] x86, x86_64: Fix checks for userspace address limit

[Jiri Olsa]

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)
-- 
1.7.1

Re: [PATCH] x86, x86_64: Fix checks for userspace address limit

[Ingo Molnar]

* 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

Re: [PATCH] x86, x86_64: Fix checks for userspace address limit

[Brian Gerst]

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.

--
Brian Gerst

Re: [PATCH] x86, x86_64: Fix checks for userspace address limit

[Ingo Molnar]

* 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

Re: [PATCH] x86, x86_64: Fix checks for userspace address limit

[Linus Torvalds]

On Mon, May 16, 2011 at 4:42 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> 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.

No, I suspect the patch is correct, and it's just a bug. I think it
comes from the "get_user_X()" cases that afaik use "jae" because they
add one less than the size (and thus avoid it entirely for the
single-byte case). See "getuser.S" in the same directory.

But right now I think I need to do a 2.6.39 release later today (after
I get some sleep), so doing it as a stable patch (presumably going
back to pretty much the beginning of time) is probably the right
thing.

              Linus

Re: [PATCH] x86, x86_64: Fix checks for userspace address limit

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, May 16, 2011 at 4:42 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > 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.
> 
> No, I suspect the patch is correct, and it's just a bug. I think it
> comes from the "get_user_X()" cases that afaik use "jae" because they
> add one less than the size (and thus avoid it entirely for the
> single-byte case). See "getuser.S" in the same directory.
> 
> But right now I think I need to do a 2.6.39 release later today (after
> I get some sleep), so doing it as a stable patch (presumably going
> back to pretty much the beginning of time) is probably the right
> thing.

Absolutely - we had this for a long time so it's not a regression and there is 
very little gain from trying to squeeze this fix in so close to v2.6.39 - and 
there are nonzero risks, considering how widely used assembly code this 
changes.

I've queued it up for v2.6.40 with your Acked-by.

Thanks,

	Ingo

[tip:perf/core] x86, 64-bit: Fix copy_[to/from]_user() checks for the userspace address limit

[tip-bot for Jiri Olsa]

Commit-ID:  26afb7c661080ae3f1f13ddf7f0c58c4f931c22b
Gitweb:     http://git.kernel.org/tip/26afb7c661080ae3f1f13ddf7f0c58c4f931c22b
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 12 May 2011 16:30:30 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 18 May 2011 12:49:00 +0200

x86, 64-bit: Fix copy_[to/from]_user() checks for the userspace address limit

As reported in BZ #30352:

  https://bugzilla.kernel.org/show_bug.cgi?id=30352

there's a kernel bug related to reading the last allowed page on x86_64.

The _copy_to_user() and _copy_from_user() functions use the following
check for address limit:

  if (buf + size >= limit)
	fail();

while it should be more permissive:

  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;
 }

The other place checking the addr limit is the access_ok() function,
which is working properly. There's just a misleading comment
for the __range_not_ok() macro - which this patch fixes as well.

The last page of the user-space address range is a guard page and
Brian Gerst observed that the guard page itself due to an erratum on K8 cpus
(#121 Sequential Execution Across Non-Canonical Boundary Causes Processor
Hang).

However, the 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. The guard page is left in place.

This bug would normally not show up because the last page is
part of the process stack and never accessed via syscalls.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Brian Gerst <brgerst@gmail.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@kernel.org>
Link: http://lkml.kernel.org/r/1305210630-7136-1-git-send-email-jolsa@redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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 d17a117..0248402 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -79,7 +79,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,X86_FEATURE_ERMS,	\
 		copy_user_generic_unrolled,copy_user_generic_string,	\
 		copy_user_enhanced_fast_string
@@ -94,7 +94,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,X86_FEATURE_ERMS,	\
 		copy_user_generic_unrolled,copy_user_generic_string,	\
 		copy_user_enhanced_fast_string

Re: [tip:perf/core] x86, 64-bit: Fix copy_[to/from]_user() checks for the userspace address limit

[Linus Torvalds]

On Wed, May 18, 2011 at 1:43 PM, tip-bot for Jiri Olsa <jolsa@redhat.com> wrote:
> As reported in BZ #30352:
>
>  https://bugzilla.kernel.org/show_bug.cgi?id=30352
>
> there's a kernel bug related to reading the last allowed page on x86_64.

Btw, I think this message is very misleading.

It has nothing what-so-ever to do with "the last allowed page".

It is not about pages at all. It's about bytes. It's the very last
byte of the address space. The fact that the *test*program* uses mmap
and then sends a single page is totally irrelevant both for the kernel
and for the patch itself.

                      Linus

Re: [tip:perf/core] x86, 64-bit: Fix copy_[to/from]_user() checks for the userspace address limit

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, May 18, 2011 at 1:43 PM, tip-bot for Jiri Olsa <jolsa@redhat.com> wrote:
> > As reported in BZ #30352:
> >
> >  https://bugzilla.kernel.org/show_bug.cgi?id=30352
> >
> > there's a kernel bug related to reading the last allowed page on x86_64.
> 
> Btw, I think this message is very misleading.
> 
> It has nothing what-so-ever to do with "the last allowed page".

Yeah, i noticed that and this is why i added this bit from Brian's explanation 
to the end of the commit:

    The bug is that the last byte before the guard page can't be read
    because of the off-by-one error. The guard page is left in place.

I should have fixed the original changelog instead of trying to clarify it via 
appending it ...

Thanks,

	Ingo