On 04/08/2023 8:23 am, Jan Beulich wrote:
> On 03.08.2023 22:36, Andrew Cooper wrote:
>> The opensuse-tumbleweed build jobs currently fail with:
>>
>> /builds/xen-project/xen/stubdom/tpm_emulator-x86_64/crypto/rsa.c: In
>> function '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.
> Looking at the code, I'm not sure about this. There could as well have been
> the intention to allow pointers there, then permitting them to be left at
> NULL. Who knows where that code came from originally.
Do you agree that the patch is no function change, or are you saying
that you think I got some of my analysis wrong?
>
>> Adjust it to compile. No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: George Dunlap <george.dun...@eu.citrix.com>
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Wei Liu <w...@xen.org>
>> CC: Julien Grall <jul...@xen.org>
>> CC: Juergen Gross <jgr...@suse.com>
>> CC: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com>
>> CC: Jason Andryuk <jandr...@gmail.com>
>> CC: Daniel Smith <dpsm...@apertussolutions.com>
>> CC: Christopher Clark <christopher.w.cl...@gmail.com>
>>
>> While I've confirmed this to fix the build issue:
>>
>> 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 (which is still abandaonded as of
>> 2018) is just as concerning as the basic error here in rsa_private().
>>
>> vtpm-stubdom isn't credibly component of a Xen system, and we're wasting
>> loads
>> of CI cycles testing it...
> Question is whether people might be using it, and I'm afraid that's a
> question we can't answer. Would it be an alternative to disable vtpm in
> this container's stubdom configure invocation? Obviously as other
> containers have their compilers updated, the same issue may surface
> there then ...
Well that's why I CC'd some of the usual suspects, but all I'm
suggesting (for now) is that we turn it off by default.
~Andrew
