Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

[Jarkko Sakkinen]

On Fri, Jun 26, 2020 at 11:14:19AM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
> >                                                                       <mailto:tlewis@xxxxxxxxxxxxxx>
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 
> Maybe add <mailto:linux-sgx@xxxxxxxxxxxxxxx> ?
> 
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> 
> ...
> 
> > +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> > +{
> > +	unsigned long encl_size = secs->size + PAGE_SIZE;
> 
> Wait, you just copied @secs from user memory in sgx_ioc_enclave_create()
> and now use ->size unverified? You're kidding, right?

The validation is done in sgx_validate_secs().

> 
> > +	struct sgx_epc_page *secs_epc;
> > +	unsigned long ssaframesize;
> > +	struct sgx_pageinfo pginfo;
> > +	struct sgx_secinfo secinfo;
> > +	struct file *backing;
> > +	long ret;
> > +
> > +	if (atomic_read(&encl->flags) & SGX_ENCL_CREATED)
> > +		return -EINVAL;
> > +
> > +	ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
> 
> So this is using more un-validated user input to do further calculations.
> What can possibly go wrong?
> 
> I sure hope *I* am wrong and am missing something here.
> 
> If not, please, for the next version, audit all your user input and
> validate it before using it. Srsly.

It works but is unclean. I'd guess reason for this is just that code has
evolved into this state over time.

I'd just move the call to sgx_calc_ssaframesize() inside
sgx_validate_secs().

/Jarkko