On 07/08/2023 7:56 pm, Jason Andryuk wrote:
> On Fri, Aug 4, 2023 at 7:36 AM Jan Beulich <jbeul...@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 <jandr...@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
