[PATCH] powerpc/mm: Fix possible out-of-bounds shift in arch_mmap_rnd()

[PATCH] powerpc/mm: Fix possible out-of-bounds shift in arch_mmap_rnd()

[Michael Ellerman]

The recent patch to add runtime configuration of the ASLR limits added a bug in
arch_mmap_rnd() where we may shift an integer (32-bits) by up to 33 bits,
leading to undefined behaviour.

In practice it exhibits as every process seg faulting instantly, presumably
because the rnd value hasn't been restricited by the modulus at all. We didn't
notice because it only happens under certain kernel configurations and if the
number of bits is actually set to a large value.

Fix it by switching to unsigned long.

Fixes: 9fea59bd7ca5 ("powerpc/mm: Add support for runtime configuration of ASLR limits")
Reported-by: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/mm/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c
index 005aa8a44915..9dbd2a733d6b 100644
--- a/arch/powerpc/mm/mmap.c
+++ b/arch/powerpc/mm/mmap.c
@@ -66,7 +66,7 @@ unsigned long arch_mmap_rnd(void)
 	if (is_32bit_task())
 		shift = mmap_rnd_compat_bits;
 #endif
-	rnd = get_random_long() % (1 << shift);
+	rnd = get_random_long() % (1ul << shift);
 
 	return rnd << PAGE_SHIFT;
 }
-- 
2.7.4

Re: [PATCH] powerpc/mm: Fix possible out-of-bounds shift in arch_mmap_rnd()

[Kees Cook]

On Tue, Apr 25, 2017 at 5:09 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> The recent patch to add runtime configuration of the ASLR limits added a bug in
> arch_mmap_rnd() where we may shift an integer (32-bits) by up to 33 bits,
> leading to undefined behaviour.
>
> In practice it exhibits as every process seg faulting instantly, presumably
> because the rnd value hasn't been restricited by the modulus at all. We didn't
> notice because it only happens under certain kernel configurations and if the
> number of bits is actually set to a large value.
>
> Fix it by switching to unsigned long.
>
> Fixes: 9fea59bd7ca5 ("powerpc/mm: Add support for runtime configuration of ASLR limits")
> Reported-by: Balbir Singh <bsingharora@gmail.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/mm/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c
> index 005aa8a44915..9dbd2a733d6b 100644
> --- a/arch/powerpc/mm/mmap.c
> +++ b/arch/powerpc/mm/mmap.c
> @@ -66,7 +66,7 @@ unsigned long arch_mmap_rnd(void)
>         if (is_32bit_task())
>                 shift = mmap_rnd_compat_bits;
>  #endif
> -       rnd = get_random_long() % (1 << shift);
> +       rnd = get_random_long() % (1ul << shift);
>
>         return rnd << PAGE_SHIFT;
>  }
> --
> 2.7.4

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees


-- 
Kees Cook
Pixel Security

Re: powerpc/mm: Fix possible out-of-bounds shift in arch_mmap_rnd()

[Michael Ellerman]

On Tue, 2017-04-25 at 12:09:41 UTC, Michael Ellerman wrote:
> The recent patch to add runtime configuration of the ASLR limits added a bug in
> arch_mmap_rnd() where we may shift an integer (32-bits) by up to 33 bits,
> leading to undefined behaviour.
> 
> In practice it exhibits as every process seg faulting instantly, presumably
> because the rnd value hasn't been restricited by the modulus at all. We didn't
> notice because it only happens under certain kernel configurations and if the
> number of bits is actually set to a large value.
> 
> Fix it by switching to unsigned long.
> 
> Fixes: 9fea59bd7ca5 ("powerpc/mm: Add support for runtime configuration of ASLR limits")
> Reported-by: Balbir Singh <bsingharora@gmail.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Applied to powerpc next.

https://git.kernel.org/powerpc/c/b409946b2a3c1ddcde75e5f35a77e0

cheers