linux-kernel - Re: [PATCH v2 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC

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: <9dbc9cedd6b49eb5c5078dd776aed808534534ec.camel@kernel.org>
Date:   Tue, 12 Oct 2021 20:43:44 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     dave.hansen@...ux.intel.com, seanjc@...gle.com, x86@...nel.org,
        yang.zhong@...el.com
Subject: Re: [PATCH v2 2/2] x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE
 ioctl

On Tue, 2021-10-12 at 19:03 +0200, Paolo Bonzini wrote:
> On 12/10/21 18:57, Jarkko Sakkinen wrote:
> > > +
> > >   static const struct file_operations sgx_vepc_fops = {
> > >          .owner          = THIS_MODULE,
> > >          .open           = sgx_vepc_open,
> > > +       .unlocked_ioctl = sgx_vepc_ioctl,
> > > +       .compat_ioctl   = sgx_vepc_ioctl,
> > >          .release        = sgx_vepc_release,
> > >          .mmap           = sgx_vepc_mmap,
> > >   };
> > I went through this a few times, the code change is sound and
> > reasoning makes sense in the commit message.
> > 
> > The only thing that I think that is IMHO lacking is a simple
> > kselftest. I think a trivial test for SGX_IOC_VEP_REMOVE_ALL
> > would do.
> 
> Yeah, a trivial test wouldn't cover a lot; it would be much better to at 
> least set up a SECS, and check that the first call returns 1 and the 
> second returns 0.  There is no existing test for /dev/sgx_vepc at all.
> 
> Right now I'm relying on Yang for testing this in QEMU, but I'll look 
> into adding a selftest that does the full setup and runs an enclave in a 
> guest.

This having a regression would not working would not cause that much collateral
damage, especially since it would be probably quickly noticed by someone, so I
think that this is not absolutely mandatory. So you can ignore kselftest part,
and thus

Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>

Thank you, this work helps me a lot, given that my SGX testing is based around
using QEMU ATM.

/Jarkko