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] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Apr 2024 07:47:20 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Andrew Jones <ajones@...tanamicro.com>, Markus Elfring <Markus.Elfring@....de>, 
	Kunwu Chan <chentao@...inos.cn>, linux-kselftest@...r.kernel.org, kvm@...r.kernel.org, 
	kernel-janitors@...r.kernel.org, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>, Paolo Bonzini <pbonzini@...hat.com>, 
	Shuah Khan <shuah@...nel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Kunwu Chan <kunwu.chan@...mail.com>, Anup Patel <anup@...infault.org>, 
	Thomas Huth <thuth@...hat.com>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH] KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state

On Wed, Apr 24, 2024, Dan Carpenter wrote:
> On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
> > On Tue, Apr 23, 2024, Andrew Jones wrote:
> > > On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:
> > > > +others
> > > > 
> > > > On Tue, Apr 23, 2024, Markus Elfring wrote:
> > > > > …
> > > > > > This patch will add the malloc failure checking
> > > > > …
> > > > > 
> > > > > * Please use a corresponding imperative wording for the change description.
> > > > > 
> > > > > * Would you like to add the tag “Fixes” accordingly?
> > > > 
> > > > Nah, don't bother with Fixes.  OOM will cause the test to fail regardless, the
> > > > fact that it gets an assert instead a NULL pointer deref is nice to have, but by
> > > > no means does it fix a bug.
> > > > 
> > > > > > +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> > > > > > @@ -91,6 +91,7 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu)
> > > > > >  	const int state_sz = sizeof(struct kvm_nested_state) + getpagesize();
> > > > > >  	struct kvm_nested_state *state =
> > > > > >  		(struct kvm_nested_state *)malloc(state_sz);
> > > > > > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> > > > > …
> > > > > 
> > > > > Can “errno” be relevant for the error message construction?
> > > > 
> > > > Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() spits
> > > > out the actual errno, and we can just say something like "malloc() failed for
> > > > blah blah blah".  
> > > > 
> > > > But rather than keeping playing whack-a-mole, what if we add macros to perform
> > > > allocations and assert on the result?  I have zero interest in chasing down all
> > > > of the "unsafe" allocations, and odds are very good that we'll collectively fail
> > > > to enforce checking on new code.
> > > > 
> > > > E.g. something like (obviously won't compile, just for demonstration purposes)
> > > > 
> > > > #define kvm_malloc(x)
> > > > ({
> > > > 	void *__ret;
> > > > 
> > > > 	__ret  = malloc(x);
> > > > 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> > > > 	__ret;
> > > > })
> > > > 
> > > > #define kvm_calloc(x, y)
> > > > ({
> > > > 	void *__ret;
> > > > 
> > > > 	__ret  = calloc(x, y);
> > > > 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> > > > 	__ret;
> > > > })
> > > 
> > > Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
> > > put them in include/test_util.h
> > 
> > Possibly terrible idea: what if we used kmalloc() and kcalloc()?  K is for KVM :-)
> 
> That's a legit terrible idea...  It probably would trigger more static
> checker warnings because the general policy is kmalloc() is kernel code
> and we *have* to test for errors.

Roger that.

> To be honest, I would have just rejected the first patch.  You
> obviously know this and have said this earlier in the thread but just
> for the other people, this is a userspace test that runs for a short
> time and then exits.  If it gets killed because we don't have enough
> memory that's fine.  It would be better to just fix the static checker
> to not print pointless warnings or educate people to ignore warnings
> like this.

This particular patch may have been motiviated by a static checker, but I doubt
static checkers are responsible for all of the many sanity checks on malloc() in
KVM selftests.  And while I agree that the sanity checks don't and much value,
deleting the existing checks and preventing checks from being reintroduced would
be a never ending battle.

> Creating the test_malloc() to silence the warning also seems like an
> okay idea as well.

Yeah, it's not exactly my first choice, but the reality is that people write KVM
elftests by copying an existing test (often literally), and so the best way to
educate developers on the preferred approach/style is to have all existing code
adhere to a single approach/style.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ