Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461207 > unrolled thread

[PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 17:20 +0200
      Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-12 19:40 +0200
        Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 21:20 +0200
    Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 22:50 +0200
      Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to  use the new unwinder Kees Cook <keescook@chromium.org> - 2016-08-12 23:00 +0200

#1461207 — [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

Convert arch_within_stack_frames() to use the new unwinder.

Boot tested with CONFIG_HARDENED_USERCOPY.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
index 96ce151..9d0913c 100644
--- a/arch/x86/lib/usercopy.c
+++ b/arch/x86/lib/usercopy.c
@@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
 			     const void * const stackend,
 			     const void *obj, unsigned long len)
 {
-	const void *frame = NULL;
-	const void *oldframe;
+	struct unwind_state state;
+	const void *frame, *oldframe;
+
+	unwind_start(&state, current, NULL, NULL);
+
+	if (!unwind_next_frame(&state))
+		return 0;
+
+	oldframe = unwind_get_stack_ptr(&state);
+
+	if (!unwind_next_frame(&state))
+		return 0;
+
+	frame = unwind_get_stack_ptr(&state);
 
-	oldframe = __builtin_frame_address(2);
-	if (oldframe)
-		frame = __builtin_frame_address(3);
 	/*
 	 * low ----------------------------------------------> high
 	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
@@ -71,8 +80,12 @@ int arch_within_stack_frames(const void * const stack,
 		 */
 		if (obj + len <= frame)
 			return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
+
+		if (!unwind_next_frame(&state))
+			return 0;
+
 		oldframe = frame;
-		frame = *(const void * const *)frame;
+		frame = unwind_get_stack_ptr(&state);
 	}
 	return -1;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1461276 — Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
> Convert arch_within_stack_frames() to use the new unwinder.
> 
> Boot tested with CONFIG_HARDENED_USERCOPY.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index 96ce151..9d0913c 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
>  			     const void * const stackend,
>  			     const void *obj, unsigned long len)
>  {
> -	const void *frame = NULL;
> -	const void *oldframe;
> +	struct unwind_state state;
> +	const void *frame, *oldframe;
> +
> +	unwind_start(&state, current, NULL, NULL);
> +
> +	if (!unwind_next_frame(&state))
> +		return 0;
> +
> +	oldframe = unwind_get_stack_ptr(&state);

Actually, I think this isn't quite right.  Now that the function isn't
inlined, this needs to unwind another frame to be equivalent to current
behavior.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1461339 — Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 12, 2016 at 8:17 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
>> Convert arch_within_stack_frames() to use the new unwinder.
>>
>> Boot tested with CONFIG_HARDENED_USERCOPY.
>>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> index 96ce151..9d0913c 100644
>> --- a/arch/x86/lib/usercopy.c
>> +++ b/arch/x86/lib/usercopy.c
>> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
>>                            const void * const stackend,
>>                            const void *obj, unsigned long len)
>>  {
>> -     const void *frame = NULL;
>> -     const void *oldframe;
>> +     struct unwind_state state;
>> +     const void *frame, *oldframe;
>> +
>> +     unwind_start(&state, current, NULL, NULL);
>> +
>> +     if (!unwind_next_frame(&state))
>> +             return 0;
>> +
>> +     oldframe = unwind_get_stack_ptr(&state);
>
> Actually, I think this isn't quite right.  Now that the function isn't
> inlined, this needs to unwind another frame to be equivalent to current
> behavior.

Yeah, that seems right. And IIUC, as long as this is wrapped in the
CONFIG_FRAME_POINTER check, this won't use the guessing unwinder,
right? (Which is how it should be.)

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1461429 — Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 12, 2016 at 10:38:31AM -0700, Kees Cook wrote:
> On Fri, Aug 12, 2016 at 8:17 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
> >> Convert arch_within_stack_frames() to use the new unwinder.
> >>
> >> Boot tested with CONFIG_HARDENED_USERCOPY.
> >>
> >> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >> ---
> >>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
> >>  1 file changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> >> index 96ce151..9d0913c 100644
> >> --- a/arch/x86/lib/usercopy.c
> >> +++ b/arch/x86/lib/usercopy.c
> >> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
> >>                            const void * const stackend,
> >>                            const void *obj, unsigned long len)
> >>  {
> >> -     const void *frame = NULL;
> >> -     const void *oldframe;
> >> +     struct unwind_state state;
> >> +     const void *frame, *oldframe;
> >> +
> >> +     unwind_start(&state, current, NULL, NULL);
> >> +
> >> +     if (!unwind_next_frame(&state))
> >> +             return 0;
> >> +
> >> +     oldframe = unwind_get_stack_ptr(&state);
> >
> > Actually, I think this isn't quite right.  Now that the function isn't
> > inlined, this needs to unwind another frame to be equivalent to current
> > behavior.
> 
> Yeah, that seems right. And IIUC, as long as this is wrapped in the
> CONFIG_FRAME_POINTER check, this won't use the guessing unwinder,
> right? (Which is how it should be.)

Right, only the frame pointer unwinder will be used here, thanks to the
CONFIG_FRAME_POINTER guard in thread_info.h.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1461453 — Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
> Convert arch_within_stack_frames() to use the new unwinder.
> 
> Boot tested with CONFIG_HARDENED_USERCOPY.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index 96ce151..9d0913c 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
>  			     const void * const stackend,
>  			     const void *obj, unsigned long len)
>  {
> -	const void *frame = NULL;
> -	const void *oldframe;
> +	struct unwind_state state;
> +	const void *frame, *oldframe;
> +
> +	unwind_start(&state, current, NULL, NULL);
> +
> +	if (!unwind_next_frame(&state))
> +		return 0;
> +
> +	oldframe = unwind_get_stack_ptr(&state);
> +
> +	if (!unwind_next_frame(&state))
> +		return 0;
> +
> +	frame = unwind_get_stack_ptr(&state);
>  
> -	oldframe = __builtin_frame_address(2);
> -	if (oldframe)
> -		frame = __builtin_frame_address(3);
>  	/*
>  	 * low ----------------------------------------------> high
>  	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> @@ -71,8 +80,12 @@ int arch_within_stack_frames(const void * const stack,
>  		 */
>  		if (obj + len <= frame)
>  			return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> +
> +		if (!unwind_next_frame(&state))
> +			return 0;

I think there's another issue here.  This return needs to be tweaked.
IIUC, if it reliably reaches the end of the stack without finding the
object, it should return -1, but if there's something wrong with the
frame pointers which prevents the unwinder from reaching the end of the
stack, it should return 0.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1461455 — Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

On Fri, Aug 12, 2016 at 1:41 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
>> Convert arch_within_stack_frames() to use the new unwinder.
>>
>> Boot tested with CONFIG_HARDENED_USERCOPY.
>>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> index 96ce151..9d0913c 100644
>> --- a/arch/x86/lib/usercopy.c
>> +++ b/arch/x86/lib/usercopy.c
>> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
>>                            const void * const stackend,
>>                            const void *obj, unsigned long len)
>>  {
>> -     const void *frame = NULL;
>> -     const void *oldframe;
>> +     struct unwind_state state;
>> +     const void *frame, *oldframe;
>> +
>> +     unwind_start(&state, current, NULL, NULL);
>> +
>> +     if (!unwind_next_frame(&state))
>> +             return 0;
>> +
>> +     oldframe = unwind_get_stack_ptr(&state);
>> +
>> +     if (!unwind_next_frame(&state))
>> +             return 0;
>> +
>> +     frame = unwind_get_stack_ptr(&state);
>>
>> -     oldframe = __builtin_frame_address(2);
>> -     if (oldframe)
>> -             frame = __builtin_frame_address(3);
>>       /*
>>        * low ----------------------------------------------> high
>>        * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> @@ -71,8 +80,12 @@ int arch_within_stack_frames(const void * const stack,
>>                */
>>               if (obj + len <= frame)
>>                       return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
>> +
>> +             if (!unwind_next_frame(&state))
>> +                     return 0;
>
> I think there's another issue here.  This return needs to be tweaked.
> IIUC, if it reliably reaches the end of the stack without finding the
> object, it should return -1, but if there's something wrong with the
> frame pointers which prevents the unwinder from reaching the end of the
> stack, it should return 0.

Ah, yes, good catch. The callers of this function should have already
determined if the address is outside the stack itself, so this should
only be called when we expect the contents to be somewhere in the
stack. If the unwinder can't find it, then that should be an error,
yes.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web