Re: [edk2-devel] [PATCH] SecurityPkg/Tcg2PhysicalPresenceLib: Fix incorrect TCG VER comparision

["Laszlo Ersek" <lersek@redhat.com>]

On 07/10/20 16:27, Stefan Berger wrote:
> On 7/10/20 9:53 AM, Stefan Berger wrote:
>> On 7/10/20 1:43 AM, Laszlo Ersek wrote:
>>> (+Marc-André, Stefan)
>>>
>>> On 07/10/20 02:44, Gao, Zhichao wrote:
>>>> This bug is not obeserved by me. But I view the code. The condition
>>>> is incorrect and it would affect the TCG operation:
>>>>      if (!mIsTcg2PPVerLowerThan_1_3) {
>>>>          if (OperationRequest <
>>>> TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
>>>>            //
>>>>            // TCG2 PP1.3 spec defined operations that are reserved
>>>> or un-implemented
>>>>            //
>>>>            return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
>>>>          }
>>>>        } else {
>>>>         //
>>>>         // TCG PP lower than 1.3. (1.0, 1.1, 1.2)
>>>>         //
>>>>         if (OperationRequest <= TCG2_PHYSICAL_PRESENCE_NO_ACTION_MAX) {
>>>>           RequestConfirmed = TRUE;
>>>>         } else if (OperationRequest <
>>>> TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
>>>>           return TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED;
>>>>         }
>>>>        }
>>>>
>>> I've found that code myself, but I'm not familiar enough with TPM PPI
>>> stuff to understand immediately the effects of this change. I can see
>>> that where we used to return
>>> TCG_PP_GET_USER_CONFIRMATION_NOT_IMPLEMENTED before, we could now assign
>>> "RequestConfirmed = TRUE", and vice versa, due to
>>> "mIsTcg2PPVerLowerThan_1_3" being potentially inverted.
>>>
>>> But what does that *mean*? What is the behavioral change that human
>>> end-users, or software components, will experience?
>>
>>
>> The above code snipped is located in a default branch of a large
>> switch statement that handles most of the common PPI operations
>> independent of this change, so that at least is good.
>>
>> I would say that in the worst case some of the operations not
>> otherwise handled may have mistakenly failed or could have been
>> executed without user confirmation/interaction. On Linux at least PPI
>> requests can only be sent by root.
> 
> 
> I am running a somewhat dated version of edk2 (Fedora 31). The
> operations advertised are: 0,5,14,21,22,23,24,33,96,97. All of these are
> individually handled in the switch statement, so there should no be any
> impact. I am currently not aware of whether this list can be extended
> with some sort of module.

Thank you!
Laszlo