Messages in this thread | | | Date | Wed, 11 Nov 2020 20:03:48 +0800 | From | Shuo A Liu <> | Subject | Re: [PATCH v5 07/17] virt: acrn: Introduce an ioctl to set vCPU registers state |
| |
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
| |