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

[Shuo A Liu <shuo.a.liu@intel.com>]

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