Re: [PATCH v5 07/17] virt: acrn: Introduce an ioctl to set vCPU registers state - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Shuo A Liu <shuo.a.liu@intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	"H . Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Yu Wang <yu1.wang@intel.com>,
	Reinette Chatre <reinette.chatre@intel.com>,
	Zhi Wang <zhi.a.wang@intel.com>,
	Zhenyu Wang <zhenyuw@linux.intel.com>
Subject: Re: [PATCH v5 07/17] virt: acrn: Introduce an ioctl to set vCPU registers state
Date: Tue, 10 Nov 2020 15:54:26 +0100	[thread overview]
Message-ID: <X6qpovz2TVpvZjDh@kroah.com> (raw)
In-Reply-To: <20201110131419.GG17702@shuo-intel.sh.intel.com>

On Tue, Nov 10, 2020 at 09:14:19PM +0800, Shuo A Liu wrote:
> > And there really is no validation of
> > any fields?
> 
> Yes. Because HSM driver has little knowledge to do the validation.

What is "HSM driver"?  And you all are ready for fuzzers to break this
into small pieces, right?  No validation of any input parameters feels
really really wrong.  Best of luck!

> > > +struct acrn_regs {
> > > +	struct acrn_gp_regs		gprs;
> > > +	struct acrn_descriptor_ptr	gdt;
> > > +	struct acrn_descriptor_ptr	idt;
> > > +
> > > +	__u64				rip;
> > 
> > As these are all crossing the user/kernel boundry and then on to
> > somewhere "else", you have to specify the endian of all of these, right?
> > 
> > if not, why not?
> 
> The hypervisor and the driver only support X86_64 platform for now. So, the
> endian should be certain.

Then specify it please.

> > > +	__u16			reserved0[3];
> > 
> > What does the reserved fields do?
> 
> To keep same layout with the hypervisor. Because the structure will be
> passed to hypervisor directly.
> 
> > 
> > Is there a pointer to a public document for all of these structures
> > somewhere?
> 
> Unfortunately, no. I have added some documents for some strutures
> in the code via kernel-doc format.

Is this not the hypervisor that this code is for:
	https://projectacrn.org/
?

If not, what is this thing?

If so, how is there not documentation for it?

> > > +	struct acrn_regs	vcpu_regs;
> > > +} __attribute__((aligned(8)));
> > 
> > What does the alignment do here?
> 
> The hypervisor wants to access aligned data block to improve the
> efficiency. Currently, the hypervisor only runs on x86_64 platform.

That's nice, but what do you think that adding this attribute to a
structure provides you?  Have you tested this really is doing what you
think it is doing?

thanks,

greg k-h