Re: [PATCH v12 2/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

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

Hello Ingo,

Thanks a lot for the review!
I agree with your points. I'll fix the series and return with v13.

There are some comments/questions below.

On 18.05.2018 09:53, Ingo Molnar wrote:
> * Alexander Popov <alex.popov@linux.com> wrote:
>> +	/*
>> +	 * It would be nice not to have p and boundary on the stack.
>> +	 * Setting the register specifier for them is the best we can do.
>> +	 */
>> +	register unsigned long p = current->thread.lowest_stack.val;
>> +	register unsigned long boundary = p & ~(THREAD_SIZE - 1);
> 
> Does the 'register' keyword actually have any effect on the generated code?

No, for gcc it doesn't give any effect for this code. I used it to show the
intention.

However, even if the compiler allocates 'p' and 'boundary' on stack, that will
not break the stack erasing.

Should I drop 'register'?

>> +	unsigned long poison = 0;
>> +	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
>> +							sizeof(unsigned long);
> 
> Please don't break lines in such an ugly fashion!

Ok. I'll make it look like that:

	const unsigned long check_depth =
			STACKLEAK_POISON_CHECK_DEPTH / sizeof(unsigned long);


> Also, 'poison' is a very weird name for something that looks like an index.
> 
> Plus since it's bound by "check_depth" is the 'unsigned long' justified,
> or could it be 32-bit?

Thanks, I can turn both 'check_depth' and 'poison' (which I'll rename) into
'unsigned int'.

>> +	/* Reset the lowest_stack value for the next syscall */
>> +	current->thread.lowest_stack.val = current_top_of_stack() - 256;
> 
> Magic, unexplained '256' literal.

Here I'm choosing the point from which we'll start the poison search on the next
syscall in case 'lowest_stack' is not updated in track_stack().

Would you like if I use "current_top_of_stack() - THREAD_SIZE / 64" ?

Best regards,
Alexander