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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/08/2023 7:56 pm, Jason Andryuk wrote:
> 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.

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@xxxxxxxxx>

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