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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Fri, Aug 4, 2023 at 7:36 AM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxxx>

Disabling vtpm is also reasonable given the reasons Andrew outlined.

Regards,
Jason