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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X6qpovz2TVpvZjDh@kroah.com>
Date:   Tue, 10 Nov 2020 15:54:26 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Shuo A Liu <shuo.a.liu@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Yu Wang <yu1.wang@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Zhi Wang <zhi.a.wang@...el.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>
Subject: Re: [PATCH v5 07/17] virt: acrn: Introduce an ioctl to set vCPU
 registers state

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