On 1/29/2021 9:07 AM, Dave Hansen wrote:

On 1/27/21 1:25 PM, Yu-cheng Yu wrote:

quoted

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.

quoted

+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.

quoted

+{
+	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.

quoted

+	}
+
+	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.

quoted

+	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.

quoted

+	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.

quoted

+	switch (option) {
+	case ARCH_X86_CET_DISABLE:
+		if (cet->locked)
+			return -EPERM;
+
+		features = (unsigned int)arg2;

What's the purpose of this cast?

quoted

+		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.

quoted

+	case ARCH_X86_CET_LOCK:
+		cet->locked = 1;
+		return 0;

This needs to check for and enforce that arg2==0.

Yes.

quoted

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