Re: [RFC PATCH v2 3/5] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>]

On Wed, Jun 05, 2019 at 07:11:43PM -0700, Sean Christopherson wrote:
> +		goto out;
> +	}
> +
> +	/*
> +	 * Query VM_MAYEXEC as an indirect path_noexec() check (see do_mmap()),
> +	 * but with some future proofing against other cases that may deny
> +	 * execute permissions.
> +	 */
> +	if (!(vma->vm_flags & VM_MAYEXEC)) {
> +		ret = -EACCES;
> +		goto out;
> +	}
> +
> +	if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
> +		ret = -EFAULT;
> +	else
> +		ret = 0;
> +
> +out:
> +	up_read(&current->mm->mmap_sem);
> +
> +	return ret;
> +}

I would suggest to express the above instead like this for clarity
and consistency:

		goto err_map_sem;
	}

	/* Query VM_MAYEXEC as an indirect path_noexec() check
	 * (see do_mmap()).
	 */
	if (!(vma->vm_flags & VM_MAYEXEC)) {
		ret = -EACCES;
		goto err_mmap_sem;
	}

	if (copy_from_user(dst, (void __user *)src, PAGE_SIZE)) {
		ret = -EFAULT;
		goto err_mmap_sem;
	}

	return 0;

err_mmap_sem:
	up_read(&current->mm->mmap_sem);
	return ret;
}

The comment about future proofing is unnecessary.

/Jarkk