tpm2: fix SignedCompareB

[JuergenReppSIT]

SigedCompare returns -1 for the following call:

BYTE a = 0x81;
BYTE b = 0x82;
SignedCompareB(1, &a, 1, &b);

But should return 1, because signed 0x81 > 0x82

Signed-off-by: Juergen Repp [email protected]

[stefanberger]

Can you leave this part here untouched so it remains as close to the specs as possible.

[stefanberger]

Thanks for the PR. I would not have found this bug. Luckily no release has been made that includes the recent change to this code. You are basically reverting this patch here: 6ac04e4

Basically you are saying that the code in rev164 is wrong [pdf page 572 ]? : https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p64_Part4_SupportingRoutines_code_15may2021.pdf

[JuergenReppSIT]

I downloaded the spec and found the code for SignedCompare at page 543. This code seems to be ok but is different from:

libtpms/src/tpm2/MathOnByteBuffers.c

Line 105 in 0c32f39

SignedCompareB(

[stefanberger]

SigedCompare returns -1 for the following call:

BYTE a = 0x81; BYTE b = 0x82; SignedCompareB(1, &a, 1, &b);

But should return 1, because signed 0x81 > 0x82

With two's complement (are these number's in two's complement):

a = 0x81 = -127 and b = 0x82 = -126

a < b, therefore it should be '-1'.

#include <stdint.h>
#include <stdio.h>

#define LIB_EXPORT

typedef uint32_t UINT32;
typedef char BYTE;

LIB_EXPORT int
UnsignedCompareB(
                 UINT32           aSize,         // IN: size of a
                 const BYTE      *a,             // IN: a
                 UINT32           bSize,         // IN: size of b
                 const BYTE      *b              // IN: b
                 )
{
    UINT32             i;
    if(aSize > bSize)
        return 1;
    else if(aSize < bSize)
        return -1;
    else
        {
            for(i = 0; i < aSize; i++)
                {
                    if(a[i] != b[i])
                        return (a[i] > b[i]) ? 1 : -1;
                }
        }
    // Will return == if sizes are both zero
    return 0;
}
/* 9.11.2.2 SignedCompareB() */
/* Compare two signed integers: */
/* Return Values Meaning */
/* 1 if a > b */
/* 0 if a = b */
/* -1 if a < b */
int
SignedCompareB(
               const UINT32     aSize,         // IN: size of a
               const BYTE      *a,             // IN: a buffer
               const UINT32     bSize,         // IN: size of b
               const BYTE      *b              // IN: b buffer
               )
{
#if 1
    // are the signs different ?
    if(((a[0] ^ b[0]) & 0x80) > 0)
        // if the signs are different, then a is less than b if a is negative.
        return a[0] & 0x80 ? -1 : 1;
    else
        // do unsigned compare function
        return UnsignedCompareB(aSize, a, bSize, b);
#else
    int      signA, signB;       /*  sign of a and b */
    /*  For positive or 0, sign_a is 1 */
    /*  for negative, sign_a is 0 */
    signA = ((a[0] & 0x80) == 0) ? 1 : 0;
    /*  For positive or 0, sign_b is 1 */
    /*  for negative, sign_b is 0 */
    signB = ((b[0] & 0x80) == 0) ? 1 : 0;
    if(signA != signB)
    {
        return signA - signB;
    }
    if(signA == 1)
        /*  do unsigned compare function */
        return UnsignedCompareB(aSize, a, bSize, b);
    else
        /*  do unsigned compare the other way */
        return 0 - UnsignedCompareB(aSize, a, bSize, b);
#endif
}

void main(void)
{
    printf("%d  -1\n", SignedCompareB(1, (BYTE[]){0x1}, 1, (BYTE[]){0x2}));
    printf("%d   0\n", SignedCompareB(1, (BYTE[]){0x1}, 1, (BYTE[]){0x1}));
    printf("%d   1\n", SignedCompareB(1, (BYTE[]){0x2}, 1, (BYTE[]){0x1}));

    // 0x81 = -127; // 0x82 = -126
    //printf("%02x   0x81\n", (BYTE)-127);
    //printf("%02x   0x82\n", (BYTE)-126);
    printf("%d  -1\n", SignedCompareB(1, (BYTE[]){0x81}, 1, (BYTE[]){0x82}));
    printf("%d   0\n", SignedCompareB(1, (BYTE[]){0x81}, 1, (BYTE[]){0x81}));
    printf("%d   1\n", SignedCompareB(1, (BYTE[]){0x82}, 1, (BYTE[]){0x81}));

    printf("%d  -1\n", SignedCompareB(1, (BYTE[]){0x81}, 1, (BYTE[]){0x2}));
    printf("%d   0\n", SignedCompareB(1, (BYTE[]){0x2}, 1, (BYTE[]){0x2}));
    printf("%d   1\n", SignedCompareB(1, (BYTE[]){0x2}, 1, (BYTE[]){0x82}));
}

For the current algorithm this results in (values I would expected in 2nd column):

-1  -1
0   0
1   1
-1  -1
0   0
1   1
-1  -1
0   0
1   1

For the old and your proposed algorithm (basically a revert to the old one):

-1  -1
0   0
1   1
1  -1
0   0
-1   1
-1  -1
0   0
1   1

The REAL issue here is, what all did this change break?

[JuergenReppSIT]

With the new libtpms the test policynv in the tpm2-tools ci which always worked failed. Here a short example for this test in the ci:

operandA=0x81
nv_test_index=0x01500001
operandB=0x82
tpm2 startauthsession -S session.ctx --policy-session
tpm2 nvdefine -C o -p nvpass $nv_test_index -a "authread|authwrite" -s 1
echo $operandA | xxd -r -p | tpm2 nvwrite -P nvpass -i- $nv_test_index
echo $operandB | xxd -r -p |tpm2 policynv -S session.ctx -i- -P nvpass $nv_test_index sgt

This test, which also works with a AMD firmware TPM, now did return 0x126 if the new libtpms was used.
I compared your SignedCompareB with the SignedCompareB from spec 1.38 and saw that the spec version did return 1 for the test case and therefore created the PR.

[AndreasFuchsTPM]

Seems to be in the TPM Errata: https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-Library-Spec-v1.59-Errata-v1.4_pub.pdf
Section "2.5 TPM_EO – two’s complement [code]"

[JuergenReppSIT]

@stefanberger thank you for explaining the signed comparisons in such detail. I will adapt the tpm tool tests in the CI to the libtpms with the fix for SignedCompareB. And I think the PR can be closed.

[stefanberger]

@stefanberger thank you for explaining the signed comparisons in such detail. I will adapt the tpm tool tests in the CI to the libtpms with the fix for SignedCompareB. And I think the PR can be closed.

It looks like we may run into backwards compatibility problems at some point where some policies created on older TPMs/libtpms won't work on newer TPM versions / libtpms.

[JuergenReppSIT]

The values for signed compare will be compatible with the new libtpms. We will check the return codes for signed compares and skip these tests if 0x126 is returned to finally allow prs with successful tests into the ci again.

[stefanberger]

The values for signed compare will be compatible with the new libtpms. We will check the return codes for signed compares and skip these tests if 0x126 is returned to finally allow prs with successful tests into the ci again.

Tests are not so much my concern, user's policies are more of a concern.

[JuergenReppSIT]

It looks like we may run into backwards compatibility problems at some point where some policies created on older TPMs/libtpms won't work on newer TPM versions / libtpms.

yes that's really an unpleasant thing but not avoidable?

[stefanberger]

It looks like we may run into backwards compatibility problems at some point where some policies created on older TPMs/libtpms won't work on newer TPM versions / libtpms.

yes that's really an unpleasant thing but not avoidable?

By leaving it broken it could be avoided. It can cause major headaches on any number of machines.

[AndreasFuchsTPM]

It looks like we may run into backwards compatibility problems at some point where some policies created on older TPMs/libtpms won't work on newer TPM versions / libtpms.

yes that's really an unpleasant thing but not avoidable?

By leaving it broken it could be avoided. It can cause major headaches on any number of machines.

IMHO, the TCG TPM reference code will include this fix for the next "actual release" and also actual TPMs start implementing the correct(ed) behavior.
At some point, we will have swallow the pill anyhow, and now is as good a time as anytime else.

[kgoldman]

See Part 2. There is at least a NOTE:

6.8 TPM_EO (EA Arithmetic Operands)
Table 18 defines the TPM_EO constants.
The signed arithmetic operations are performed using two’s complement.
NOTE For two negative values, TPMs prior to revision 1.66 might have implemented the signed arithmetic operations using sign-magnitude.