Messages in this thread |  | | Date | Wed, 7 Jan 2015 16:22:38 -0800 | | Subject | Re: [PATH] Fix stack randomization on x86_64 bit | | From | Kees Cook <> |
| |
On Wed, Jan 7, 2015 at 11:47 AM, Hector Marco Gisbert <hecmargi@upv.es> wrote: > > [PATH] Fix stack randomization on x86_64 bit
s/PATH/PATCH/
> > The issue is that the stack for processes is not properly randomized on 64 > bit > architectures due to an integer overflow. > > > The affected function is randomize_stack_top() in file "fs/binfmt_elf.c": > > static unsigned long randomize_stack_top(unsigned long stack_top) > { > unsigned int random_variable = 0; > > if ((current->flags & PF_RANDOMIZE) && > !(current->personality & ADDR_NO_RANDOMIZE)) { > random_variable = get_random_int() & STACK_RND_MASK; > random_variable <<= PAGE_SHIFT; > } > #ifdef CONFIG_STACK_GROWSUP > return PAGE_ALIGN(stack_top) + random_variable; > #else > return PAGE_ALIGN(stack_top) - random_variable; > #endif > } > > > Note that, it declares the "random_variable" variable as "unsigned int". > Since > the result of the shifting operation between STACK_RND_MASK (which is > 0x3fffff on > x86_64, 22 bits) and PAGE_SHIFT (which is 12 on x86_64): > > random_variable <<= PAGE_SHIFT; > > then the two leftmost bits are dropped when storing the result in the > "random_variable". This variable shall be at least 34 bits long to hold the > (22+12) result. > > These two dropped bits have an impact on the entropy of process stack. > Concretely, the total stack entropy is reduced by four: from 2^28 to 2^30 > (One > fourth of expected entropy). > > This patch restores back the entropy by correcting the types involved in the > operations in the functions randomize_stack_top() and > stack_maxrandom_size(). > > > Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es> > Signed-off-by: Ismael Ripoll <iripoll@upv.es>
Good catch!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> > > diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c > index 919b912..df4552b 100644 > --- a/arch/x86/mm/mmap.c > +++ b/arch/x86/mm/mmap.c > @@ -35,12 +35,12 @@ struct va_alignment __read_mostly va_align = { > .flags = -1, > }; > > -static unsigned int stack_maxrandom_size(void) > +static unsigned long stack_maxrandom_size(void) > { > - unsigned int max = 0; > + unsigned long max = 0; > if ((current->flags & PF_RANDOMIZE) && > !(current->personality & ADDR_NO_RANDOMIZE)) { > - max = ((-1U) & STACK_RND_MASK) << PAGE_SHIFT; > + max = ((-1UL) & STACK_RND_MASK) << PAGE_SHIFT; > } > > return max; > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index d8fc060..ee668b4 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -554,11 +554,11 @@ out: > > static unsigned long randomize_stack_top(unsigned long stack_top) > { > - unsigned int random_variable = 0; > + unsigned long random_variable = 0; > > if ((current->flags & PF_RANDOMIZE) && > !(current->personality & ADDR_NO_RANDOMIZE)) { > - random_variable = get_random_int() & STACK_RND_MASK; > + random_variable = (unsigned long) get_random_int() & > STACK_RND_MASK; > random_variable <<= PAGE_SHIFT; > } > #ifdef CONFIG_STACK_GROWSUP > >
-- Kees Cook Chrome OS Security
|  |