On 8/14/23 07:25, George Dunlap wrote:
On Tue, Aug 8, 2023 at 10:27 PM Daniel P. Smith <[email protected] <mailto:[email protected]>> wrote:On 8/3/23 16:36, Andrew Cooper wrote: > The opensuse-tumbleweed build jobs currently fail with: >> /builds/xen-project/xen/stubdom/tpm_emulator-x86_64/crypto/rsa.c: Infunction 'rsa_private':> /builds/xen-project/xen/stubdom/tpm_emulator-x86_64/crypto/rsa.c:56:7: error: the comparison will always evaluate as 'true' for the address of 'p' will never be NULL [-Werror=address]> 56 | if (!key->p || !key->q || !key->u) { > | ^ > In file included from /builds/xen-project/xen/stubdom/tpm_emulator-x86_64/crypto/rsa.c:17:> /builds/xen-project/xen/stubdom/tpm_emulator-x86_64/crypto/rsa.h:28:12: note: 'p' declared here> 28 | tpm_bn_t p; > | ^ > > This is because all tpm_bn_t's are 1-element arrays (of either a GMP or > OpenSSL BIGNUM flavour). The author was probably meaning to do value checks, > but that's not what the code does. > > Adjust it to compile. No functional change. > > Signed-off-by: Andrew Cooper <[email protected] <mailto:[email protected]>> > --- > CC: George Dunlap <[email protected] <mailto:[email protected]>> > CC: Jan Beulich <[email protected] <mailto:[email protected]>> > CC: Stefano Stabellini <[email protected] <mailto:[email protected]>> > CC: Wei Liu <[email protected] <mailto:[email protected]>> > CC: Julien Grall <[email protected] <mailto:[email protected]>> > CC: Juergen Gross <[email protected] <mailto:[email protected]>> > CC: Marek Marczykowski-Górecki <[email protected] <mailto:[email protected]>> > CC: Jason Andryuk <[email protected] <mailto:[email protected]>> > CC: Daniel Smith <[email protected] <mailto:[email protected]>> > CC: Christopher Clark <[email protected] <mailto:[email protected]>> > > While I've confirmed this to fix the build issue: > > https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/955160430 <https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/955160430> > > I'm -1 overall to the change, and would prefer to disable vtpm-stubdom > entirely. > > It's TPM 1.2 only, using decades-old libs, and some stuff in the upstream > https://github.com/PeterHuewe/tpm-emulator <https://github.com/PeterHuewe/tpm-emulator> (which is still abandaonded as of > 2018) is just as concerning as the basic error here in rsa_private(). For semantics sake, the Guest PV interface is 1.2 compliant but the PV backend, vtpmmgr, is capable of using TPM2.0. > vtpm-stubdom isn't credibly component of a Xen system, and we're wasting loads > of CI cycles testing it... Unfortunately, I cannot disagree here. This is the only proper vTPM, from a trustworthy architecture perspective, that I know of existing today. Until I can find someone willing to fund updating the implementation and moving it to being an emulated vTPM and not a PV interface, it is likely to stay in this state for some time.Did you mean "I cannot *agree* here"? "Cannot disagree" means you agree that we're "wasting loads of CI cycles testing it", but the second sentence seems to imply that it's (currently) irreplacable.
The architecture/design is what I don't want to see lost, the implementation itself, IMHO, has severely bit rotted. So what I was trying to say is that while it is an important reference design, I cannot disagree with the sentiment that building the broken implementation is wasting a significant amount of CI resources, both network and CPU time. IOW, I am probably the only one that would potentially make any noise if a patch was submitted to make it default disabled, and I am saying here that I would not do so.
v/r, dps
