On Mon, Aug 19, 2024 at 05:33:24PM +0100, Mark Brown wrote:

On Mon, Aug 19, 2024 at 10:10:36AM +0100, Catalin Marinas wrote:

quoted

On Thu, Aug 01, 2024 at 01:06:40PM +0100, Mark Brown wrote:

quoted

+	if (system_supports_gcs() && (vm_flags & VM_SHADOW_STACK)) {
+		/*
+		 * An executable GCS isn't a good idea, and the mm
+		 * core can't cope with a shared GCS.
+		 */
+		if (vm_flags & (VM_EXEC | VM_ARM64_BTI | VM_SHARED))
+			return false;
+	}

quoted

I wonder whether we should clear VM_MAYEXEC early on during the vma
creation. This way the mprotect() case will be handled in the core code.
At a quick look, do_mmap() seems to always set VM_MAYEXEC but discard it
for non-executable file mmap. Last time I looked (when doing MTE) there
wasn't a way for the arch code to clear specific VM_* flags, only to
validate them. But I think we should just clear VM_MAYEXEC and also
return an error for VM_EXEC in the core do_mmap() if VM_SHADOW_STACK. It
would cover the other architectures doing shadow stacks.

Yes, I think adding something generic would make sense here.  That feels
like a cleanup which could be split out?

It can be done separately. It doesn't look like x86 has such checks.
Adding it generically would be a slight ABI tightening but I doubt it
matters, no sane software would use an executable shadow stack.

quoted

Regarding VM_SHARED, how do we even end up with this via the
map_shadow_stack() syscall? I can't see how one can pass MAP_SHARED to
do_mmap() on this path. I'm fine with a VM_WARN_ON() if you want the
check (and there's no way a user can trigger it).

It's just a defenesive programming thing, I'm not aware of any way in
which it should be possible to trigger this.

quoted

Is there any arch restriction with setting BTI and GCS? It doesn't make
sense but curious if it matters. We block the exec permission anyway
(unless the BTI pages moved to PIE as well, I don't remember).

As you say BTI should be meaningless for a non-executable page like GCS,
I'm not aware of any way in which it matters.  BTI is separate to PIE.

My thoughts were whether we can get rid of this hunk entirely by
handling it in the core code. We'd allow BTI if one wants such useless
combination but clear VM_MAYEXEC in the core code (and ignore VM_SHARED
since you can't set it anyway).

-- 
Catalin