| Message ID | 20220427173128.2603085-6-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stackleak: fixes and rework | expand |
On 27.04.2022 20:31, Mark Rutland wrote: > The logic within __stackleak_erase() can be a little hard to follow, as > `boundary` switches from being the low bound to the high bound mid way > through the function, and `kstack_ptr` is used to represent the start of > the region to erase while `boundary` represents the end of the region to > erase. > > Make this a little clearer by consistently using clearer variable names. > The `boundary` variable is removed, the bounds of the region to erase > are described by `erase_low` and `erase_high`, and bounds of the task > stack are described by `task_stack_low` and `task_stck_high`. A typo here in `task_stck_high`. > As the same time, remove the comment above the variables, since it is > unclear whether it's intended as rationale, a complaint, or a TODO, and > is more confusing than helpful. Yes, this comment is a bit confusing :) I can elaborate. In the original grsecurity patch, the stackleak erasing was written in asm. When I adopted it and proposed for the upstream, Linus strongly opposed this. So I developed stackleak erasing in C. And I wrote this comment to remember that having 'kstack_ptr' and 'boundary' variables on the stack (which we are clearing) would not be good. That was also the main reason why I reused the 'boundary' variable: I wanted the compiler to allocate it in the register and I avoided creating many local variables. Mark, did your refactoring make the compiler allocate local variables on the stack instead of the registers? > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Alexander Popov <alex.popov@linux.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > --- > kernel/stackleak.c | 30 ++++++++++++++---------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/kernel/stackleak.c b/kernel/stackleak.c > index 24b7cf01b2972..d5f684dc0a2d9 100644 > --- a/kernel/stackleak.c > +++ b/kernel/stackleak.c > @@ -73,40 +73,38 @@ late_initcall(stackleak_sysctls_init); > static __always_inline void __stackleak_erase(void) > { > const unsigned long task_stack_low = stackleak_task_low_bound(current); > - > - /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */ > - unsigned long kstack_ptr = current->lowest_stack; > - unsigned long boundary = task_stack_low; > + unsigned long erase_low = current->lowest_stack; > + unsigned long erase_high; > unsigned int poison_count = 0; > const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); > > /* Search for the poison value in the kernel stack */ > - while (kstack_ptr > boundary && poison_count <= depth) { > - if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON) > + while (erase_low > task_stack_low && poison_count <= depth) { > + if (*(unsigned long *)erase_low == STACKLEAK_POISON) > poison_count++; > else > poison_count = 0; > > - kstack_ptr -= sizeof(unsigned long); > + erase_low -= sizeof(unsigned long); > } > > #ifdef CONFIG_STACKLEAK_METRICS > - current->prev_lowest_stack = kstack_ptr; > + current->prev_lowest_stack = erase_low; > #endif > > /* > - * Now write the poison value to the kernel stack. Start from > - * 'kstack_ptr' and move up till the new 'boundary'. We assume that > - * the stack pointer doesn't change when we write poison. > + * Now write the poison value to the kernel stack between 'erase_low' > + * and 'erase_high'. We assume that the stack pointer doesn't change > + * when we write poison. > */ > if (on_thread_stack()) > - boundary = current_stack_pointer; > + erase_high = current_stack_pointer; > else > - boundary = current_top_of_stack(); > + erase_high = current_top_of_stack(); > > - while (kstack_ptr < boundary) { > - *(unsigned long *)kstack_ptr = STACKLEAK_POISON; > - kstack_ptr += sizeof(unsigned long); > + while (erase_low < erase_high) { > + *(unsigned long *)erase_low = STACKLEAK_POISON; > + erase_low += sizeof(unsigned long); > } > > /* Reset the 'lowest_stack' value for the next syscall */
On Sun, May 08, 2022 at 11:49:46PM +0300, Alexander Popov wrote: > On 27.04.2022 20:31, Mark Rutland wrote: > > The logic within __stackleak_erase() can be a little hard to follow, as > > `boundary` switches from being the low bound to the high bound mid way > > through the function, and `kstack_ptr` is used to represent the start of > > the region to erase while `boundary` represents the end of the region to > > erase. > > > > Make this a little clearer by consistently using clearer variable names. > > The `boundary` variable is removed, the bounds of the region to erase > > are described by `erase_low` and `erase_high`, and bounds of the task > > stack are described by `task_stack_low` and `task_stck_high`. > > A typo here in `task_stck_high`. Ah; whoops. > > As the same time, remove the comment above the variables, since it is > > unclear whether it's intended as rationale, a complaint, or a TODO, and > > is more confusing than helpful. > > Yes, this comment is a bit confusing :) I can elaborate. > > In the original grsecurity patch, the stackleak erasing was written in asm. > When I adopted it and proposed for the upstream, Linus strongly opposed this. > So I developed stackleak erasing in C. > > And I wrote this comment to remember that having 'kstack_ptr' and 'boundary' > variables on the stack (which we are clearing) would not be good. Ok, so I think that falls into the "complaint" bucket I mentioned. I understand that we don't have any guarantee from the compiler as to how it will use the stack, and that's obviously a potential problem. > That was also the main reason why I reused the 'boundary' variable: I wanted > the compiler to allocate it in the register and I avoided creating many > local variables. > > Mark, did your refactoring make the compiler allocate local variables on the > stack instead of the registers? Considering the whole series, testing with GCC 11.1.0: * On arm64: before: stackleak_erase() uses 48 bytes of stack after: stackleak_erase() uses 0 bytes of stack Note: this is entirely due to patch 1; arm64 has enough GPRs that it doesn't need to use the stack. * On x86_64: before: stackleak_erase() uses 0 bytes of stack after: stackleak_erase() uses 0 bytes of stack * On i386 before: stackleak_erase() uses 8 bytes of stach after: stackleak_erase() uses 16 bytes of stack The i386 case isn't ideal, but given that those bytes will easily be used by the entry triage code before getting to any syscall handling, I don't believe that's an issue in practice. Thanks, Mark. > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Alexander Popov <alex.popov@linux.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Kees Cook <keescook@chromium.org> > > --- > > kernel/stackleak.c | 30 ++++++++++++++---------------- > > 1 file changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/kernel/stackleak.c b/kernel/stackleak.c > > index 24b7cf01b2972..d5f684dc0a2d9 100644 > > --- a/kernel/stackleak.c > > +++ b/kernel/stackleak.c > > @@ -73,40 +73,38 @@ late_initcall(stackleak_sysctls_init); > > static __always_inline void __stackleak_erase(void) > > { > > const unsigned long task_stack_low = stackleak_task_low_bound(current); > > - > > - /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */ > > - unsigned long kstack_ptr = current->lowest_stack; > > - unsigned long boundary = task_stack_low; > > + unsigned long erase_low = current->lowest_stack; > > + unsigned long erase_high; > > unsigned int poison_count = 0; > > const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); > > /* Search for the poison value in the kernel stack */ > > - while (kstack_ptr > boundary && poison_count <= depth) { > > - if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON) > > + while (erase_low > task_stack_low && poison_count <= depth) { > > + if (*(unsigned long *)erase_low == STACKLEAK_POISON) > > poison_count++; > > else > > poison_count = 0; > > - kstack_ptr -= sizeof(unsigned long); > > + erase_low -= sizeof(unsigned long); > > } > > #ifdef CONFIG_STACKLEAK_METRICS > > - current->prev_lowest_stack = kstack_ptr; > > + current->prev_lowest_stack = erase_low; > > #endif > > /* > > - * Now write the poison value to the kernel stack. Start from > > - * 'kstack_ptr' and move up till the new 'boundary'. We assume that > > - * the stack pointer doesn't change when we write poison. > > + * Now write the poison value to the kernel stack between 'erase_low' > > + * and 'erase_high'. We assume that the stack pointer doesn't change > > + * when we write poison. > > */ > > if (on_thread_stack()) > > - boundary = current_stack_pointer; > > + erase_high = current_stack_pointer; > > else > > - boundary = current_top_of_stack(); > > + erase_high = current_top_of_stack(); > > - while (kstack_ptr < boundary) { > > - *(unsigned long *)kstack_ptr = STACKLEAK_POISON; > > - kstack_ptr += sizeof(unsigned long); > > + while (erase_low < erase_high) { > > + *(unsigned long *)erase_low = STACKLEAK_POISON; > > + erase_low += sizeof(unsigned long); > > } > > /* Reset the 'lowest_stack' value for the next syscall */ >
On Tue, May 10, 2022 at 02:01:49PM +0100, Mark Rutland wrote: > On Sun, May 08, 2022 at 11:49:46PM +0300, Alexander Popov wrote: > > On 27.04.2022 20:31, Mark Rutland wrote: > > > The logic within __stackleak_erase() can be a little hard to follow, as > > > `boundary` switches from being the low bound to the high bound mid way > > > through the function, and `kstack_ptr` is used to represent the start of > > > the region to erase while `boundary` represents the end of the region to > > > erase. > > > > > > Make this a little clearer by consistently using clearer variable names. > > > The `boundary` variable is removed, the bounds of the region to erase > > > are described by `erase_low` and `erase_high`, and bounds of the task > > > stack are described by `task_stack_low` and `task_stck_high`. > > > > A typo here in `task_stck_high`. > > Ah; whoops. No worries; I fixed this when I took the patch. > > That was also the main reason why I reused the 'boundary' variable: I wanted > > the compiler to allocate it in the register and I avoided creating many > > local variables. > > > > Mark, did your refactoring make the compiler allocate local variables on the > > stack instead of the registers? > > Considering the whole series, testing with GCC 11.1.0: > > * On arm64: > before: stackleak_erase() uses 48 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > Note: this is entirely due to patch 1; arm64 has enough GPRs that it > doesn't need to use the stack. > > * On x86_64: > before: stackleak_erase() uses 0 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > * On i386 > before: stackleak_erase() uses 8 bytes of stach > after: stackleak_erase() uses 16 bytes of stack > > The i386 case isn't ideal, but given that those bytes will easily be used by > the entry triage code before getting to any syscall handling, I don't believe > that's an issue in practice. I am biased and totally fine with choosing a solution where 64-bit improvement comes at a 32-bit cost.
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 24b7cf01b2972..d5f684dc0a2d9 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -73,40 +73,38 @@ late_initcall(stackleak_sysctls_init); static __always_inline void __stackleak_erase(void) { const unsigned long task_stack_low = stackleak_task_low_bound(current); - - /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */ - unsigned long kstack_ptr = current->lowest_stack; - unsigned long boundary = task_stack_low; + unsigned long erase_low = current->lowest_stack; + unsigned long erase_high; unsigned int poison_count = 0; const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); /* Search for the poison value in the kernel stack */ - while (kstack_ptr > boundary && poison_count <= depth) { - if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON) + while (erase_low > task_stack_low && poison_count <= depth) { + if (*(unsigned long *)erase_low == STACKLEAK_POISON) poison_count++; else poison_count = 0; - kstack_ptr -= sizeof(unsigned long); + erase_low -= sizeof(unsigned long); } #ifdef CONFIG_STACKLEAK_METRICS - current->prev_lowest_stack = kstack_ptr; + current->prev_lowest_stack = erase_low; #endif /* - * Now write the poison value to the kernel stack. Start from - * 'kstack_ptr' and move up till the new 'boundary'. We assume that - * the stack pointer doesn't change when we write poison. + * Now write the poison value to the kernel stack between 'erase_low' + * and 'erase_high'. We assume that the stack pointer doesn't change + * when we write poison. */ if (on_thread_stack()) - boundary = current_stack_pointer; + erase_high = current_stack_pointer; else - boundary = current_top_of_stack(); + erase_high = current_top_of_stack(); - while (kstack_ptr < boundary) { - *(unsigned long *)kstack_ptr = STACKLEAK_POISON; - kstack_ptr += sizeof(unsigned long); + while (erase_low < erase_high) { + *(unsigned long *)erase_low = STACKLEAK_POISON; + erase_low += sizeof(unsigned long); } /* Reset the 'lowest_stack' value for the next syscall */
The logic within __stackleak_erase() can be a little hard to follow, as `boundary` switches from being the low bound to the high bound mid way through the function, and `kstack_ptr` is used to represent the start of the region to erase while `boundary` represents the end of the region to erase. Make this a little clearer by consistently using clearer variable names. The `boundary` variable is removed, the bounds of the region to erase are described by `erase_low` and `erase_high`, and bounds of the task stack are described by `task_stack_low` and `task_stck_high`. As the same time, remove the comment above the variables, since it is unclear whether it's intended as rationale, a complaint, or a TODO, and is more confusing than helpful. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Kees Cook <keescook@chromium.org> --- kernel/stackleak.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)