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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Nov 11, 2020 at 05:54:31PM +0800, Shuo A Liu wrote:
> On Tue 10.Nov'20 at 15:54:26 +0100, Greg Kroah-Hartman wrote:
> > 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!
> 
> This driver is HSM (Hypervisor Service Module) driver.
> Take this hypercall for example. The register values are set by user, we
> can do nothing to verify them. If the values are wrong, the VM will
> crash and the hypervisor will handle that.

Ah, nice, so you are creating a situation where any user can crash the
VM, what can go wrong :(

> > > >
> > > > 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?
> 
> Oh, yes. We have the structures documentation on the website. Pls refer
> https://projectacrn.github.io/latest/api/hypercall_api.html#
> I meant that some fields of structures might be lack of explanation.

Please work on the documentation of the fields, otherwise it doesn't
really make much sense what is happening here, right?

Along these lines, where is the userspace code that makes all of these
ioctls?  Surely the fields must be documented there, right?

> > > > > +	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?
> 
> It could make the compiler put the data structure at aligned address.

Are you sure this is the correct way to do that?

> In the kernel the structures are almost from kmalloc, so the attribute
> might be not meaningful. But in the hypervisor, there are many such data
> structures in stack or in static pool, this attribute can make sure the
> data structures are located at the aligned address.

This is kernel data, not hypervisor data, in kernel memory.  If you
require the hypervisor to get the structures at a specific alignment in
memory, you better check that in the kernel code, otherwise how can you
ensure it?

thanks,

greg k-h