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

[Arnd Bergmann]

On Mon, Oct 2, 2023, at 14:28, Alexander Graf wrote:
> On 30.09.23 08:20, Greg Kroah-Hartman wrote:
>> On Fri, Sep 29, 2023 at 09:26:16PM +0200, Alexander Graf wrote:
>>> On 29.09.23 19:28, Arnd Bergmann wrote:

>>> 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.

Generally speaking, I can see a number of advantages to using an
in-kernel abstraction:

- if there are both in-kernel and userspace API users, or multiple
  concurrent userspace clients, an abstraction layer helps to serialize
  between any stateful commands.

- in an abstract interface, the kernel can enforce command specific
  permission checks, rather than allowing access either to all or none
  of the commands.

- having the actual commands created by the kernel means that a bug
  in the virtio device implementation parsing the commands is less
  likely to be exploitable from user space.

- An explicit set of defined ioctl commands is easier to review and
  audit for kernel developers as we try to ensure that this is a
  sensible kernel interface

I don't know enough about your use cases or the specific
command set to tell if any of those points actually matter
here. In the python implementation you linked to, there are
only a handful of commands that actually get passed through.
It should be fairly easy to prototype a kernel driver that
implements them as individual ioctl commands, to give us a
better idea of how this compares to the current code.

     Arnd