Re: [PATCH v18 24/25] x86/cet/shstk: Add arch_prctl functions for shadow stack

["Yu, Yu-cheng" <yu-cheng.yu@intel.com>]

On 1/29/2021 9:07 AM, Dave Hansen wrote:
> On 1/27/21 1:25 PM, Yu-cheng Yu wrote:
>> arch_prctl(ARCH_X86_CET_STATUS, u64 *args)
>>      Get CET feature status.
>>
>>      The parameter 'args' is a pointer to a user buffer.  The kernel returns
>>      the following information:
>>
>>      *args = shadow stack/IBT status
>>      *(args + 1) = shadow stack base address
>>      *(args + 2) = shadow stack size
> 
> What's the deal for 32-bit binaries?  The in-kernel code looks 64-bit
> only, but I don't see anything restricting the interface to 64-bit.

Items in args are 64-bit.  A 32-bit binary uses the same interface, but 
uses only lower bits.  I will add that in the comments.

>> +static int copy_status_to_user(struct cet_status *cet, u64 arg2)
> 
> This has static scope, but it's still awfully generically named.  A cet_
> prefix would be nice.

I will add that.

>> +{
>> +	u64 buf[3] = {0, 0, 0};
>> +
>> +	if (cet->shstk_size) {
>> +		buf[0] |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
>> +		buf[1] = (u64)cet->shstk_base;
>> +		buf[2] = (u64)cet->shstk_size;
> 
> What's the casting for?

cet->shstk_base and cet->shstk_size are both 'unsigned long', not u64, 
so the cast.

>> +	}
>> +
>> +	return copy_to_user((u64 __user *)arg2, buf, sizeof(buf));
>> +}
>> +
>> +int prctl_cet(int option, u64 arg2)
>> +{
>> +	struct cet_status *cet;
>> +	unsigned int features;
>> +
>> +	/*
>> +	 * GLIBC's ENOTSUPP == EOPNOTSUPP == 95, and it does not recognize
>> +	 * the kernel's ENOTSUPP (524).  So return EOPNOTSUPP here.
>> +	 */
>> +	if (!IS_ENABLED(CONFIG_X86_CET))
>> +		return -EOPNOTSUPP;
> 
> Let's ignore glibc for a moment.  What error code *should* the kernel be
> returning here?  errno(3) says:
> 
>         EOPNOTSUPP      Operation not supported on socket (POSIX.1)
> ...
>         ENOTSUP         Operation not supported (POSIX.1)
> 

Yeah, other places in kernel use ENOTSUPP.  This seems to be out of 
line.  And since the issue is long-existing, applications already know 
how to deal with it.  I should have made that argument.  Change it to 
ENOTSUPP.

>> +	cet = &current->thread.cet;
>> +
>> +	if (option == ARCH_X86_CET_STATUS)
>> +		return copy_status_to_user(cet, arg2);
> 
> What's the point of doing copy_status_to_user() if the processor doesn't
> support CET?  In other words, shouldn't this be below the CPU feature check?

The thought was to tell the difference between the kernel itself does 
not support CET and the system does not have CET.  And, if the kernel 
supports it, show CET status of the thread.

> Also, please cast arg2 *here*.  It becomes a user pointer here, not at
> the copy_to_user().

I will fix it.

>> +	if (!static_cpu_has(X86_FEATURE_CET))
>> +		return -EOPNOTSUPP;
> 
> So, you went to the trouble of adding a disabled-features.h entry for
> this.  Why not just do:
> 
> 	if (cpu_feature_enabled(X86_FEATURE_CET))
> 		...
> 
> instead of the IS_ENABLED() check above?  That should get rid of one of
> these if's.
> 

Explained above.

>> +	switch (option) {
>> +	case ARCH_X86_CET_DISABLE:
>> +		if (cet->locked)
>> +			return -EPERM;
>> +
>> +		features = (unsigned int)arg2;
> 
> What's the purpose of this cast?
> 
>> +		if (features & ~GNU_PROPERTY_X86_FEATURE_1_VALID)
>> +			return -EINVAL;
>> +		if (features & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
>> +			cet_disable_shstk();
>> +		return 0;
> 
> This doesn't enforce that the high bits of arg2 be 0.  Shouldn't we call
> them reserved and enforce that they be 0?

Yes, the code already checks invalid bits.  We don't need the cast.

>> +	case ARCH_X86_CET_LOCK:
>> +		cet->locked = 1;
>> +		return 0;
> 
> This needs to check for and enforce that arg2==0.

Yes.

> 
>> +	default:
>> +		return -ENOSYS;
>> +	}
>> +}