PolicyNV performs signed-magnitude comparison instead of twos-complement

[chrisfenner]

I discovered this issue experimentally working with policyNV in another repository. Filing this issue to create an issue number to reference from test code, which I will point at in a follow-on comment to this issue.

TPM 2.0 Spec, Rev1.59 Part 3, § 23.9.1 "PolicyNV" says:

The signed arithmetic operations are performed using twos-compliment.

As seen here in the implementation of this helper used by PolicyNV:

https://github.com/microsoft/ms-tpm-20-ref/blob/d7a7c200fae3ab947efef8902d219072de94cc3e/TPMCmd/tpm/src/support/MathOnByteBuffers.c#L104-L109

SignedCompareB returns the negative of the comparison of the two values if both are negative. This is how signed-magnitude arithmetic works, but not how twos-complement works.

An example in 16-bit twos-complement:

• -1 = 0xffff
• -2 = 0xfffe
• -1 > -2
• 0xffff > 0xfffe

SignedCompare(0xffff, 0xfffe) as-is will return 0 - UnsignedCompareB(0xffff, 0xfffe), which is -1, which is incorrect (indicating 0xffff < 0xfffe, which is not true in unsigned arithmetic or signed twos-complement arithmetic).

I think SignedCompareB(a, b) should simply return UnsignedCompareB(a, b) if the sign bits are the same.

[chrisfenner]

Failing test cases are commented out in this block of tests for go-tpm's PolicyNV implementation:

https://github.com/chrisfenner/go-tpm/blob/544f65a994f1e36dae02e8f00adb60cc804bae07/tpm2/test/tpm2_test.go#L1575-L1599

[chrisfenner]

An alternative solution I can see is to just roll with the behavior and update the spec to require signed-magnitude behavior here instead of twos-complement. However, the code as written will consider 0 and "negative 0" (0x80..00) not to be equal, so there is still a (more minor) implementation bug we should address.

[josephlr]

@chrisfenner I think this can be closed. Given that https://github.com/microsoft/ms-tpm-20-ref/commit/f16879b67351b39643ebae8611ec14ad043cd37b fixed the issue.

[chrisfenner]

@josephlr You're right. I had thought "well, this can help track the errata/regression test work" but that tracking belongs inside TCG.