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

[Shuo A Liu]

On Wed 11.Nov'20 at 11:28:40 +0100, Greg Kroah-Hartman wrote:
> 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 :(

Not any user, only the one who create the VM. Others cannot access the
same VM. Let me list a piece of pesudo code of the device usage:

 fd = open the /dev/acrn_hsm, HSM driver binds a VM with the fd
 ioctl (fd, CREATE_VM, SET_VCPU_REGS, START_VM ...)
 close fd

The one who create the VM should have full control of the VM.

> > > > >
> > > > > 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?

Right. We will try to enrich them.

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

A userspace tool uses the ioctls, you can find the source from:
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/core/vmmapi.c
There is a bit difference with the version i posted as i did some 
polish for upstream.

> > > > > > +	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?

I tried the attribute it works.

test.c:

struct foo_256_aligned {
	int a;
	unsigned char b;
} __attribute__((aligned(256)));

struct foo {
	int a;
	unsigned char b;
};
int main(int argc, char** argv) {
	struct foo_256_aligned a;
	struct foo b;
	printf("%p %p\n", &a, &b);
}

# gcc -o test test.c && ./test
0x7ffe2af04b00 0x7ffe2af04af8

> > 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?

Oh, this is a public header file which will be used by the hypervisor as
well. For kernel data from kmalloc, it should be also ok even if it
isn't 8 bytes aligned. The hypervisor will copy them if need keep
persistent.

Thanks
shuo