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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h