| Message ID | 20221026102018.4144-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | XSA-409 fixes | expand |
On 26.10.2022 12:20, Andrew Cooper wrote: > --- /dev/null > +++ b/tools/tests/p2m-pool/Makefile > @@ -0,0 +1,42 @@ > +XEN_ROOT = $(CURDIR)/../../.. > +include $(XEN_ROOT)/tools/Rules.mk > + > +TARGET := test-p2m-pool > + > +.PHONY: all > +all: $(TARGET) > + > +.PHONY: clean > +clean: > + $(RM) -- *.o $(TARGET) $(DEPS_RM) > + > +.PHONY: distclean > +distclean: clean > + $(RM) -- *~ > + > +.PHONY: install > +install: all > + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) > + $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN) > + > +.PHONY: uninstall > +uninstall: > + $(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET) > + > +CFLAGS += $(CFLAGS_xeninclude) > +CFLAGS += $(CFLAGS_libxenctrl) > +CFLAGS += $(CFLAGS_libxenforeginmemory) Typo here or typo also elsewhere: CFLAGS_libxenforeignmemory? I have to admit that I can't really spot where these variables are populated. The place in Rules.mk that I could spot uses CFLAGS_libxen$(1) = $$(CFLAGS_xeninclude) i.e. the expansion doesn't really depend on the library. Apart from this looks okay to me, maybe apart from ... > --- /dev/null > +++ b/tools/tests/p2m-pool/test-p2m-pool.c > @@ -0,0 +1,181 @@ > +#include <err.h> > +#include <errno.h> > +#include <inttypes.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/mman.h> > + > +#include <xenctrl.h> > +#include <xenforeignmemory.h> > +#include <xengnttab.h> > +#include <xen-tools/libs.h> > + > +static unsigned int nr_failures; > +#define fail(fmt, ...) \ > +({ \ > + nr_failures++; \ > + (void)printf(fmt, ##__VA_ARGS__); \ > +}) > + > +static xc_interface *xch; > +static uint32_t domid; > + > +static struct xen_domctl_createdomain create = { > + .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > + .max_vcpus = 1, > + .max_grant_frames = 1, > + .grant_opts = XEN_DOMCTL_GRANT_version(1), > + > + .arch = { > +#if defined(__x86_64__) || defined(__i386__) > + .emulation_flags = XEN_X86_EMU_LAPIC, > +#endif > + }, > +}; > + > +static uint64_t default_p2m_size_bytes = > +#if defined(__x86_64__) || defined(__i386__) > + 256 << 12; /* Only x86 HAP for now. x86 Shadow broken. */ ... this shadow related comment (the commit message could at least say what's wrong there, to give a hint at what would need fixing to remove this restriction) and ... > +#elif defined (__arm__) || defined(__aarch64__) > + 16 << 12; > +#endif > + > +static void run_tests(void) > +{ > + xen_pfn_t physmap[] = { 0 }; > + uint64_t size_bytes, old_size_bytes; > + int rc; > + > + printf("Test default p2m mempool size\n"); > + > + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); > + if ( rc ) > + return fail(" Fail: get p2m mempool size: %d - %s\n", > + errno, strerror(errno)); > + > + printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n", > + size_bytes, size_bytes >> 10, size_bytes >> 20); > + > + > + /* > + * Check that the domain has the expected default allocation size. This > + * will fail if the logic in Xen is altered without an equivelent > + * adjustment here. > + */ > + if ( size_bytes != default_p2m_size_bytes ) > + return fail(" Fail: size %"PRIu64" != expected size %"PRIu64"\n", > + size_bytes, default_p2m_size_bytes); > + > + > + printf("Test that allocate doesn't alter pool size\n"); > + > + /* > + * Populate the domain with some RAM. This will cause more of the p2m > + * pool to be used. > + */ > + old_size_bytes = size_bytes; > + > + rc = xc_domain_setmaxmem(xch, domid, -1); > + if ( rc ) > + return fail(" Fail: setmaxmem: : %d - %s\n", > + errno, strerror(errno)); > + > + rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap); > + if ( rc ) > + return fail(" Fail: populate physmap: %d - %s\n", > + errno, strerror(errno)); > + > + /* > + * Re-get the p2m size. Should not have changed as a consequence of > + * populate physmap. > + */ > + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); > + if ( rc ) > + return fail(" Fail: get p2m mempool size: %d - %s\n", > + errno, strerror(errno)); > + > + if ( old_size_bytes != size_bytes ) > + return fail(" Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n", > + old_size_bytes, size_bytes); > + > + > + > + printf("Test bad set size\n"); > + > + /* > + * Check that setting a non-page size results in failure. > + */ > + rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1); > + if ( rc != -1 || errno != EINVAL ) > + return fail(" Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n", > + rc, errno, strerror(errno)); > + > + > + printf("Test very large set size\n"); > + > + /* > + * Check that setting a large P2M size succeeds. This is expecting to > + * trigger continuations. > + */ > + rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20); > + if ( rc ) > + return fail(" Fail: Set size 64MB: %d - %s\n", > + errno, strerror(errno)); > + > + > + /* > + * Check that the reported size matches what set consumed. > + */ > + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); > + if ( rc ) > + return fail(" Fail: get p2m mempool size: %d - %s\n", > + errno, strerror(errno)); > + > + if ( size_bytes != 64 << 20 ) > + return fail(" Fail: expected mempool size %u, got %"PRIu64"\n", > + 64 << 20, size_bytes); > +} > + > +int main(int argc, char **argv) > +{ > + int rc; > + > + printf("P2M Shadow memory pool tests\n"); ... the question of why "Shadow" here. Jan
On 26/10/2022 15:24, Jan Beulich wrote: > On 26.10.2022 12:20, Andrew Cooper wrote: >> --- /dev/null >> +++ b/tools/tests/p2m-pool/Makefile >> @@ -0,0 +1,42 @@ >> +XEN_ROOT = $(CURDIR)/../../.. >> +include $(XEN_ROOT)/tools/Rules.mk >> + >> +TARGET := test-p2m-pool >> + >> +.PHONY: all >> +all: $(TARGET) >> + >> +.PHONY: clean >> +clean: >> + $(RM) -- *.o $(TARGET) $(DEPS_RM) >> + >> +.PHONY: distclean >> +distclean: clean >> + $(RM) -- *~ >> + >> +.PHONY: install >> +install: all >> + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) >> + $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN) >> + >> +.PHONY: uninstall >> +uninstall: >> + $(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET) >> + >> +CFLAGS += $(CFLAGS_xeninclude) >> +CFLAGS += $(CFLAGS_libxenctrl) >> +CFLAGS += $(CFLAGS_libxenforeginmemory) > Typo here or typo also elsewhere: CFLAGS_libxenforeignmemory? I > have to admit that I can't really spot where these variables are > populated. The place in Rules.mk that I could spot uses > > CFLAGS_libxen$(1) = $$(CFLAGS_xeninclude) > > i.e. the expansion doesn't really depend on the library. Hmm. It was copy&paste from test-resource and I forgot to trim foreignmemory, but I guess I'll need to go and fix the typo everywhere. > > Apart from this looks okay to me, maybe apart from ... > >> --- /dev/null >> +++ b/tools/tests/p2m-pool/test-p2m-pool.c >> @@ -0,0 +1,181 @@ >> +#include <err.h> >> +#include <errno.h> >> +#include <inttypes.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include <sys/mman.h> >> + >> +#include <xenctrl.h> >> +#include <xenforeignmemory.h> >> +#include <xengnttab.h> >> +#include <xen-tools/libs.h> >> + >> +static unsigned int nr_failures; >> +#define fail(fmt, ...) \ >> +({ \ >> + nr_failures++; \ >> + (void)printf(fmt, ##__VA_ARGS__); \ >> +}) >> + >> +static xc_interface *xch; >> +static uint32_t domid; >> + >> +static struct xen_domctl_createdomain create = { >> + .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >> + .max_vcpus = 1, >> + .max_grant_frames = 1, >> + .grant_opts = XEN_DOMCTL_GRANT_version(1), >> + >> + .arch = { >> +#if defined(__x86_64__) || defined(__i386__) >> + .emulation_flags = XEN_X86_EMU_LAPIC, >> +#endif >> + }, >> +}; >> + >> +static uint64_t default_p2m_size_bytes = >> +#if defined(__x86_64__) || defined(__i386__) >> + 256 << 12; /* Only x86 HAP for now. x86 Shadow broken. */ > ... this shadow related comment (the commit message could at least > say what's wrong there, to give a hint at what would need fixing to > remove this restriction) and ... That was in reference to the PV issue. Will follow it up on patch 1 where there's more context. > >> +#elif defined (__arm__) || defined(__aarch64__) >> + 16 << 12; >> +#endif >> + >> +static void run_tests(void) >> +{ >> + xen_pfn_t physmap[] = { 0 }; >> + uint64_t size_bytes, old_size_bytes; >> + int rc; >> + >> + printf("Test default p2m mempool size\n"); >> + >> + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); >> + if ( rc ) >> + return fail(" Fail: get p2m mempool size: %d - %s\n", >> + errno, strerror(errno)); >> + >> + printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n", >> + size_bytes, size_bytes >> 10, size_bytes >> 20); >> + >> + >> + /* >> + * Check that the domain has the expected default allocation size. This >> + * will fail if the logic in Xen is altered without an equivelent >> + * adjustment here. >> + */ >> + if ( size_bytes != default_p2m_size_bytes ) >> + return fail(" Fail: size %"PRIu64" != expected size %"PRIu64"\n", >> + size_bytes, default_p2m_size_bytes); >> + >> + >> + printf("Test that allocate doesn't alter pool size\n"); >> + >> + /* >> + * Populate the domain with some RAM. This will cause more of the p2m >> + * pool to be used. >> + */ >> + old_size_bytes = size_bytes; >> + >> + rc = xc_domain_setmaxmem(xch, domid, -1); >> + if ( rc ) >> + return fail(" Fail: setmaxmem: : %d - %s\n", >> + errno, strerror(errno)); >> + >> + rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap); >> + if ( rc ) >> + return fail(" Fail: populate physmap: %d - %s\n", >> + errno, strerror(errno)); >> + >> + /* >> + * Re-get the p2m size. Should not have changed as a consequence of >> + * populate physmap. >> + */ >> + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); >> + if ( rc ) >> + return fail(" Fail: get p2m mempool size: %d - %s\n", >> + errno, strerror(errno)); >> + >> + if ( old_size_bytes != size_bytes ) >> + return fail(" Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n", >> + old_size_bytes, size_bytes); >> + >> + >> + >> + printf("Test bad set size\n"); >> + >> + /* >> + * Check that setting a non-page size results in failure. >> + */ >> + rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1); >> + if ( rc != -1 || errno != EINVAL ) >> + return fail(" Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n", >> + rc, errno, strerror(errno)); >> + >> + >> + printf("Test very large set size\n"); >> + >> + /* >> + * Check that setting a large P2M size succeeds. This is expecting to >> + * trigger continuations. >> + */ >> + rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20); >> + if ( rc ) >> + return fail(" Fail: Set size 64MB: %d - %s\n", >> + errno, strerror(errno)); >> + >> + >> + /* >> + * Check that the reported size matches what set consumed. >> + */ >> + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); >> + if ( rc ) >> + return fail(" Fail: get p2m mempool size: %d - %s\n", >> + errno, strerror(errno)); >> + >> + if ( size_bytes != 64 << 20 ) >> + return fail(" Fail: expected mempool size %u, got %"PRIu64"\n", >> + 64 << 20, size_bytes); >> +} >> + >> +int main(int argc, char **argv) >> +{ >> + int rc; >> + >> + printf("P2M Shadow memory pool tests\n"); > ... the question of why "Shadow" here. Hmm, stale, but even the name of this test (p2m-pool) is subject to a resolution of the naming question on patch 1. ~Andrew
diff --git a/tools/tests/Makefile b/tools/tests/Makefile index d99146d56a64..7ce8b7b881db 100644 --- a/tools/tests/Makefile +++ b/tools/tests/Makefile @@ -11,6 +11,7 @@ endif SUBDIRS-y += xenstore SUBDIRS-y += depriv SUBDIRS-y += vpci +SUBDIRS-y += p2m-pool .PHONY: all clean install distclean uninstall all clean distclean install uninstall: %: subdirs-% diff --git a/tools/tests/p2m-pool/.gitignore b/tools/tests/p2m-pool/.gitignore new file mode 100644 index 000000000000..cce6d97b1cc8 --- /dev/null +++ b/tools/tests/p2m-pool/.gitignore @@ -0,0 +1 @@ +test-p2m-pool diff --git a/tools/tests/p2m-pool/Makefile b/tools/tests/p2m-pool/Makefile new file mode 100644 index 000000000000..24f348f20582 --- /dev/null +++ b/tools/tests/p2m-pool/Makefile @@ -0,0 +1,42 @@ +XEN_ROOT = $(CURDIR)/../../.. +include $(XEN_ROOT)/tools/Rules.mk + +TARGET := test-p2m-pool + +.PHONY: all +all: $(TARGET) + +.PHONY: clean +clean: + $(RM) -- *.o $(TARGET) $(DEPS_RM) + +.PHONY: distclean +distclean: clean + $(RM) -- *~ + +.PHONY: install +install: all + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) + $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN) + +.PHONY: uninstall +uninstall: + $(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET) + +CFLAGS += $(CFLAGS_xeninclude) +CFLAGS += $(CFLAGS_libxenctrl) +CFLAGS += $(CFLAGS_libxenforeginmemory) +CFLAGS += $(CFLAGS_libxengnttab) +CFLAGS += $(APPEND_CFLAGS) + +LDFLAGS += $(LDLIBS_libxenctrl) +LDFLAGS += $(LDLIBS_libxenforeignmemory) +LDFLAGS += $(LDLIBS_libxengnttab) +LDFLAGS += $(APPEND_LDFLAGS) + +%.o: Makefile + +$(TARGET): test-p2m-pool.o + $(CC) -o $@ $< $(LDFLAGS) + +-include $(DEPS_INCLUDE) diff --git a/tools/tests/p2m-pool/test-p2m-pool.c b/tools/tests/p2m-pool/test-p2m-pool.c new file mode 100644 index 000000000000..1ffb19eeb420 --- /dev/null +++ b/tools/tests/p2m-pool/test-p2m-pool.c @@ -0,0 +1,181 @@ +#include <err.h> +#include <errno.h> +#include <inttypes.h> +#include <stdio.h> +#include <string.h> +#include <sys/mman.h> + +#include <xenctrl.h> +#include <xenforeignmemory.h> +#include <xengnttab.h> +#include <xen-tools/libs.h> + +static unsigned int nr_failures; +#define fail(fmt, ...) \ +({ \ + nr_failures++; \ + (void)printf(fmt, ##__VA_ARGS__); \ +}) + +static xc_interface *xch; +static uint32_t domid; + +static struct xen_domctl_createdomain create = { + .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, + .max_vcpus = 1, + .max_grant_frames = 1, + .grant_opts = XEN_DOMCTL_GRANT_version(1), + + .arch = { +#if defined(__x86_64__) || defined(__i386__) + .emulation_flags = XEN_X86_EMU_LAPIC, +#endif + }, +}; + +static uint64_t default_p2m_size_bytes = +#if defined(__x86_64__) || defined(__i386__) + 256 << 12; /* Only x86 HAP for now. x86 Shadow broken. */ +#elif defined (__arm__) || defined(__aarch64__) + 16 << 12; +#endif + +static void run_tests(void) +{ + xen_pfn_t physmap[] = { 0 }; + uint64_t size_bytes, old_size_bytes; + int rc; + + printf("Test default p2m mempool size\n"); + + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); + if ( rc ) + return fail(" Fail: get p2m mempool size: %d - %s\n", + errno, strerror(errno)); + + printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n", + size_bytes, size_bytes >> 10, size_bytes >> 20); + + + /* + * Check that the domain has the expected default allocation size. This + * will fail if the logic in Xen is altered without an equivelent + * adjustment here. + */ + if ( size_bytes != default_p2m_size_bytes ) + return fail(" Fail: size %"PRIu64" != expected size %"PRIu64"\n", + size_bytes, default_p2m_size_bytes); + + + printf("Test that allocate doesn't alter pool size\n"); + + /* + * Populate the domain with some RAM. This will cause more of the p2m + * pool to be used. + */ + old_size_bytes = size_bytes; + + rc = xc_domain_setmaxmem(xch, domid, -1); + if ( rc ) + return fail(" Fail: setmaxmem: : %d - %s\n", + errno, strerror(errno)); + + rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap); + if ( rc ) + return fail(" Fail: populate physmap: %d - %s\n", + errno, strerror(errno)); + + /* + * Re-get the p2m size. Should not have changed as a consequence of + * populate physmap. + */ + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); + if ( rc ) + return fail(" Fail: get p2m mempool size: %d - %s\n", + errno, strerror(errno)); + + if ( old_size_bytes != size_bytes ) + return fail(" Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n", + old_size_bytes, size_bytes); + + + + printf("Test bad set size\n"); + + /* + * Check that setting a non-page size results in failure. + */ + rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1); + if ( rc != -1 || errno != EINVAL ) + return fail(" Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n", + rc, errno, strerror(errno)); + + + printf("Test very large set size\n"); + + /* + * Check that setting a large P2M size succeeds. This is expecting to + * trigger continuations. + */ + rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20); + if ( rc ) + return fail(" Fail: Set size 64MB: %d - %s\n", + errno, strerror(errno)); + + + /* + * Check that the reported size matches what set consumed. + */ + rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes); + if ( rc ) + return fail(" Fail: get p2m mempool size: %d - %s\n", + errno, strerror(errno)); + + if ( size_bytes != 64 << 20 ) + return fail(" Fail: expected mempool size %u, got %"PRIu64"\n", + 64 << 20, size_bytes); +} + +int main(int argc, char **argv) +{ + int rc; + + printf("P2M Shadow memory pool tests\n"); + + xch = xc_interface_open(NULL, NULL, 0); + + if ( !xch ) + err(1, "xc_interface_open"); + + rc = xc_domain_create(xch, &domid, &create); + if ( rc ) + { + if ( errno == EINVAL || errno == EOPNOTSUPP ) + printf(" Skip: %d - %s\n", errno, strerror(errno)); + else + fail(" Domain create failure: %d - %s\n", + errno, strerror(errno)); + goto out; + } + + printf(" Created d%u\n", domid); + + run_tests(); + + rc = xc_domain_destroy(xch, domid); + if ( rc ) + fail(" Failed to destroy domain: %d - %s\n", + errno, strerror(errno)); + out: + return !!nr_failures; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
Exercise some basic functionality of the new xc_{get,set}_p2m_mempool_size() hypercalls. This passes on x86, but fails currently on ARM. ARM will be fixed up in future patches. This is part of XSA-409 / CVE-2022-33747. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Xen Security Team <security@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Henry Wang <Henry.Wang@arm.com> CC: Anthony PERARD <anthony.perard@citrix.com> --- tools/tests/Makefile | 1 + tools/tests/p2m-pool/.gitignore | 1 + tools/tests/p2m-pool/Makefile | 42 ++++++++ tools/tests/p2m-pool/test-p2m-pool.c | 181 +++++++++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+) create mode 100644 tools/tests/p2m-pool/.gitignore create mode 100644 tools/tests/p2m-pool/Makefile create mode 100644 tools/tests/p2m-pool/test-p2m-pool.c