Re: [PATCH] x86, x86_64: Fix checks for userspace address limit
From: Ingo Molnar
Date: Mon May 16 2011 - 07:43:19 EST
* Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
--
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/