Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Fri, Aug 09, 2019 at 06:02:08PM +0300, Jarkko Sakkinen wrote:
> On Thu, 2019-08-08 at 08:40 -0700, Sean Christopherson wrote:
> > On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote:
> > > > > + /* TCS pages need to be RW in the PTEs, but can be 0 in the
> > > > > EPCM. */
> > > > > + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> > > > > SGX_SECINFO_TCS)
> > > > > + prot |= PROT_READ | PROT_WRITE;
> > > >
> > > > For TCS pages you add both RD and WR maximum protection bits.
> > > > For the enclave to be able to run, user mode will have to change the
> > > > "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise
> > > > eenter fails). This is exactly what your selftest does.
> > >
> > > Recap where the TCS requirements came from? Why does it need
> > > RW in PTEs and can be 0 in the EPCM? The comment should explain
> > > it rather leave it as a claim IMHO.
> >
> > Hardware ignores SECINFO.FLAGS.{R,W,X} coming from userspace and instead
> > forces RWX=0. It does this to prevent software from directly accessing
> > the TCS. But hardware still accesses the TCS through a virtual address,
> > e.g. to allow software to zap the page for reclaim, which means hardware
> > generates reads and writes to the TCS, i.e. the PTEs need RW permissions.
>
> Manipulating a PTE should not require any specific permissions on the
> page that it is defining. Why RW is required in SGX context?
By PTEs I meant the TCS needs to be mapped RW in the kernel's page tables,
e.g. hardware generates read and write accesses to the TCS when entering
an enclave.
> > So, for the EADD ioctl(), it's not unreasonable for userspace to provide
> > SECINFO.FLAGS.{R,W,X} = 0 for the TCS to match what will actually get
> > jammed into the EPCM. Allowing userspace to specify RWX=0 means the
> > kernel needs to manually add PROT_READ and PROT_WRITE to the allowed prot
> > bits so that mmap()/mprotect() work as expected.
>
> Anyway, appreciate your throughtout explanation, thanks.
>
> /Jarkko
>
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Thu, 2019-08-08 at 08:40 -0700, Sean Christopherson wrote:
> On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote:
> > > > + /* TCS pages need to be RW in the PTEs, but can be 0 in the
> > > > EPCM. */
> > > > + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> > > > SGX_SECINFO_TCS)
> > > > + prot |= PROT_READ | PROT_WRITE;
> > >
> > > For TCS pages you add both RD and WR maximum protection bits.
> > > For the enclave to be able to run, user mode will have to change the
> > > "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise
> > > eenter fails). This is exactly what your selftest does.
> >
> > Recap where the TCS requirements came from? Why does it need
> > RW in PTEs and can be 0 in the EPCM? The comment should explain
> > it rather leave it as a claim IMHO.
>
> Hardware ignores SECINFO.FLAGS.{R,W,X} coming from userspace and instead
> forces RWX=0. It does this to prevent software from directly accessing
> the TCS. But hardware still accesses the TCS through a virtual address,
> e.g. to allow software to zap the page for reclaim, which means hardware
> generates reads and writes to the TCS, i.e. the PTEs need RW permissions.
Manipulating a PTE should not require any specific permissions on the
page that it is defining. Why RW is required in SGX context?
> So, for the EADD ioctl(), it's not unreasonable for userspace to provide
> SECINFO.FLAGS.{R,W,X} = 0 for the TCS to match what will actually get
> jammed into the EPCM. Allowing userspace to specify RWX=0 means the
> kernel needs to manually add PROT_READ and PROT_WRITE to the allowed prot
> bits so that mmap()/mprotect() work as expected.
Anyway, appreciate your throughtout explanation, thanks.
/Jarkko
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote:
> > > + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
> > > + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> > > SGX_SECINFO_TCS)
> > > + prot |= PROT_READ | PROT_WRITE;
> >
> > For TCS pages you add both RD and WR maximum protection bits.
> > For the enclave to be able to run, user mode will have to change the
> > "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise
> > eenter fails). This is exactly what your selftest does.
>
> Recap where the TCS requirements came from? Why does it need
> RW in PTEs and can be 0 in the EPCM? The comment should explain
> it rather leave it as a claim IMHO.
Hardware ignores SECINFO.FLAGS.{R,W,X} coming from userspace and instead
forces RWX=0. It does this to prevent software from directly accessing
the TCS. But hardware still accesses the TCS through a virtual address,
e.g. to allow software to zap the page for reclaim, which means hardware
generates reads and writes to the TCS, i.e. the PTEs need RW permissions.
So, for the EADD ioctl(), it's not unreasonable for userspace to provide
SECINFO.FLAGS.{R,W,X} = 0 for the TCS to match what will actually get
jammed into the EPCM. Allowing userspace to specify RWX=0 means the
kernel needs to manually add PROT_READ and PROT_WRITE to the allowed prot
bits so that mmap()/mprotect() work as expected.
>From the SDM:
(* For TCS pages, force EPCM.rwx bits to 0 and no debug access *)
IF (SCRATCH_SECINFO.FLAGS.PT = PT_TCS)
THEN
SCRATCH_SECINFO.FLAGS.R <= 0;
SCRATCH_SECINFO.FLAGS.W <= 0;
SCRATCH_SECINFO.FLAGS.X <= 0;
(DS:RCX).FLAGS.DBGOPTIN <= 0; // force TCS.FLAGS.DBGOPTIN off
DS:RCX.CSSA <= 0;
DS:RCX.AEP <= 0;
DS:RCX.STATE <= 0;
FI;
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
ECPM permissions are mentioned in SDM EADD instruction operation. PTE I don't know. -- Jethro Beekman | Fortanix On 2019-08-07 08:17, Jarkko Sakkinen wrote: On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote: On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote: + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) + prot |= PROT_READ | PROT_WRITE; For TCS pages you add both RD and WR maximum protection bits. For the enclave to be able to run, user mode will have to change the "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise eenter fails). This is exactly what your selftest does. Recap where the TCS requirements came from? Why does it need RW in PTEs and can be 0 in the EPCM? The comment should explain it rather leave it as a claim IMHO. I mean after ~3 weeks of not looking into SGX (because being on vacation etc.) I cannot remember details of this. /Jarkko smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote: > > + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ > > + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == > > SGX_SECINFO_TCS) > > + prot |= PROT_READ | PROT_WRITE; > > For TCS pages you add both RD and WR maximum protection bits. > For the enclave to be able to run, user mode will have to change the > "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise > eenter fails). This is exactly what your selftest does. Recap where the TCS requirements came from? Why does it need RW in PTEs and can be 0 in the EPCM? The comment should explain it rather leave it as a claim IMHO. /Jarkko
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote: > On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote: > > > + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ > > > + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == > > > SGX_SECINFO_TCS) > > > + prot |= PROT_READ | PROT_WRITE; > > > > For TCS pages you add both RD and WR maximum protection bits. > > For the enclave to be able to run, user mode will have to change the > > "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise > > eenter fails). This is exactly what your selftest does. > > Recap where the TCS requirements came from? Why does it need > RW in PTEs and can be 0 in the EPCM? The comment should explain > it rather leave it as a claim IMHO. I mean after ~3 weeks of not looking into SGX (because being on vacation etc.) I cannot remember details of this. /Jarkko
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Mon, Aug 05, 2019 at 09:16:44AM -0700, Sean Christopherson wrote:
> On Sat, Jul 13, 2019 at 08:07:52PM +0300, Jarkko Sakkinen wrote:
> > +static unsigned long sgx_get_unmapped_area(struct file *file,
> > + unsigned long addr,
> > + unsigned long len,
> > + unsigned long pgoff,
> > + unsigned long flags)
> > +{
> > + if (flags & MAP_PRIVATE)
> > + return -EINVAL;
> > +
> > + if (flags & MAP_FIXED)
> > + return addr;
> > +
> > + if (len < 2 * PAGE_SIZE || len & (len - 1))
> > + return -EINVAL;
> > +
> > + addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> > + flags);
> > + if (IS_ERR_VALUE(addr))
> > + return addr;
> > +
> > + addr = (addr + (len - 1)) & ~(len - 1);
> > +
> > + return addr;
> > +}
>
> Thinking about this more, I don't think the driver should verify or adjust
> @addr and @len during non-fixed mmap(). There is no requirement that
> userspace must map the full ELRANGE, or that an enclave's ELRANGE can
> *never* be mapped to non-EPC. The architectural requirement is that an
> access that hits ELRANGE must map to the EPC *if* the CPU is executing the
> associated enclave.
Yeah, well, these were done in a quite different "ecosystem" where range
was the enclave and not like now. Can be considered as cruft that we
have simply missed when moving the fd associated enclaves.
In the current framework of things it definitely makes sense to remove
these calculations.
So, I guess we will end up to:
static unsigned long sgx_get_unmapped_area(struct file *file,
unsigned long addr,
unsigned long len,
unsigned long pgoff,
unsigned long flags)
{
if (flags & MAP_PRIVATE)
return -EINVAL;
if (flags & MAP_FIXED)
return addr;
return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
}
/Jarkko
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
On Sat, Jul 13, 2019 at 08:07:52PM +0300, Jarkko Sakkinen wrote:
> +static unsigned long sgx_get_unmapped_area(struct file *file,
> +unsigned long addr,
> +unsigned long len,
> +unsigned long pgoff,
> +unsigned long flags)
> +{
> + if (flags & MAP_PRIVATE)
> + return -EINVAL;
> +
> + if (flags & MAP_FIXED)
> + return addr;
> +
> + if (len < 2 * PAGE_SIZE || len & (len - 1))
> + return -EINVAL;
> +
> + addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> + flags);
> + if (IS_ERR_VALUE(addr))
> + return addr;
> +
> + addr = (addr + (len - 1)) & ~(len - 1);
> +
> + return addr;
> +}
Thinking about this more, I don't think the driver should verify or adjust
@addr and @len during non-fixed mmap(). There is no requirement that
userspace must map the full ELRANGE, or that an enclave's ELRANGE can
*never* be mapped to non-EPC. The architectural requirement is that an
access that hits ELRANGE must map to the EPC *if* the CPU is executing the
associated enclave.
This means that a process can have random mappings that overlap an
enclave's ELRANGE while the enclave is active, it just can't access that
memory from within the enclave.
A good example is a large enclave, e.g. 3gb of actual code/data, that
userspace wants to locate below 4gb for some reason, e.g. running an
unmodified non-enclave binary under graphene. Requiring @addr+@len to be
naturally sized and aligned will force @addr to 0x0, which is often
disallowed by the kernel.
RE: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
> From: Jarkko Sakkinen
> Sent: Saturday, July 13, 2019 20:08
> Subject: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
> +static long sgx_ioc_enclave_add_page(struct file *filep, void __user
> +*arg) {
> + struct sgx_encl *encl = filep->private_data;
> + struct sgx_enclave_add_page addp;
> + struct sgx_secinfo secinfo;
> + struct page *data_page;
> + unsigned long prot;
> + void *data;
> + int ret;
> +
> + if (copy_from_user(, arg, sizeof(addp)))
> + return -EFAULT;
> +
> + if (copy_from_user(, (void __user *)addp.secinfo,
> +sizeof(secinfo)))
> + return -EFAULT;
> +
> + data_page = alloc_page(GFP_HIGHUSER);
> + if (!data_page)
> + return -ENOMEM;
> +
> + data = kmap(data_page);
> +
> + prot = _calc_vm_trans(secinfo.flags, SGX_SECINFO_R, PROT_READ)
> |
> +_calc_vm_trans(secinfo.flags, SGX_SECINFO_W, PROT_WRITE) |
> +_calc_vm_trans(secinfo.flags, SGX_SECINFO_X, PROT_EXEC);
> +
> + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */
> + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) ==
> SGX_SECINFO_TCS)
> + prot |= PROT_READ | PROT_WRITE;
For TCS pages you add both RD and WR maximum protection bits.
For the enclave to be able to run, user mode will have to change the
"vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise eenter
fails).
This is exactly what your selftest does.
But when mmap (or mprotect) is called with PROT_READ bit, it automatically adds
the PROT_EXEC bit unless the host application has been compiled with '-z
noexecstack' option; pasting below the mmap() code which does it:
if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
if (!(file && path_noexec(>f_path)))
prot |= PROT_EXEC;
The problem is that if PROT_EXEC bit is added then sgx_mmap callback will fail
since PROT_EXEC will get blocked by your code and not allowed for TCS pages.
This restriction is not necessary at all, i.e. I wouldn't block PROT_EXEC on
tcs area because anyway, the hardware will never let those areas to execute:
the SGX protection flags are fixed by the cpu and can not be changed by any
mean.
So in order to facilitate user's interface I would let prot |= PROT_READ |
PROT_WRITE | PROT_EXEC; we do not give up to any security criteria and make
user interaction easier.
> +
> + ret = sgx_encl_page_import_user(data, addp.src, prot);
> + if (ret)
> + goto out;
> +
> + ret = sgx_encl_add_page(encl, addp.addr, data, ,
> addp.mrmask,
> + prot);
> + if (ret)
> + goto out;
> +
> +out:
> + kunmap(data_page);
> + __free_page(data_page);
> + return ret;
> +}
-
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
