Re: x86/sgx: v23-rc2

[Jethro Beekman <jethro@fortanix.com>]

[-- Attachment #1: Type: text/plain, Size: 3765 bytes --]

On 2019-10-17 19:57, Sean Christopherson wrote:
> +Cc Andy
> 
> On Mon, Oct 14, 2019 at 08:43:09AM +0000, Jethro Beekman wrote:
>> On 2019-10-11 20:15, Sean Christopherson wrote:
>>> On Fri, Oct 11, 2019 at 04:37:25PM +0000, Jethro Beekman wrote:
>>>> UAPI:
>>>>
>>>> This got a whole lot more complex for userspace compared to the out-of-tree
>>>> driver.
>>>>
>>>> 1. Manually needing to mmap a naturally-aligned memory region by allocating
>>>> too much memory and then unmapping parts is quite annoying. Why was the
>>>> auto-aligning removed? I think this will need to be handled the same for
>>>> every consumer of SGX, so I don't see why this is not handled in the kernel.
>>>> It never seems wrong to align if NULL is passed as the requested address.
>>>> Alternatively, is there room in the flags for a MAP_ALIGNED bit?
>>>
>>> I'm pretty sure everyone agrees it's annoying.  The short of it is that
>>> the SGX driver is the wrong place to do the alignment.  The driver could
>>> key off addr=0, but we don't want to take on that implicit behavior.
>>
>> Why not?
> 
> Because it's a hack.  If a MAP_ALIGNED flag is added then SGX is stuck
> with kludgy code that serves no purpose.  And userspace needs to manually
> align the result if it provides an actual hint.  Regardless of whether
> there are use cases for providing a hint for ELRANGE, having divergent
> logic is ugly.
> 
>>> A MAP_ALIGNED flag to have the allocation be naturally aligned is the
>>> ideal solution.  It's definitely something we should pursue, but that can
>>> and probably should be done in parallel to the SGX series.
>>>
>>>> 2. Having to re-open the device for every enclave is also annoying. This
>>>> means you need a filesystem available throughout the process lifetime. I
>>>> tried dup, but that doesn't work. Can we make dup work?
>>>
>>> The semantics of dup() won't get you what want, as dup() just creates a
>>> new descriptor pointing at the same file.
>>>
>>> An alternative solution that was proposed was to have an ioctl() for
>>> creating an enclave.  But that means using an anonymous inode, which runs
>>> afoul of SELinux permissions, e.g. every _process_ that runs enclaves
>>> would require EXECMEM.  Linus was quite clear that SGX wouldn't be merged
>>> if using it required users to degrade existing security.
>>
>> It's ok if it's the same inode, it just needs to be a different struct file.
>>
>>> I'm open to other ideas.  I wasn't aware this was a pain point and file
>>> stuff isn't exactly my area of expertise, so I haven't put much/any
>>> thought into alternatives.
>>
>> The default permissions for /dev/sgx/enclave are root-only. This means you
>> want to be able to do the same thing as network servers: initialize some
>> resources as root, then drop privileges. This used to mean opening /dev/sgx
>> and keeping the fd around which meant you could launch enclaves at will. With
>> the new API, this is no longer possible, you can only launch one enclave per
>> fd. Is there a different type of operation that doesn't just duplicate the fd
>> but also the struct file? If not, can we add an ioctl for that?
> 
> My approach to this would be to chown /dev/sgx/enclave so that it's owned
> by root but accessible to users belonging to an sgx-specific group, e.g.
> via a udev rule.
> 
>> There are other scenarios where it's not just the permissions on
>> /dev/sgx/enclave that are the problem but using the filesystem in general
>> that is. Maybe you've used seccomp to disable file operations, etc.
> 
> Andy and Jarkko, thoughts?

Folks, any more thoughts on how to resolve the issue that you need to call open() for every enclave?

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]