Re: [PATCH] subdom: Fix -Werror=address failure in tmp_emulator

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.cooper3@xxxxxxxxxx>
>> ---
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>> CC: Jason Andryuk <jandryuk@xxxxxxxxx>
>> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> CC: Christopher Clark <christopher.w.clark@xxxxxxxxx>
>>
>> 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