| Message ID | 20220309164954.23751-2-mdoucha@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] KVM test infrastructure | expand |
Hi Martin, Martin Doucha <mdoucha@suse.cz> writes: > Signed-off-by: Martin Doucha <mdoucha@suse.cz> > --- > runtest/kvm | 1 + > testcases/kernel/kvm/.gitignore | 1 + > testcases/kernel/kvm/kvm_pagefault01.c | 235 +++++++++++++++++++++++++ > 3 files changed, 237 insertions(+) > create mode 100644 runtest/kvm > create mode 100644 testcases/kernel/kvm/.gitignore > create mode 100644 testcases/kernel/kvm/kvm_pagefault01.c > > diff --git a/runtest/kvm b/runtest/kvm > new file mode 100644 > index 000000000..16e7c07ff > --- /dev/null > +++ b/runtest/kvm > @@ -0,0 +1 @@ > +kvm_pagefault01 kvm_pagefault01 > diff --git a/testcases/kernel/kvm/.gitignore b/testcases/kernel/kvm/.gitignore > new file mode 100644 > index 000000000..349260359 > --- /dev/null > +++ b/testcases/kernel/kvm/.gitignore > @@ -0,0 +1 @@ > +/kvm_pagefault01 > diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c > new file mode 100644 > index 000000000..e7d957f3e > --- /dev/null > +++ b/testcases/kernel/kvm/kvm_pagefault01.c > @@ -0,0 +1,235 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2020 SUSE LLC > + * Author: Nicolai Stange <nstange@suse.de> > + * LTP port: Martin Doucha <mdoucha@suse.cz> > + */ > + > +/*\ > + * CVE 2021-38198 > + * > + * Check that x86_64 KVM correctly enforces (lack of) write permissions > + * in 4-level and 5-level memory page table mode. Missing page faults fixed in: > + * > + * commit b1bd5cba3306691c771d558e94baa73e8b0b96b7 > + * Author: Lai Jiangshan <laijs@linux.alibaba.com> > + * Date: Thu Jun 3 13:24:55 2021 +0800 > + * > + * KVM: X86: MMU: Use the correct inherited permissions to get shadow page > + */ > + > +#include "kvm_test.h" > + > +#ifdef COMPILE_PAYLOAD Minor nit; I'd prefer guest to payload. > +#ifdef __x86_64__ > + > +#include "kvm_x86.h" > + > +#define PTE_BITMASK 0x1ff > +#define PAGESIZE 0x1000 > + > +int handle_page_fault(void *userdata, struct kvm_interrupt_frame *ifrm, > + unsigned long errcode) > +{ > + struct kvm_cregs buf; > + > + kvm_read_cregs(&buf); > + > + /* Check that the page fault was caused by write to *readonly below */ > + if (buf.cr2 == (uintptr_t)userdata) { > + tst_res(TPASS, "KVM enforces memory write permissions"); > + kvm_exit(); > + } > + > + /* Unexpected page fault, fall back to default handler */ > + return 0; > +} > + > +void main(void) > +{ > + uintptr_t tmp; > + struct page_table_entry_pae *subpte, *pte = kvm_pagetable; > + int val, *writable, *readonly, *cacher1, *cacher2; > + > + if (!(kvm_rdmsr(MSR_EFER) & EFER_LMA)) > + tst_brk(TBROK, "Bootstrap did not enable 64bit paging"); > + > + /* > + * Find the first page table entry which branches. This entry was > + * configured by bootstrap as follows: > + * 0x00000000 - 0x3fffffff in pte[0] (identity mapped) > + * 0x40000000 - 0x7fffffff in pte[1] (identity mapped) > + * 0x80000000 - 0xbfffffff in pte[2] (unmapped) > + * 0xc0000000 - 0xffffffff in pte[3] (only last page identity mapped) > + */ > + while (!pte[1].present) { > + tmp = kvm_get_page_address_pae(pte); > + pte = (struct page_table_entry_pae *)tmp; > + } > + > + /* > + * Setup mapping above the 32bit address space. The test needs two > + * different unused 1GB chunks of address space. Remapping part of > + * the lower 4GB address space would make it harder to reproduce > + * the bug because any memory access in the same 1GB chunk (even > + * fetching instructions by the CPU) could evict entries from page > + * table cache and force the bypassable write permission check > + * to happen even on buggy kernels. > + * > + * Allocate 3 pages for page table + 2 pages for data > + */ > + writable = tst_heap_alloc_aligned(5 * PAGESIZE, PAGESIZE); > + memset(writable, 0, 5 * PAGESIZE); > + tmp = (uintptr_t)writable; > + pte[4].address = tmp >> 12; > + pte[4].user_access = 1; > + pte[4].writable = 1; > + pte[4].present = 1; > + pte[5] = pte[4]; > + pte[5].writable = 0; > + > + subpte = (struct page_table_entry_pae *)tmp; > + subpte[0].address = (tmp + PAGESIZE) >> 12; > + subpte[0].user_access = 1; > + subpte[0].writable = 0; > + subpte[0].present = 1; > + subpte[1].address = (tmp + 2 * PAGESIZE) >> 12; > + subpte[1].user_access = 1; > + subpte[1].writable = 1; > + subpte[1].present = 1; > + > + subpte = (struct page_table_entry_pae *)(tmp + PAGESIZE); > + subpte[0].address = (tmp + 3 * PAGESIZE) >> 12; > + subpte[0].user_access = 1; > + subpte[0].writable = 1; > + subpte[0].present = 1; > + > + subpte = (struct page_table_entry_pae *)(tmp + 2 * PAGESIZE); > + subpte[0].address = (tmp + 4 * PAGESIZE) >> 12; > + subpte[0].user_access = 1; > + subpte[0].writable = 1; > + subpte[0].present = 1; > + > + /* Create pointers into the new mapping */ > + cacher1 = (int *)0x100000000ULL; > + writable = (int *)0x100200000ULL; > + cacher2 = (int *)0x140000000ULL; > + readonly = (int *)0x140200000ULL; > + tst_set_interrupt_callback(INTR_PAGE_FAULT, handle_page_fault, > + readonly); > + > + /* Fill page table cache */ > + val = *cacher1; > + *writable = val; > + val = *cacher2; > + > + /* Trigger page fault (unless the kernel is vulnerable) */ > + *readonly = val; > + > + /* This line should be unreachable */ > + tst_res(TFAIL, "Write to read-only address did not page fault"); > +} > + > +#else /* __x86_64__ */ > +TST_TEST_TCONF("Test supported only on x86_64"); > +#endif /* __x86_64__ */ > + > +#else /* COMPILE_PAYLOAD */ > + > +#include <ctype.h> > +#include <stdio.h> > +#include <unistd.h> > +#include "tst_module.h" > + > +#define TDP_MMU_SYSFILE "/sys/module/kvm/parameters/tdp_mmu" > +#define TDP_AMD_SYSFILE "/sys/module/kvm_amd/parameters/npt" > +#define TDP_INTEL_SYSFILE "/sys/module/kvm_intel/parameters/ept" > + > +#define BUF_SIZE 64 > + > +static int read_bool_sys_param(const char *filename) > +{ > + char buf[BUF_SIZE]; > + int i, fd, ret; > + > + fd = open(filename, O_RDONLY); > + > + if (fd < 0) > + return -1; > + > + ret = read(fd, buf, BUF_SIZE - 1); > + SAFE_CLOSE(fd); > + > + if (ret < 1) > + return -1; > + > + buf[ret] = '\0'; > + > + for (i = 0; buf[i] && !isspace(buf[i]); i++) > + ; > + > + buf[i] = '\0'; > + > + if (isdigit(buf[0])) { > + sscanf(buf, "%d", &ret); checkpatch complains that the return value is not checked. Also it wants you to use tst_parse_int. > + return ret; > + } > + > + if (!strcasecmp(buf, "N")) > + return 0; > + > + /* Assume that any other value than 0 or N means the param is enabled */ > + return 1; > +} > + > +static void reload_module(const char *module, char *arg) > +{ > + const char *const argv[] = {"modprobe", module, arg, NULL}; > + > + tst_res(TINFO, "Reloading module %s with parameter %s", module, arg); > + tst_module_unload(module); > + tst_cmd(argv, NULL, NULL, 0); > +} > + > +static void disable_tdp(void) > +{ > + if (!access(TDP_MMU_SYSFILE, F_OK)) { > + /* FIXME: Is this sufficient to disable TDP? */ What happens if this doesn't work and TDP is enabled? I seem to have it enabled and the test still passes even if I comment out the call to disable_tdp. I'm wondering whether it will be easy to tell if a test failure is due to TDP or if it can result in silent false negatives? > + SAFE_FILE_PRINTF(TDP_MMU_SYSFILE, "0"); > + return; > + } > + > + if (read_bool_sys_param(TDP_AMD_SYSFILE) > 0) > + reload_module("kvm_amd", "npt=0"); > + > + if (read_bool_sys_param(TDP_INTEL_SYSFILE) > 0) > + reload_module("kvm_intel", "ept=0"); > +} > + > +static void setup(void) > +{ > + disable_tdp(); > + tst_kvm_setup(); > +} > + > +static struct tst_test test = { > + .test_all = tst_kvm_run, > + .setup = setup, > + .cleanup = tst_kvm_cleanup, > + .needs_root = 1, > + .save_restore = (const char *const []) { > + "?/sys/module/kvm/parameters/tdp_mmu", This needs updating to use struct tst_path_val. > + NULL > + }, > + .supported_archs = (const char *const []) { > + "x86_64", > + NULL > + }, > + .tags = (struct tst_tag[]){ > + {"linux-git", "b1bd5cba3306"}, > + {"CVE", "2021-38198"}, > + {} > + } > +}; > + > +#endif /* COMPILE_PAYLOAD */ > -- > 2.35.1
On 15. 03. 22 15:19, Richard Palethorpe wrote: > Hi Martin, > > Martin Doucha <mdoucha@suse.cz> writes: >> + if (isdigit(buf[0])) { >> + sscanf(buf, "%d", &ret); > > checkpatch complains that the return value is not checked. Also it wants > you to use tst_parse_int. if (isdigit()) above ensures that the return value does not matter. But I'll change it to tst_parse_int() in v2. >> +static void disable_tdp(void) >> +{ >> + if (!access(TDP_MMU_SYSFILE, F_OK)) { >> + /* FIXME: Is this sufficient to disable TDP? */ > > What happens if this doesn't work and TDP is enabled? I seem to have it > enabled and the test still passes even if I comment out the call > to disable_tdp. > > I'm wondering whether it will be easy to tell if a test failure is due > to TDP or if it can result in silent false negatives? AFAIK, the kernel bug was fixed before the tdp_mmu sysfile was introduced. So somebody will have to revert the fix and run the test on custom kernel to answer that question. The bug is not reproducible with TDP enabled. >> + .save_restore = (const char *const []) { >> + "?/sys/module/kvm/parameters/tdp_mmu", > > This needs updating to use struct tst_path_val. Yes, the patch was sent before the .save_restore structure was changed.
Hello, Martin Doucha <mdoucha@suse.cz> writes: > On 15. 03. 22 15:19, Richard Palethorpe wrote: >> Hi Martin, >> >> Martin Doucha <mdoucha@suse.cz> writes: >>> + if (isdigit(buf[0])) { >>> + sscanf(buf, "%d", &ret); >> >> checkpatch complains that the return value is not checked. Also it wants >> you to use tst_parse_int. > > if (isdigit()) above ensures that the return value does not matter. But > I'll change it to tst_parse_int() in v2. > >>> +static void disable_tdp(void) >>> +{ >>> + if (!access(TDP_MMU_SYSFILE, F_OK)) { >>> + /* FIXME: Is this sufficient to disable TDP? */ >> >> What happens if this doesn't work and TDP is enabled? I seem to have it >> enabled and the test still passes even if I comment out the call >> to disable_tdp. >> >> I'm wondering whether it will be easy to tell if a test failure is due >> to TDP or if it can result in silent false negatives? > > AFAIK, the kernel bug was fixed before the tdp_mmu sysfile was > introduced. Ah I see TDP is very new https://lwn.net/Articles/832835/ > So somebody will have to revert the fix and run the test on > custom kernel to answer that question. Did you reproduce the bug? I could try this next week. > > The bug is not reproducible with TDP enabled. > >>> + .save_restore = (const char *const []) { >>> + "?/sys/module/kvm/parameters/tdp_mmu", >> >> This needs updating to use struct tst_path_val. > > Yes, the patch was sent before the .save_restore structure was changed.
On 15. 03. 22 16:44, Richard Palethorpe wrote: > Martin Doucha <mdoucha@suse.cz> writes: >> So somebody will have to revert the fix and run the test on >> custom kernel to answer that question. > > Did you reproduce the bug? > > I could try this next week. Yes, I've verified the reproducer on older SLE-12SP5 and SLE-15SP3 kernels. But none of them currently have the tdp_mmu sysfile so the test had to use the Intel/AMD KVM module reload fallback.
Hello Martin, Martin Doucha <mdoucha@suse.cz> writes: > On 15. 03. 22 16:44, Richard Palethorpe wrote: >> Martin Doucha <mdoucha@suse.cz> writes: >>> So somebody will have to revert the fix and run the test on >>> custom kernel to answer that question. >> >> Did you reproduce the bug? >> >> I could try this next week. > > Yes, I've verified the reproducer on older SLE-12SP5 and SLE-15SP3 > kernels. But none of them currently have the tdp_mmu sysfile so the test > had to use the Intel/AMD KVM module reload fallback. I see, in that case I don't think it's important to check it.
On 17. 03. 22 8:35, Richard Palethorpe wrote: > Martin Doucha <mdoucha@suse.cz> writes: >> Yes, I've verified the reproducer on older SLE-12SP5 and SLE-15SP3 >> kernels. But none of them currently have the tdp_mmu sysfile so the test >> had to use the Intel/AMD KVM module reload fallback. > > I see, in that case I don't think it's important to check it. It's still useful to test the reproducer on a known broken kernel with TDP disabled via tdp_mmu sysfile instead of KVM module reload. That's the control flow that'll be used on all future kernels so we should check whether it really works.
diff --git a/runtest/kvm b/runtest/kvm new file mode 100644 index 000000000..16e7c07ff --- /dev/null +++ b/runtest/kvm @@ -0,0 +1 @@ +kvm_pagefault01 kvm_pagefault01 diff --git a/testcases/kernel/kvm/.gitignore b/testcases/kernel/kvm/.gitignore new file mode 100644 index 000000000..349260359 --- /dev/null +++ b/testcases/kernel/kvm/.gitignore @@ -0,0 +1 @@ +/kvm_pagefault01 diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c new file mode 100644 index 000000000..e7d957f3e --- /dev/null +++ b/testcases/kernel/kvm/kvm_pagefault01.c @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 SUSE LLC + * Author: Nicolai Stange <nstange@suse.de> + * LTP port: Martin Doucha <mdoucha@suse.cz> + */ + +/*\ + * CVE 2021-38198 + * + * Check that x86_64 KVM correctly enforces (lack of) write permissions + * in 4-level and 5-level memory page table mode. Missing page faults fixed in: + * + * commit b1bd5cba3306691c771d558e94baa73e8b0b96b7 + * Author: Lai Jiangshan <laijs@linux.alibaba.com> + * Date: Thu Jun 3 13:24:55 2021 +0800 + * + * KVM: X86: MMU: Use the correct inherited permissions to get shadow page + */ + +#include "kvm_test.h" + +#ifdef COMPILE_PAYLOAD +#ifdef __x86_64__ + +#include "kvm_x86.h" + +#define PTE_BITMASK 0x1ff +#define PAGESIZE 0x1000 + +int handle_page_fault(void *userdata, struct kvm_interrupt_frame *ifrm, + unsigned long errcode) +{ + struct kvm_cregs buf; + + kvm_read_cregs(&buf); + + /* Check that the page fault was caused by write to *readonly below */ + if (buf.cr2 == (uintptr_t)userdata) { + tst_res(TPASS, "KVM enforces memory write permissions"); + kvm_exit(); + } + + /* Unexpected page fault, fall back to default handler */ + return 0; +} + +void main(void) +{ + uintptr_t tmp; + struct page_table_entry_pae *subpte, *pte = kvm_pagetable; + int val, *writable, *readonly, *cacher1, *cacher2; + + if (!(kvm_rdmsr(MSR_EFER) & EFER_LMA)) + tst_brk(TBROK, "Bootstrap did not enable 64bit paging"); + + /* + * Find the first page table entry which branches. This entry was + * configured by bootstrap as follows: + * 0x00000000 - 0x3fffffff in pte[0] (identity mapped) + * 0x40000000 - 0x7fffffff in pte[1] (identity mapped) + * 0x80000000 - 0xbfffffff in pte[2] (unmapped) + * 0xc0000000 - 0xffffffff in pte[3] (only last page identity mapped) + */ + while (!pte[1].present) { + tmp = kvm_get_page_address_pae(pte); + pte = (struct page_table_entry_pae *)tmp; + } + + /* + * Setup mapping above the 32bit address space. The test needs two + * different unused 1GB chunks of address space. Remapping part of + * the lower 4GB address space would make it harder to reproduce + * the bug because any memory access in the same 1GB chunk (even + * fetching instructions by the CPU) could evict entries from page + * table cache and force the bypassable write permission check + * to happen even on buggy kernels. + * + * Allocate 3 pages for page table + 2 pages for data + */ + writable = tst_heap_alloc_aligned(5 * PAGESIZE, PAGESIZE); + memset(writable, 0, 5 * PAGESIZE); + tmp = (uintptr_t)writable; + pte[4].address = tmp >> 12; + pte[4].user_access = 1; + pte[4].writable = 1; + pte[4].present = 1; + pte[5] = pte[4]; + pte[5].writable = 0; + + subpte = (struct page_table_entry_pae *)tmp; + subpte[0].address = (tmp + PAGESIZE) >> 12; + subpte[0].user_access = 1; + subpte[0].writable = 0; + subpte[0].present = 1; + subpte[1].address = (tmp + 2 * PAGESIZE) >> 12; + subpte[1].user_access = 1; + subpte[1].writable = 1; + subpte[1].present = 1; + + subpte = (struct page_table_entry_pae *)(tmp + PAGESIZE); + subpte[0].address = (tmp + 3 * PAGESIZE) >> 12; + subpte[0].user_access = 1; + subpte[0].writable = 1; + subpte[0].present = 1; + + subpte = (struct page_table_entry_pae *)(tmp + 2 * PAGESIZE); + subpte[0].address = (tmp + 4 * PAGESIZE) >> 12; + subpte[0].user_access = 1; + subpte[0].writable = 1; + subpte[0].present = 1; + + /* Create pointers into the new mapping */ + cacher1 = (int *)0x100000000ULL; + writable = (int *)0x100200000ULL; + cacher2 = (int *)0x140000000ULL; + readonly = (int *)0x140200000ULL; + tst_set_interrupt_callback(INTR_PAGE_FAULT, handle_page_fault, + readonly); + + /* Fill page table cache */ + val = *cacher1; + *writable = val; + val = *cacher2; + + /* Trigger page fault (unless the kernel is vulnerable) */ + *readonly = val; + + /* This line should be unreachable */ + tst_res(TFAIL, "Write to read-only address did not page fault"); +} + +#else /* __x86_64__ */ +TST_TEST_TCONF("Test supported only on x86_64"); +#endif /* __x86_64__ */ + +#else /* COMPILE_PAYLOAD */ + +#include <ctype.h> +#include <stdio.h> +#include <unistd.h> +#include "tst_module.h" + +#define TDP_MMU_SYSFILE "/sys/module/kvm/parameters/tdp_mmu" +#define TDP_AMD_SYSFILE "/sys/module/kvm_amd/parameters/npt" +#define TDP_INTEL_SYSFILE "/sys/module/kvm_intel/parameters/ept" + +#define BUF_SIZE 64 + +static int read_bool_sys_param(const char *filename) +{ + char buf[BUF_SIZE]; + int i, fd, ret; + + fd = open(filename, O_RDONLY); + + if (fd < 0) + return -1; + + ret = read(fd, buf, BUF_SIZE - 1); + SAFE_CLOSE(fd); + + if (ret < 1) + return -1; + + buf[ret] = '\0'; + + for (i = 0; buf[i] && !isspace(buf[i]); i++) + ; + + buf[i] = '\0'; + + if (isdigit(buf[0])) { + sscanf(buf, "%d", &ret); + return ret; + } + + if (!strcasecmp(buf, "N")) + return 0; + + /* Assume that any other value than 0 or N means the param is enabled */ + return 1; +} + +static void reload_module(const char *module, char *arg) +{ + const char *const argv[] = {"modprobe", module, arg, NULL}; + + tst_res(TINFO, "Reloading module %s with parameter %s", module, arg); + tst_module_unload(module); + tst_cmd(argv, NULL, NULL, 0); +} + +static void disable_tdp(void) +{ + if (!access(TDP_MMU_SYSFILE, F_OK)) { + /* FIXME: Is this sufficient to disable TDP? */ + SAFE_FILE_PRINTF(TDP_MMU_SYSFILE, "0"); + return; + } + + if (read_bool_sys_param(TDP_AMD_SYSFILE) > 0) + reload_module("kvm_amd", "npt=0"); + + if (read_bool_sys_param(TDP_INTEL_SYSFILE) > 0) + reload_module("kvm_intel", "ept=0"); +} + +static void setup(void) +{ + disable_tdp(); + tst_kvm_setup(); +} + +static struct tst_test test = { + .test_all = tst_kvm_run, + .setup = setup, + .cleanup = tst_kvm_cleanup, + .needs_root = 1, + .save_restore = (const char *const []) { + "?/sys/module/kvm/parameters/tdp_mmu", + NULL + }, + .supported_archs = (const char *const []) { + "x86_64", + NULL + }, + .tags = (struct tst_tag[]){ + {"linux-git", "b1bd5cba3306"}, + {"CVE", "2021-38198"}, + {} + } +}; + +#endif /* COMPILE_PAYLOAD */
Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- runtest/kvm | 1 + testcases/kernel/kvm/.gitignore | 1 + testcases/kernel/kvm/kvm_pagefault01.c | 235 +++++++++++++++++++++++++ 3 files changed, 237 insertions(+) create mode 100644 runtest/kvm create mode 100644 testcases/kernel/kvm/.gitignore create mode 100644 testcases/kernel/kvm/kvm_pagefault01.c