Re: [PATCH 01/23] signal: Add an optional check for altstack size

[Bae, Chang Seok]

Hi Eric,

On Oct 22, 2021, at 08:20, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
> 
> "Chang S. Bae" <chang.seok.bae@xxxxxxxxx> writes:
> 
>> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> 
>> The upcoming support for dynamically enabled FPU features on x86 requires
>> an architecture specific sanity check and serialization of the store to
>> task::sas_ss_size. The check is required to ensure that:
>> 
>>   - Enabling of a dynamic feature, which changes the sigframe size fits
>>     into an enabled sigaltstack
>> 
>>   - Installing a too small sigaltstack after a dynamic feature has been
>>     added is not possible.
>> 
>> It needs serialization to prevent race conditions of all sorts in the
>> feature enable code as that has to walk the thread list of the process.
>> 
>> Add the infrastructure in form of a config option and provide empty stubs
>> for architectures which do not need that.
> 
> Last I looked Al Viro was doing a lot of work on sigframes, adding him
> to the cc.
> 
> 
> That said description in the patch is very sorely lacking.

Will update the changelog though, let me clarify what you pointed out here 
at first.

> First the reason for the new locking is not really explained, it talks
> about serialization but it does not talk about what is protected.
> Especially given that the signal delivery code already has to check if
> the signal frame on the stack when pushing a new signal I don't
> understand what the code is trying to prevent.

Later in this series, a new x86-specific prctl() is introduced so that an 
application may ask permission to use dynamic states. It means the sigframe 
size is also dynamic. Besides that, in the implemented mechanism [1]:

    "Task asks for permission for a facility and kernel checks whether that's
     supported. If supported it does:
    ...
    3) Validate that no task has a sigaltstack installed
       which is smaller than the resulting sigframe size
    ...
    "

The new sigframe size out of new permission is validated via each thread’s 
altstack size. Accessing each task::sas_ss_size can be racy with
sigaltstack(). So, it is protected by sighand lock. (3) looks like this in a
nutshell:

	spin_lock_irq(&current->sighand->siglock);
	...
	int validate_sigaltstack(unsigned int usize) {
		struct task_struct *thread, *leader = current->group_leader;
		unsigned long framesize = get_sigframe_size();
		...
		for_each_thread(leader, thread) {
			if (thread->sas_ss_size && thread->sas_ss_size < framesize)
			return -ENOSPC;
		}
		...
	}
	...
	spin_unlock_irq(&current->sighand->siglock);

> Second the reason that 2K is not enough mentioned.  The current value of
> MINSIGSTKSZ on x86 is 2K.

The MINSIGSTKSZ constant is 2K but this is already too small in x86 with 
AVX512. Increasing this might break binaries that were already compiled with 
the old value. They used to work because their sigalstack was never used.

> Third the issues with modifying the userspace ABI are not discussed.
> Frankly that is a pretty big consideration.  MINSIGSTKSZ is exported to
> userspace and userspace fundamentally needs to allocate the alternate
> signal frame.

Now, there is an effort to redefine MINSIGSTKSZ as a dynamic value.
The latest glibc v2.34 has this [2]:

    "Add _SC_MINSIGSTKSZ and _SC_SIGSTKSZ. When _DYNAMIC_STACK_SIZE_SOURCE
     or _GNU_SOURCE are defined, MINSIGSTKSZ and SIGSTKSZ are no longer 
     constant on Linux. MINSIGSTKSZ is redefined to sysconf(_SC_MINSIGSTKSZ)
     and SIGSTKSZ is redefined to sysconf (_SC_SIGSTKSZ). This supports
     dynamic sized register sets for modern architectural features like
     Arm SVE."

> Forth the sigframe size on x86 is already dynamic and is already
> computed by get_sigframe_size.

Also, the x86 kernel supports exporting it via the AT_MINSIGSTKSZ aux vector
[4] since v5.14. I had developed the code with H.J. who authored the glibc
code [3].

> So can we please please please have a better description of what
> is going on and the trade offs that are being made.

Okay, but I think the dynamic MINSIGSTKSZ is not what this patch does, no?
Maybe the task::sas_ss_size part needs more clarity though.

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20211021225527.10184-7-chang.seok.bae@xxxxxxxxx/
[2] https://sourceware.org/pipermail/libc-alpha/2021-August/129718.html
[3] https://sourceware.org/git/?p=glibc.git;a=commit;h=6c57d320484988e87e446e2e60ce42816bf51d53
[4] https://lore.kernel.org/lkml/20210518200320.17239-1-chang.seok.bae@xxxxxxxxx/