Re: [PATCH v2 1/2] misc: Add Nitro Secure Module driver

[Alexander Graf]

Hey Greg,

On 30.09.23 08:20, Greg Kroah-Hartman wrote:
> On Fri, Sep 29, 2023 at 09:26:16PM +0200, Alexander Graf wrote:
> > Hi Arnd!
> >
> > On 29.09.23 19:28, Arnd Bergmann wrote:
> > > On Fri, Sep 29, 2023, at 09:33, Alexander Graf wrote:
> > > > When running Linux inside a Nitro Enclave, the hypervisor provides a
> > > > special virtio device called "NSM". This device has 2 main functions:
> > > >
> > > >     1) Provide attestation reports
> > > >     2) Modify PCR state
> > > >     3) Provide entropy
> > > >
> > > > This patch adds the core NSM driver that exposes a /dev/nsm device node
> > > > which user space can use to request attestation documents and influence
> > > > PCR states. A follow up patch will add a hwrng driver to feed its entropy
> > > > into the kernel.
> > > >
> > > > Originally-by: Petre Eftime <petre.eftime@xxxxxxxxx>
> > > > Signed-off-by: Alexander Graf <graf@xxxxxxxxxx>
> > > Hi Alex,
> > >
> > > I've taken a first look at this driver and have some minor comments.
> >
> > Thanks a bunch!
> >
> >
> > > The main point here is that I think we need to look at possible
> > > alternatives for the user space interface, and (if possible) change
> > > to a set of higher-level ioctl commands from the simple passthrough.
> >
> > I'm slightly torn on that bit. I think in hindsight the NSM device probably
> > should have been a reserved vsock CID and the hwrng one should have just
> > been virtio-rng.
> >
> > The problem is that Nitro Enclaves were launched in 2020 and since an
> > ecosystem developed in multiple languages to support building code inside:
> >
> > https://github.com/aws/aws-nitro-enclaves-nsm-api/blob/main/src/driver/mod.rs#L66
> > https://github.com/donkersgoed/aws-nsm-interface/blob/main/aws_nsm_interface/__init__.py#L264-L274
> >    https://github.com/hf/nsm/blob/main/nsm.go#L99-L129
> >
> >
> > All of these use the (downstream) ioctl that this patch also implements. We
> > could change it, but instead of making it easier for user space to adapt the
> > device node, it would probably hurt more.
> >
> > I agree that this is not a great place to be in. This driver absolutely
> > should have been upstreamed 3 years ago. But I can't turn back time (yet)
> > :).
> As you know, this is no excuse to put an api in the kernel that isn't
> correct or good for the long-term.  Just because people do foolish
> things outside of the kernel tree never means we have to accept them in
> our tree.  Instead we can ask them to fix them properly as part of us
> taking the code.
>
> So please, work on doing this right.


Sorry if my message above came over as a push to put an "incorrect api" 
into the kernel.

In situations like this where you can either give user space full access 
to the device's command space through a generic API or you can create 
command awareness in the kernel and make it the kernel's task to learn 
about each command, IMHO it's never a clear cut on which one is better. 
Especially in virtual environments where the set of commands can change 
quickly over time.

So what I was trying to say above is that *if* we consider both paths 
equally viable, I'd err on the one that enables the existing ecosystem. 
However if there are good reasons to not do command pass-through, I'm 
all for abstracting it away :)

Looking at prior art, the most similar implementations to this are TPMs 
and virtio-vsock. With virtio-vsock, kernel space has no idea what it 
talks to on the other hand and makes it 100% user space's problem. With 
TPMs, you typically use /dev/tpm0 to gain raw command access to the 
target device. So while we could engineer something smarter here, I'm 
not convinced yet it's a net win.


Alex




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879