[intel-sgx-kernel-dev,v4,06/12] fs/pipe.c: export create_pipe_files() and replace_fd()
diff mbox

Message ID 20171016191855.16964-7-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Oct. 16, 2017, 7:18 p.m. UTC
Export create_pipe_files() and replace_fd() so that the SGX driver is
able to create stdin and stdout pipes.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 fs/file.c | 1 +
 fs/pipe.c | 1 +
 2 files changed, 2 insertions(+)

Comments

Jarkko Sakkinen Oct. 19, 2017, 12:36 p.m. UTC | #1
On Thu, Oct 19, 2017 at 01:06:17AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 16, 2017 at 10:18:49PM +0300, Jarkko Sakkinen wrote:
> > Export create_pipe_files() and replace_fd() so that the SGX driver is
> > able to create stdin and stdout pipes.
> 
> Err, no way.

What would be a better alternative?

/Jarkko
Jarkko Sakkinen Oct. 20, 2017, 10:14 a.m. UTC | #2
On Thu, Oct 19, 2017 at 07:55:34AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 19, 2017 at 03:36:16PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 19, 2017 at 01:06:17AM -0700, Christoph Hellwig wrote:
> > > On Mon, Oct 16, 2017 at 10:18:49PM +0300, Jarkko Sakkinen wrote:
> > > > Export create_pipe_files() and replace_fd() so that the SGX driver is
> > > > able to create stdin and stdout pipes.
> > > 
> > > Err, no way.
> > 
> > What would be a better alternative?
> 
> Don't abuse pipes for your interface.

coredump uses pipes in a similar way. Maybe you could elaborate a bit
why this is abusing? That could help me to find something more
acceptable.

The only reasonable alternative that comes into mind would be netlink
socket.

/Jarkko
Dave Hansen Oct. 20, 2017, 2:32 p.m. UTC | #3
I've always been curious, and the changelog and thread are curiously
oblique on this topic: what the heck does this driver use pipes *for*?
Jarkko Sakkinen Oct. 23, 2017, 2:55 a.m. UTC | #4
On Fri, Oct 20, 2017 at 07:32:42AM -0700, Dave Hansen wrote:
> I've always been curious, and the changelog and thread are curiously
> oblique on this topic: what the heck does this driver use pipes *for*?

For communication with the process hosting the launch enclave.

/Jarkko
Dave Hansen Oct. 23, 2017, 5:09 a.m. UTC | #5
On 10/22/2017 07:55 PM, Jarkko Sakkinen wrote:
> On Fri, Oct 20, 2017 at 07:32:42AM -0700, Dave Hansen wrote:
>> I've always been curious, and the changelog and thread are curiously
>> oblique on this topic: what the heck does this driver use pipes *for*?
> For communication with the process hosting the launch enclave.

But, why pipes?  Why does the kernel have to be the one setting these
up?  Why is this communication necessary in the first place?
Jarkko Sakkinen Oct. 24, 2017, 1:39 p.m. UTC | #6
On Sun, Oct 22, 2017 at 10:09:16PM -0700, Dave Hansen wrote:
> On 10/22/2017 07:55 PM, Jarkko Sakkinen wrote:
> > On Fri, Oct 20, 2017 at 07:32:42AM -0700, Dave Hansen wrote:
> >> I've always been curious, and the changelog and thread are curiously
> >> oblique on this topic: what the heck does this driver use pipes *for*?
> > For communication with the process hosting the launch enclave.
> 
> But, why pipes?  Why does the kernel have to be the one setting these
> up?  Why is this communication necessary in the first place?

1. Kernel gives a SIGSTRUCT instance to the LE hosting process.
2. LE hosting process gives the SIGSTRUCT to the LE.
3. LE gives EINITTOKEN to the LE hosting process after generating it.
4. LE hosting process gives it back to the kernel.

I do not understand why using pipes for this is such a big crime to
implement this. I do have an alternative proposal if it is.

What I can do is to use one struct shmem_file instance and assing it
to a file descriptor instead. Kernel and LE hosting process can then
use that for communication.

It would simplify the infrastructure so I will vote that anyhow even if
using pipes would turn out to be acceptable. And does this solution does
not require new exports.

I would still like to hear a better explanation than Christoph gave why
using pipes is a crime and why coredump still uses them if it is.

/Jarkko
Dave Hansen Oct. 24, 2017, 3:10 p.m. UTC | #7
On 10/24/2017 06:39 AM, Jarkko Sakkinen wrote:
> On Sun, Oct 22, 2017 at 10:09:16PM -0700, Dave Hansen wrote:
>> On 10/22/2017 07:55 PM, Jarkko Sakkinen wrote:
>>> On Fri, Oct 20, 2017 at 07:32:42AM -0700, Dave Hansen wrote:
>>>> I've always been curious, and the changelog and thread are curiously
>>>> oblique on this topic: what the heck does this driver use pipes *for*?
>>> For communication with the process hosting the launch enclave.
>>
>> But, why pipes?  Why does the kernel have to be the one setting these
>> up?  Why is this communication necessary in the first place?
> 
> 1. Kernel gives a SIGSTRUCT instance to the LE hosting process.
> 2. LE hosting process gives the SIGSTRUCT to the LE.
> 3. LE gives EINITTOKEN to the LE hosting process after generating it.
> 4. LE hosting process gives it back to the kernel.

Let me see if I can turn that into english.  Enclaves are all rooted in
a chain of trust.  To run an enclave, you need to have that enclave
blessed by the hardware or blessed by a "launch enclave" (aka. LE).  The
LE is hosted inside a normal process, just as the enclave we are trying
to launch is hosted in a normal process.  In order to launch a normal
enclave, we talk to the LE which gives us a token that allows us to
start a new enclave.

These pipes are the mechanism that we use so that the process starting
the new process can talk to the launch enclave.

How's that?

> I do not understand why using pipes for this is such a big crime to
> implement this. I do have an alternative proposal if it is.

The crime is not writing a good changelog to explain what you are doing
and why you need to do it.

> What I can do is to use one struct shmem_file instance and assing it
> to a file descriptor instead. Kernel and LE hosting process can then
> use that for communication.

Could you explain a bit about what kind of information needs to go back
and forth?  Is it just "give me a launch key" followed by "here you go",
or is it more complicated than that?
Jarkko Sakkinen Oct. 24, 2017, 4:40 p.m. UTC | #8
On Tue, Oct 24, 2017 at 08:10:37AM -0700, Dave Hansen wrote:
> On 10/24/2017 06:39 AM, Jarkko Sakkinen wrote:
> > On Sun, Oct 22, 2017 at 10:09:16PM -0700, Dave Hansen wrote:
> >> On 10/22/2017 07:55 PM, Jarkko Sakkinen wrote:
> >>> On Fri, Oct 20, 2017 at 07:32:42AM -0700, Dave Hansen wrote:
> >>>> I've always been curious, and the changelog and thread are curiously
> >>>> oblique on this topic: what the heck does this driver use pipes *for*?
> >>> For communication with the process hosting the launch enclave.
> >>
> >> But, why pipes?  Why does the kernel have to be the one setting these
> >> up?  Why is this communication necessary in the first place?
> > 
> > 1. Kernel gives a SIGSTRUCT instance to the LE hosting process.
> > 2. LE hosting process gives the SIGSTRUCT to the LE.
> > 3. LE gives EINITTOKEN to the LE hosting process after generating it.
> > 4. LE hosting process gives it back to the kernel.
> 
> Let me see if I can turn that into english.  Enclaves are all rooted in
> a chain of trust.  To run an enclave, you need to have that enclave
> blessed by the hardware or blessed by a "launch enclave" (aka. LE).  The
> LE is hosted inside a normal process, just as the enclave we are trying
> to launch is hosted in a normal process.  In order to launch a normal
> enclave, we talk to the LE which gives us a token that allows us to
> start a new enclave.

In a kworker turned in-the ring-3 by the user mode helper framework.

> These pipes are the mechanism that we use so that the process starting
> the new process can talk to the launch enclave.
> 
> How's that?

Client process talks to the kernel with ioctl and passes address and
SIGSTRUCT for an enclave it wants to launch. Kernel talks to the LE
hosting process with pipes currently.

For architectural enclaves such as LE that are bound to the root key
hash you don't need the token. You can pass SGX_ENCLAVE_INIT_ARCH flag
to the SGX_ICO_ENCLAVE_INIT if you are launching such an enclave. That
is what kernel does for LE. There's no recursive loops :-)

Actually I just realized that we do not need SGX_ENCLAVE_INIT_ARCH.
Kernel can deduce this by information inside the SIGSTRUCT and value
of the root key hash that it knows. Much more full proof than a flag.

> > I do not understand why using pipes for this is such a big crime to
> > implement this. I do have an alternative proposal if it is.
> 
> The crime is not writing a good changelog to explain what you are doing
> and why you need to do it.

I will refine the changelog. Hard to find the weak spots before you
expose the code for review :-) Thank you.

> 
> > What I can do is to use one struct shmem_file instance and assing it
> > to a file descriptor instead. Kernel and LE hosting process can then
> > use that for communication.
> 
> Could you explain a bit about what kind of information needs to go back
> and forth?  Is it just "give me a launch key" followed by "here you go",
> or is it more complicated than that?

Input:

1. SHA256 hash of the enclave signer public key modulus
2. SIGSTRUCT containing measurements, signature, modulus of the signer
   etc.

Output:

3. EINITTOKEN which cryptographic token generated with hardware
   generated license key (a different for each boot cycle).

I think shmem_file would be a nice alternative, better than using
pipes for this use case.

/Jarkko

Patch
diff mbox

diff --git a/fs/file.c b/fs/file.c
index 1fc7fbbb4510..b1fa28919b22 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -871,6 +871,7 @@  int replace_fd(unsigned fd, struct file *file, unsigned flags)
 	spin_unlock(&files->file_lock);
 	return err;
 }
+EXPORT_SYMBOL_GPL(replace_fd);
 
 SYSCALL_DEFINE3(dup3, unsigned int, oldfd, unsigned int, newfd, int, flags)
 {
diff --git a/fs/pipe.c b/fs/pipe.c
index 97e5be897753..ee33a84127e7 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -784,6 +784,7 @@  int create_pipe_files(struct file **res, int flags)
 	iput(inode);
 	return err;
 }
+EXPORT_SYMBOL_GPL(create_pipe_files);
 
 static int __do_pipe_flags(int *fd, struct file **files, int flags)
 {