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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/14/23 07:25, George Dunlap wrote:
> On Tue, Aug 8, 2023 at 10:27 PM Daniel P. Smith 
> <dpsmith@xxxxxxxxxxxxxxxxxxxx <mailto:dpsmith@xxxxxxxxxxxxxxxxxxxx>> 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: 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.
>      >
>      > Adjust it to compile.  No functional change.
>      >
>      > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx
>     <mailto:andrew.cooper3@xxxxxxxxxx>>
>      > ---
>      > CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx
>     <mailto:George.Dunlap@xxxxxxxxxxxxx>>
>      > CC: Jan Beulich <JBeulich@xxxxxxxx <mailto:JBeulich@xxxxxxxx>>
>      > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx
>     <mailto:sstabellini@xxxxxxxxxx>>
>      > CC: Wei Liu <wl@xxxxxxx <mailto:wl@xxxxxxx>>
>      > CC: Julien Grall <julien@xxxxxxx <mailto:julien@xxxxxxx>>
>      > CC: Juergen Gross <jgross@xxxxxxxx <mailto:jgross@xxxxxxxx>>
>      > CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx
>     <mailto:marmarek@xxxxxxxxxxxxxxxxxxxxxx>>
>      > CC: Jason Andryuk <jandryuk@xxxxxxxxx <mailto:jandryuk@xxxxxxxxx>>
>      > CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx
>     <mailto:dpsmith@xxxxxxxxxxxxxxxxxxxx>>
>      > CC: Christopher Clark <christopher.w.clark@xxxxxxxxx
>     <mailto:christopher.w.clark@xxxxxxxxx>>
>      >
>      > 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