Re: [PATCH v2 07/13] stackleak: rework poison scanning

[Alexander Popov <alex.popov@linux.com>]

On 31.05.2022 21:13, Kees Cook wrote:
> On Fri, May 27, 2022 at 02:25:12AM +0300, Alexander Popov wrote:
>> On 24.05.2022 16:31, Mark Rutland wrote:
>>> [...]
>>> It's also worth noting that `noinstr` code will also not be instrumented
>>> regardless of frame size -- we might want some build-time assertion for those.
>>
>> I developed a trick that shows noinstr functions that stackleak would like to instrument:
>>
>> diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c
>> index 42f0252ee2a4..6db748d44957 100644
>> --- a/scripts/gcc-plugins/stackleak_plugin.c
>> +++ b/scripts/gcc-plugins/stackleak_plugin.c
>> @@ -397,6 +397,9 @@ static unsigned int stackleak_cleanup_execute(void)
>>   	const char *fn = DECL_NAME_POINTER(current_function_decl);
>>   	bool removed = false;
>>
>> +	if (verbose)
>> +		fprintf(stderr, "stackleak: I see noinstr function %s()\n", fn);
>> +
>>   	/*
>>   	 * Leave stack tracking in functions that call alloca().
>>   	 * Additional case:
>> @@ -464,12 +467,12 @@ static bool stackleak_gate(void)
>>   		if (STRING_EQUAL(section, ".meminit.text"))
>>   			return false;
>>   		if (STRING_EQUAL(section, ".noinstr.text"))
>> -			return false;
>> +			return true;
>>   		if (STRING_EQUAL(section, ".entry.text"))
>>   			return false;
>>   	}
>>
>> -	return track_frame_size >= 0;
>> +	return false;
>>   }
>>
>>   /* Build the function declaration for stackleak_track_stack() */
>> @@ -589,8 +592,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
>>   				build_for_x86 = true;
>>   		} else if (!strcmp(argv[i].key, "disable")) {
>>   			disable = true;
>> -		} else if (!strcmp(argv[i].key, "verbose")) {
>> -			verbose = true;
>>   		} else {
>>   			error(G_("unknown option '-fplugin-arg-%s-%s'"),
>>   					plugin_name, argv[i].key);
>> @@ -598,6 +599,8 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
>>   		}
>>   	}
>>
>> +	verbose = true;
>> +
>>   	if (disable) {
>>   		if (verbose)
>>   			fprintf(stderr, "stackleak: disabled for this translation unit\n");
>>
>>
>> Building defconfig for x86_64 gives this:
>>
>> stackleak: I see noinstr function __do_fast_syscall_32()
>> stackleak: instrument __do_fast_syscall_32(): calls_alloca
>> --
>> stackleak: I see noinstr function do_syscall_64()
>> stackleak: instrument do_syscall_64(): calls_alloca
>> --
>> stackleak: I see noinstr function do_int80_syscall_32()
>> stackleak: instrument do_int80_syscall_32(): calls_alloca
> 
> As you say, these are from RANDOMIZE_KSTACK_OFFSET, and are around
> bounds-checked, and should already be getting wiped since these will
> call into deeper (non-noinst) functions.

Kees, it crossed my mind that for correct stack erasing the kernel with 
RANDOMIZE_KSTACK_OFFSET needs at least one stackleak_track_stack() call during 
the syscall handling.

Otherwise current->lowest_stack would point to the stack address where no stack 
frame was placed because of alloca with random size.

Am I right?

How about calling stackleak_track_stack() explicitly after the kernel stack 
randomization?


>> stackleak: I see noinstr function do_machine_check()
>> stackleak: instrument do_machine_check()
>> --
>> stackleak: I see noinstr function exc_general_protection()
>> stackleak: instrument exc_general_protection()
>> --
>> stackleak: I see noinstr function fixup_bad_iret()
>> stackleak: instrument fixup_bad_iret()
>>
>>
>> The cases with calls_alloca are caused by CONFIG_RANDOMIZE_KSTACK_OFFSET=y.
>> Kees knows about that peculiarity.
>>
>> Other cases are noinstr functions with large stack frame:
>> do_machine_check(), exc_general_protection(), and fixup_bad_iret().
>>
>> I think adding a build-time assertion is not possible, since it would break
>> building the kernel.
> 
> Do these functions share the syscall behavior of always calling into
> non-noinst functions that _do_ have stack depth instrumentation?

This is a right question.

I can't say for sure, but it looks like do_machine_check(), 
exc_general_protection() and fixup_bad_iret() do some low-level exception/trap 
handling and don't affect syscall handling. Do you agree?

>> [...]
>>> In security/Kconfig.hardening we have:
>>>
>>> | config STACKLEAK_TRACK_MIN_SIZE
>>> |         int "Minimum stack frame size of functions tracked by STACKLEAK"
>>> |         default 100
>>> |         range 0 4096
>>> |         depends on GCC_PLUGIN_STACKLEAK
>>> |         help
>>> |           The STACKLEAK gcc plugin instruments the kernel code for tracking
>>> |           the lowest border of the kernel stack (and for some other purposes).
>>> |           It inserts the stackleak_track_stack() call for the functions with
>>> |           a stack frame size greater than or equal to this parameter.
>>> |           If unsure, leave the default value 100.
>>>
>>> ... where the vast majority of that range is going to lead to a BUILD_BUG().
>>
>> Honestly, I don't like the idea of having the STACKLEAK_TRACK_MIN_SIZE option in the Kconfig.
>>
>> I was forced by the maintainers to introduce it when I was working on the stackleak patchset.
>>
>> How about dropping CONFIG_STACKLEAK_TRACK_MIN_SIZE from Kconfig?
>>
>> That would also allow to drop this build-time assertion.
> 
> Should this be arch-specific? (i.e. just make it a per-arch Kconfig
> default, instead of user-selectable into weird values?)

I don't think CONFIG_STACKLEAK_TRACK_MIN_SIZE is arch-specific, since 
STACKLEAK_SEARCH_DEPTH is the same for all architectures that support stackleak.

Best regards,
Alexander