From: "Alexey Gladkov (Intel)" legion@kernel.org
TDX only supports kernel-initiated MMIO operations. The handle_mmio() function checks if the #VE exception occurred in the kernel and rejects the operation if it did not.
However, userspace can deceive the kernel into performing MMIO on its behalf. For example, if userspace can point a syscall to an MMIO address, syscall does get_user() or put_user() on it, triggering MMIO #VE. The kernel will treat the #VE as in-kernel MMIO.
Ensure that the target MMIO address is within the kernel before decoding instruction.
Cc: stable@vger.kernel.org Signed-off-by: Alexey Gladkov (Intel) legion@kernel.org --- arch/x86/coco/tdx/tdx.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 078e2bac2553..c90d2fdb5fc4 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -405,6 +405,11 @@ static bool mmio_write(int size, unsigned long addr, unsigned long val) EPT_WRITE, addr, val); }
+static inline bool is_kernel_addr(unsigned long addr) +{ + return (long)addr < 0; +} + static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) { unsigned long *reg, val, vaddr; @@ -434,6 +439,11 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) return -EINVAL; }
+ if (!user_mode(regs) && !is_kernel_addr(ve->gla)) { + WARN_ONCE(1, "Access to userspace address is not supported"); + return -EINVAL; + } + /* * Reject EPT violation #VEs that split pages. *
On 9/6/24 04:49, Alexey Gladkov wrote:
+static inline bool is_kernel_addr(unsigned long addr) +{
- return (long)addr < 0;
+}
static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) { unsigned long *reg, val, vaddr; @@ -434,6 +439,11 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) return -EINVAL; }
- if (!user_mode(regs) && !is_kernel_addr(ve->gla)) {
WARN_ONCE(1, "Access to userspace address is not supported");return -EINVAL;- }
Should we really be open-coding a "is_kernel_addr" check? I mean, TASK_SIZE_MAX is there for a reason. While I doubt we'd ever change the positive vs. negative address space convention on 64-bit, I don't see a good reason to write a 64-bit x86-specific is_kernel_addr() when a more generic, portable and conventional idiom would do.
So, please use either a:
addr < TASK_SIZE_MAX
check, or use fault_in_kernel_space() directly.
On Tue, Sep 10, 2024 at 12:54:19PM -0700, Dave Hansen wrote:
On 9/6/24 04:49, Alexey Gladkov wrote:
+static inline bool is_kernel_addr(unsigned long addr) +{
- return (long)addr < 0;
+}
static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) { unsigned long *reg, val, vaddr; @@ -434,6 +439,11 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) return -EINVAL; }
- if (!user_mode(regs) && !is_kernel_addr(ve->gla)) {
WARN_ONCE(1, "Access to userspace address is not supported");return -EINVAL;- }
Should we really be open-coding a "is_kernel_addr" check? I mean, TASK_SIZE_MAX is there for a reason. While I doubt we'd ever change the positive vs. negative address space convention on 64-bit, I don't see a good reason to write a 64-bit x86-specific is_kernel_addr() when a more generic, portable and conventional idiom would do.
I took arch/x86/events/perf_event.h:1262 as an example. There is no special reason in its own function.
So, please use either a:
addr < TASK_SIZE_MAX
check, or use fault_in_kernel_space() directly.
I'll use fault_in_kernel_space() since SEV uses it. Thanks.
On Wed, Sep 11, 2024 at 02:08:47PM +0200, Alexey Gladkov wrote:
On Tue, Sep 10, 2024 at 12:54:19PM -0700, Dave Hansen wrote:
On 9/6/24 04:49, Alexey Gladkov wrote:
+static inline bool is_kernel_addr(unsigned long addr) +{
- return (long)addr < 0;
+}
static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) { unsigned long *reg, val, vaddr; @@ -434,6 +439,11 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve) return -EINVAL; }
- if (!user_mode(regs) && !is_kernel_addr(ve->gla)) {
WARN_ONCE(1, "Access to userspace address is not supported");return -EINVAL;- }
Should we really be open-coding a "is_kernel_addr" check? I mean, TASK_SIZE_MAX is there for a reason. While I doubt we'd ever change the positive vs. negative address space convention on 64-bit, I don't see a good reason to write a 64-bit x86-specific is_kernel_addr() when a more generic, portable and conventional idiom would do.
I took arch/x86/events/perf_event.h:1262 as an example. There is no special reason in its own function.
So, please use either a:
addr < TASK_SIZE_MAX
check, or use fault_in_kernel_space() directly.
I'll use fault_in_kernel_space() since SEV uses it. Thanks.
Also user_mode() check is redundant until later in the patchset. Move it to the patch that allows userspace MMIO.
On Fri, Sep 06, 2024 at 01:49:59PM +0200, Alexey Gladkov wrote:
From: "Alexey Gladkov (Intel)" legion@kernel.org
TDX only supports kernel-initiated MMIO operations. The handle_mmio() function checks if the #VE exception occurred in the kernel and rejects the operation if it did not.
However, userspace can deceive the kernel into performing MMIO on its behalf. For example, if userspace can point a syscall to an MMIO address, syscall does get_user() or put_user() on it, triggering MMIO #VE. The kernel will treat the #VE as in-kernel MMIO.
Ensure that the target MMIO address is within the kernel before decoding instruction.
Cc: stable@vger.kernel.org Signed-off-by: Alexey Gladkov (Intel) legion@kernel.org
Reviewed-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com
linux-stable-mirror@lists.linaro.org
-
Alexey Gladkov -
Dave Hansen -
Kirill A. Shutemov