Re: [PATCH 0/1] Insert LibSPDM in QEMU enabling in-tree compilation

[Alistair Francis <alistair23@gmail.com>]

On Thu, Oct 17, 2024 at 11:37 PM Ágatha Freitas <htafreit@gmail.com> wrote:
>
>
>
> On Thu, Oct 17, 2024 at 7:00 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Thu, Oct 17, 2024 at 02:00:35PM +1000, Alistair Francis wrote:
>> > On Thu, Oct 17, 2024 at 2:35 AM htafr <htafreit@gmail.com> wrote:
>> > >
>> > > (I) Summary
>> > > ===========================================================================
>> > >
>> > > This patch is the beginning of the support of the Security Protocol and
>> > > Data Model (SPDM). There are some known issues (see II), but it's
>> > > usable and not many users are going to use this functionality for now,
>> > > but for those who will it may facilitate the development.
>> > >
>> > > There are some people working with LibSPDM to implement the SPDM on
>> > > emulated devices, however current works that use QEMU compile LibSPDM
>> > > out-of-tree [1][2][3]. This patch enables the compilation of LibSPDM when
>> > > user pass the parameter '--enable-libspdm' to configure file, this option
>> > > is disabled by default. The following parameters were also added:
>> > >
>> > >   --libspdm-crypto=CHOICE  set LibSPDM crypto algorithm [mbedtls] (choices:
>> > >                            mbedtls/openssl)
>> > >   --libspdm-toolchain=VALUE
>> > >                            toolchain to use for LibSPDM compilation [GCC]
>> > >
>> > > In order to facilitate future code development using LibSPDM API, this
>> > > patch also provides the definition of the macro 'CONFIG_LIBSPDM'.
>> >
>> > We have talked about this before, see
>> > https://patchew.org/QEMU/cover.1691509717.git.alistair.francis@wdc.com/
>> >
>> > The general agreement seemed to be that it will be hard to do SPDM
>> > configuration inside QEMU, hence the external library (like the QEMU
>> > TPM support).
>>
>> More generally, seeing this libspdm proposed for QEMU, without any
>> corresponding usage of it it dubious. It is hard to judge whether
>> it makes any sense, without seeing how it will be used in real
>> device code inside QEMU.
>
>
> Currently, I'm working with EDK2 and QEMU so I have a branch [1] with ongoing
> modifications in files backends/spdm.c and hw/nvme/auth.c. Although the current
> modifications are able to exchange SPDM messages, it's far from being complete
> and it's not following better code practices yet. I'm making use of Alistair's and
> Mallawa's previous work in NVMe to authenticate it through PCI [2].
>
> [1]  WIP: SPDM integration
>       Link: https://github.com/htafr/qemu/tree/libspdm-dev
> [2] WIP: SPDM in OVMF
>       Link: https://github.com/htafr/edk2/tree/ovmf-spdm

I also started working on this, see
https://github.com/tianocore/edk2/pull/5715 .
I was working towards SPDM communication over DOE as well.

Unfortunately it stalled with the EDK2 review process being so incredibly slow

>
>>
>>
>> On the cryptography side, I'm not a fan of linking another
>> crypto library to QEMU, that's different from what we already
>> support in our crypto layer. openssl in particular is a problem
>> due to its licensing - people tend to hand-waive away the
>> licensing incompatibility by pretending openssl is a "system library"
>> but I disagree with that interpretation.
>>
>> > > (II) Known Limitations
>> > > ===========================================================================
>> > >
>> > > 1. This patch enables LibSPDM in-tree compilation for Linux systems only.
>> > > 2. LibSPDM compilation uses CMake, so meson build system is making use
>> > >    of the CMake module [4].
>> > > 3. Some problems may occur when compiling LibSPDM with MbedTls such as:
>> > >     error: "_GNU_SOURCE" redefined [-Werror]
>> > >       10 | #define _GNU_SOURCE
>> > >
>> > >    It's possible to compile using --disable-werror.
>> > >
>> > > (III) Sample configuration
>> > > ===========================================================================
>> > >
>> > > ../configure \
>> > >   --disable-werror \
>> > >   --enable-libspdm \
>> > >   --libspdm-crypto=mbedtls \
>> > >   --enable-gcov
>> > >
>> > > References:
>> > > [1] riscv-spdm
>> > >   Link: https://github.com/htafr/riscv-spdm
>> > > [2] spdm-benchmark
>> > >   Link: https://github.com/rcaalves/spdm-benchmark
>> > > [3] qemu-spdm-emulation-guide
>> > >   Link: https://github.com/twilfredo/qemu-spdm-emulation-guide
>> >
>> > This one has been merged upstream and mainline QEMU supports it now:
>> >
>> > https://www.qemu.org/docs/master/specs/spdm.html
>>
>> So with that merged, is this proposal for linking to libspdm redundant ?
>>
>> With regards,
>> Daniel
>> --
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>>
>
> I'm not sure if I understood the redundancy. Would it be against QEMU practices
> to have another openssl as well as mbedtls linked inside it?
>
> Also, I didn't know the LibSPDM insertion was already discussed previously as
> Alistair pointed out. I think I should have sent this patch as RFC instead. As this is
> my first interaction in any mail list, I'm sorry for any mistakes I made.

No worries, there were no mistakes made. I was just pointing out that
we have had this discussion before and settled on an external SPDM
implementation.

That doesn't mean we can't change that in the future, but I think it
needs a good justification and like Daniel says at least a partial
implementation to go with it

Alistair