linux-kernel - Re: [PATCH] KVM: selftests: Add tests - invalid inputs for KVM_CREATE_GUEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZOepM0jZooyCppUs@google.com>
Date:   Thu, 24 Aug 2023 12:02:11 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Ackerley Tng <ackerleytng@...gle.com>
Cc:     pbonzini@...hat.com, tglx@...utronix.de, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, shuah@...nel.org,
        andrew.jones@...ux.dev, ricarkol@...gle.com,
        chao.p.peng@...ux.intel.com, tabba@...gle.com, jarkko@...nel.org,
        yu.c.zhang@...ux.intel.com, vannapurve@...gle.com,
        erdemaktas@...gle.com, mail@...iej.szmigiero.name, vbabka@...e.cz,
        david@...hat.com, qperret@...gle.com, michael.roth@....com,
        wei.w.wang@...el.com, liam.merwick@...cle.com,
        isaku.yamahata@...il.com, kirill.shutemov@...ux.intel.com
Subject: Re: [PATCH] KVM: selftests: Add tests - invalid inputs for KVM_CREATE_GUEST_MEMFD

On Mon, Aug 21, 2023, Ackerley Tng wrote:
> Test that invalid inputs for KVM_CREATE_GUEST_MEMFD, such as
> non-page-aligned page size and invalid flags, are rejected by the
> KVM_CREATE_GUEST_MEMFD with EINVAL
> 
> Signed-off-by: Ackerley Tng <ackerleytng@...gle.com>
> ---
>  .../testing/selftests/kvm/guest_memfd_test.c  | 33 +++++++++++++++++++
>  .../selftests/kvm/include/kvm_util_base.h     | 11 +++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c
> index eb93c608a7e0..a8e37f001297 100644
> --- a/tools/testing/selftests/kvm/guest_memfd_test.c
> +++ b/tools/testing/selftests/kvm/guest_memfd_test.c
> @@ -90,6 +90,37 @@ static void test_fallocate(int fd, size_t page_size, size_t total_size)
>  	TEST_ASSERT(!ret, "fallocate to restore punched hole should succeed");
>  }
>  
> +static void test_create_guest_memfd_invalid(struct kvm_vm *vm, size_t page_size)
> +{
> +	int fd;
> +	uint64_t size;

This should be size_t.

> +	uint64_t flags;
> +	uint64_t valid_flags = 0;

Revert fir/xmas-tree please.

> +
> +	for (size = 1; size < page_size; size++) {
> +		fd = __vm_create_guest_memfd(vm, size, 0);
> +		TEST_ASSERT(

No, bad Google3, bad.  Never immediately wrap after an opening parenthesis.

> +			fd == -1,
> +			"Creating guest memfds with non-page-aligned page sizes should fail");
> +		TEST_ASSERT(errno == EINVAL, "... and errno should be set to EINVAL");

Don't split/delay "errno" checks, it's all too easy for errno to get clobbered.
And there's absolutely zero reason to split these, the ret+errno get printed so
the odds of what went wrong not being super duper obvious are very low.  What
_is_ worth printing is the size.

> +	}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	valid_flags = KVM_GUEST_MEMFD_ALLOW_HUGEPAGE;
> +#endif

Ugh, this is annoying.  But HPAGE_PMD_SIZE wrapping with CONFIG_TRANSPARENT_HUGEPAGE
and so guest_memfd() can't (easily) enforce the alignment check if THP is disabled,
i.e. always letting userspace specify KVM_GUEST_MEMFD_ALLOW_HUGEPAGE would be
messy.

Oh!  And we should also test for unaligned huge pages, i.e. multiples of page_size
that aren't PMD-aligned.  At that point, I would say don't pass in @page_size to
this particular testcase, e.g. have main() be something like this:

	vm = vm_create_barebones();

	test_create_guest_memfd_invalid(vm);

	page_size = getpagesize();
	total_size = page_size * 4;
	fd = vm_create_guest_memfd(vm, total_size, 0);

	test_file_read_write(fd);
	test_mmap(fd, page_size);
	test_file_size(fd, page_size, total_size);
	test_fallocate(fd, page_size, total_size);

And then in here, use get_trans_hugepagesz() to do negative testing of
KVM_GUEST_MEMFD_ALLOW_HUGEPAGE.

> +
> +	for (flags = 1; flags; flags <<= 1) {
> +		if (flags & valid_flags)

This only ever tests one flag in isolation, e.g. if it would detect if KVM did
something ridiculous like

	if (flags && !(flags & KVM_GUEST_MEMFD_ALLOW_HUGEPAGE))
		return -EINVAL;

Iterating over all possible values doesn't make sense, and giving "lower" flags
preference is likewise a bit silly, so what if we do (note the s/flags/flag)

	for (flag = 1; flag; flag <<= 1) {
		if (flag & valid_flags)
			continue;

		fd = __vm_create_guest_memfd(vm, page_size, flag);
		TEST_ASSERT(fd == -1 && errno == EINVAL,
			    "guest_memfd() with flags '0x%llx' should fail with EINVAL", flag);

		for_each_set_bit(bit, &valid_flags, 64) {
			fd = __vm_create_guest_memfd(vm, page_size, flag | BIT_ULL(bit));
			TEST_ASSERT(fd == -1 && errno == EINVAL,
				    "guest_memfd() with flags '0x%llx' should fail with EINVAL",
			    	    flag | BIT_ULL(bit));
		}
	}

i.e. test the invalid flag in isolation, and then also test it in combination with
each valid flag.  It's from from exhaustive, but it'll at least ensure we have *some*
coverage if/when new flags come along.