subdom: Fix -Werror=address failure in tmp_emulator

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

Posted by Andrew Cooper 8 months, 4 weeks ago

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

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);

base-commit: 092cae024ab6cd9bd5788eb6ca3ae1a05e796c0a
-- 
2.30.2

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

Posted by Daniel P. Smith 8 months, 3 weeks ago

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

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

Posted by George Dunlap 8 months, 2 weeks ago

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

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

Posted by Daniel P. Smith 8 months, 2 weeks ago

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

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

Posted by Jan Beulich 8 months, 4 weeks ago

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

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

Posted by Andrew Cooper 8 months, 4 weeks ago

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

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

Posted by Jan Beulich 8 months, 4 weeks ago

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

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

Posted by Jason Andryuk 8 months, 3 weeks ago

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

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

Posted by Andrew Cooper 8 months, 3 weeks ago

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