| Message ID | 20230803203650.1474936-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | subdom: Fix -Werror=address failure in tmp_emulator | expand |
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. > Adjust it to compile. No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Juergen Gross <jgross@suse.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > CC: Jason Andryuk <jandryuk@gmail.com> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Christopher Clark <christopher.w.clark@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 ... Jan
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@citrix.com> >> --- >> CC: George Dunlap <George.Dunlap@eu.citrix.com> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Wei Liu <wl@xen.org> >> CC: Julien Grall <julien@xen.org> >> CC: Juergen Gross <jgross@suse.com> >> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> CC: Jason Andryuk <jandryuk@gmail.com> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> CC: Christopher Clark <christopher.w.clark@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
On 04.08.2023 11:29, Andrew Cooper wrote: > 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? Oh, I'm sorry for the potentially ambiguous reply: I agree there's no functional change. I'm merely not sure about your guessing of value checks being meant. Jan
On Fri, Aug 4, 2023 at 7:36 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 04.08.2023 11:29, Andrew Cooper wrote: > > 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. The logic looks to be that if p, q, or u are not present, then perform a regular modular exponentiation. If they are available, then perform an optimized Chinese Remainder Theorem exponentiation. In that light, it's written as if pointers were expected. However, the code history doesn't show pointers for p/q/u. An RSA key can't have 0 for p/q/u, so value checks don't make sense. Hmmm, I suppose a 0 value could make sense to indicate uninitialization. tpm_rsa_import_key() and tpm_rsa_generate_key() set p, q, & u. tpm_rsa_copy_key() copies those over. So it should be okay to use this patch to just always use the CRT path. It would be safer to check for 0 though, I suppose. > > Do you agree that the patch is no function change, or are you saying > > that you think I got some of my analysis wrong? > > Oh, I'm sorry for the potentially ambiguous reply: I agree there's no functional > change. I'm merely not sure about your guessing of value checks being meant. Agreed - no functional change. Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Disabling vtpm is also reasonable given the reasons Andrew outlined. Regards, Jason
On 07/08/2023 7:56 pm, Jason Andryuk wrote: > On Fri, Aug 4, 2023 at 7:36 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 04.08.2023 11:29, Andrew Cooper wrote: >>> 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. > The logic looks to be that if p, q, or u are not present, then perform > a regular modular exponentiation. If they are available, then perform > an optimized Chinese Remainder Theorem exponentiation. > > In that light, it's written as if pointers were expected. However, > the code history doesn't show pointers for p/q/u. An RSA key can't > have 0 for p/q/u, so value checks don't make sense. Hmmm, I suppose a > 0 value could make sense to indicate uninitialization. > > tpm_rsa_import_key() and tpm_rsa_generate_key() set p, q, & u. > tpm_rsa_copy_key() copies those over. So it should be okay to use > this patch to just always use the CRT path. It would be safer to > check for 0 though, I suppose. Ok, I'll adjust the commit message. >>> Do you agree that the patch is no function change, or are you saying >>> that you think I got some of my analysis wrong? >> Oh, I'm sorry for the potentially ambiguous reply: I agree there's no functional >> change. I'm merely not sure about your guessing of value checks being meant. > Agreed - no functional change. > > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Thanks. I'll put it in like this to fix CI, then do ... > > Disabling vtpm is also reasonable given the reasons Andrew outlined. ... this separately. ~Andrew
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@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Juergen Gross <jgross@suse.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > CC: Jason Andryuk <jandryuk@gmail.com> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Christopher Clark <christopher.w.clark@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(). 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. v/r, dps
On Tue, Aug 8, 2023 at 10:27 PM Daniel P. Smith < dpsmith@apertussolutions.com> 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@citrix.com> > > --- > > CC: George Dunlap <George.Dunlap@eu.citrix.com> > > CC: Jan Beulich <JBeulich@suse.com> > > CC: Stefano Stabellini <sstabellini@kernel.org> > > CC: Wei Liu <wl@xen.org> > > CC: Julien Grall <julien@xen.org> > > CC: Juergen Gross <jgross@suse.com> > > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > CC: Jason Andryuk <jandryuk@gmail.com> > > CC: Daniel Smith <dpsmith@apertussolutions.com> > > CC: Christopher Clark <christopher.w.clark@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(). > > 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. -George
On 8/14/23 07:25, George Dunlap wrote: > > > On Tue, Aug 8, 2023 at 10:27 PM Daniel P. Smith > <dpsmith@apertussolutions.com <mailto:dpsmith@apertussolutions.com>> 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@citrix.com > <mailto:andrew.cooper3@citrix.com>> > > --- > > CC: George Dunlap <George.Dunlap@eu.citrix.com > <mailto:George.Dunlap@eu.citrix.com>> > > CC: Jan Beulich <JBeulich@suse.com <mailto:JBeulich@suse.com>> > > CC: Stefano Stabellini <sstabellini@kernel.org > <mailto:sstabellini@kernel.org>> > > CC: Wei Liu <wl@xen.org <mailto:wl@xen.org>> > > CC: Julien Grall <julien@xen.org <mailto:julien@xen.org>> > > CC: Juergen Gross <jgross@suse.com <mailto:jgross@suse.com>> > > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com > <mailto:marmarek@invisiblethingslab.com>> > > CC: Jason Andryuk <jandryuk@gmail.com <mailto:jandryuk@gmail.com>> > > CC: Daniel Smith <dpsmith@apertussolutions.com > <mailto:dpsmith@apertussolutions.com>> > > CC: Christopher Clark <christopher.w.clark@gmail.com > <mailto:christopher.w.clark@gmail.com>> > > > > 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
diff --git a/stubdom/Makefile b/stubdom/Makefile index a21e1c3fa3a8..d5fb354e7e37 100644 --- a/stubdom/Makefile +++ b/stubdom/Makefile @@ -243,6 +243,7 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz patch -d $@ -p1 < vtpm_extern.patch patch -d $@ -p1 < vtpm-microsecond-duration.patch patch -d $@ -p1 < vtpm-command-duration.patch + patch -d $@ -p1 < vtpm-tpm_bn_t-addr.patch mkdir $@/build cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement" touch $@ diff --git a/stubdom/vtpm-tpm_bn_t-addr.patch b/stubdom/vtpm-tpm_bn_t-addr.patch new file mode 100644 index 000000000000..53172ae1c244 --- /dev/null +++ b/stubdom/vtpm-tpm_bn_t-addr.patch @@ -0,0 +1,18 @@ +All tpm_bn_t's are a 1-element array of one form or another, meaning the code +below is tautological and triggers -Werror=address. + +diff -ru tpm_emulator-x86_64.orig/crypto/rsa.c tpm_emulator-x86_64/crypto/rsa.c +--- tpm_emulator-x86_64.orig/crypto/rsa.c 2011-12-20 18:30:06.000000000 +0000 ++++ tpm_emulator-x86_64/crypto/rsa.c 2023-08-03 20:44:17.379166284 +0100 +@@ -53,10 +53,7 @@ + tpm_bn_init2(c, key->size); + tpm_bn_import(p, in_len, 1, in); + +- if (!key->p || !key->q || !key->u) { +- /* c = p ^ d mod n */ +- tpm_bn_powm(c, p, key->d, key->n); +- } else { ++ { + tpm_bn_init2(m1, key->size / 2); + tpm_bn_init2(m2, key->size / 2); + tpm_bn_init2(h, key->size);
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@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Juergen Gross <jgross@suse.com> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> CC: Jason Andryuk <jandryuk@gmail.com> CC: Daniel Smith <dpsmith@apertussolutions.com> CC: Christopher Clark <christopher.w.clark@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... --- stubdom/Makefile | 1 + stubdom/vtpm-tpm_bn_t-addr.patch | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 stubdom/vtpm-tpm_bn_t-addr.patch base-commit: 092cae024ab6cd9bd5788eb6ca3ae1a05e796c0a