Re: [PATCH v4 1/2] Import CBOR library

[Greg Kroah-Hartman]

On Wed, Oct 11, 2023 at 11:48:43PM +0300, Petre Eftime wrote:
> On Wed, Oct 11, 2023 at 8:46 PM Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Oct 11, 2023 at 02:24:48PM +0200, Arnd Bergmann wrote:
> > > On Tue, Oct 10, 2023, at 10:27, Greg Kroah-Hartman wrote:
> > > > On Tue, Oct 10, 2023 at 10:08:43AM +0200, Alexander Graf wrote:
> > > >> On 10.10.23 10:03, Greg Kroah-Hartman wrote:
> > > >>
> > > >> > > Out of these, the NSM communication protocol uses all except Semantic tags
> > > >> > > and Floats. The CBOR library that this patch imports does not have special
> > > >> > > handling for Semantic tags, which leaves only floats which are already
> > > >> > > #ifdef'ed out. That means there is not much to trim.
> > > >> > >
> > > >> > > What you see here is what's needed to parse CBOR in kernel - if that's what
> > > >> > > we want to do. I'm happy to rip it out again and make it a pure user space
> > > >> > > problem to do CBOR :).
> > > >> > Yes, why are we parsing this in the kernel?  What could go wrong with
> > > >> > adding yet-another-parser in privileged context?  :)
> > > >> >
> > > >> > Why does this have to be in the kernel, the data sent/recieved is over
> > > >> > virtio, so why does the kernel have to parse it?  I couldn't figure that
> > > >> > out from the driver, yet the driver seems to have a lot of hard-coded
> > > >> > parsing logic in it to assume specific message formats?
> > > >>
> > > >>
> > > >> The parsing doesn't have to be in kernel and it probably shouldn't be
> > > >> either. V3 of the patch was punting all the parsing to user space, at which
> > > >> point you and Arnd said I should give it a try to do the protocol parsing in
> > > >> kernel space instead. That's why the parser is here.
> > > >
> > > > Arnd said that, not me :)
> > > >
> > > >> If we conclude that all this in-kernel parsing is not worth it, I'm very
> > > >> happy to just go back to the the v3 ioctl interface and post v5 with hwrng
> > > >> merged into misc, but remove all CBOR logic again :)
> > > >
> > > > I think the less parsers we have in the kernel, the safer we are for
> > > > obvious reasons.  Unless you have a parser for this in rust?  :)
> > > >
> > > > I don't really know, having a generic interface is good, but at the
> > > > expense of this api is probably not good.  individual ioctls might be
> > > > better if there are not going to be any other drivers for this type of
> > > > thing?
> > >
> > > I was definitely expecting something simpler than what was possible
> > > in the v4 patch. I had another look now, and it's clear that the
> > > ioctl interface is still not great because the variable data structures
> > > shine through for some of the calls, and even to get to this point,
> > > a whole lot of complexity is required underneath.
> > >
> > > To get anything better, one would probably have to redesign the entire
> > > interface stack (hypervisor, kernel and userland) to use regular
> > > fixed data structures, and this seems unlikely to happen.
> >
> > Why not fix this and do it properly?  What's preventing that from
> > happening?  We don't want to create an interface here that is broken, or
> > insecure, or a pain to maintain, right?
> >
> > thanks,
> >
> > greg k-h
> 
> I would think the proposal to have fixed structures would be a
> downgrade in terms of maintainability, usability and security, not an
> improvement.
> 
> This current interface allows the hypervisor to extend the existing
> functionality at any time, and the Linux kernel does not have to
> change anything for that to work, the application does not have to be
> recompiled to use the new kernel headers at any point. Adding new
> APIs, adding new fields to API responses, or adding optional
> parameters to the API is fully backwards compatible, would also not
> require changes in userspace, as the CBOR data structures that are not
> recognized can simply be skipped. This allows easy backwards
> compatibility in most cases, and the userspace would be able to opt in
> to new features only if it requires them, without forcing the upgrade
> if it's not required.
> 
> With fixed structures, then the driver would need to be more
> explicitly versioned and would need to be able to handle multiple
> versions of the API at the same time, which is both more complex, less
> flexible and more prone to errors.

Yes, but you are trading off the complexity of adding
yet-another-protocol-parser to the kernel vs. making a strict
user/kernel api, right?

Which one is correct?  Can you verify that this parser really is correct
and doesn't have any overflows/security issues in it?  I can't, maybe
someone needs to write it in rust :)

thanks,

greg k-h