RE: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind operations

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nikolay Borisov <nik.borisov@xxxxxxxx>
>Subject: Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind
>operations
>
>
>
>On 14.10.26 г. 6:06 ч., Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Nikolay Borisov <nik.borisov@xxxxxxxx>
>>> Subject: Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind
>>> operations
>>>
>>>
>>>
>>> On 24.09.26 г. 7:10 ч., Zhenzhong Duan wrote:
>>>> Introduce support for binding and unbinding the Trust Device Interface
>>>> (TDI) using the TDCM_OP_BIND and TDCM_OP_UNBIND commands.
>>>>
>>>> Executing TDCM_OP_BIND transitions the TDI state to a locked state
>>>> (TDI_STATE_CONFIG_LOCKED). Add a sanity check following the bind
>operation
>>>> to verify this state transition succeeded before finalizing device locking.
>>>>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
>>>
>>> <snip>
>>>
>>>>    	struct tdx_devsec *tdevsec __free(kfree) = kzalloc(sizeof(*tdevsec),
>>> GFP_KERNEL);
>>>> @@ -174,6 +233,19 @@ static struct pci_tsm *tdx_devsec_lock(struct
>tsm_dev
>>> *tsm_dev, struct pci_dev *
>>>>    	if (!tdx_is_dev_teeio_support(tdevsec))
>>>>    		return ERR_PTR(-EOPNOTSUPP);
>>>>
>>>> +	struct tdx_devsec *tdevsec_bind __free(tdx_tdi_unbind_dev) =
>>> tdx_tdi_bind_dev(tdevsec);
>>>> +	if (IS_ERR(tdevsec_bind))
>>>> +		return ERR_CAST(tdevsec_bind);
>>>> +
>>>> +	ret = tdx_tdi_read_state(tdevsec, &state);
>>>> +	if (ret)
>>>> +		return ERR_PTR(ret);
>>>> +
>>>> +	if (state != TDI_STATE_CONFIG_LOCKED)
>>>> +		return ERR_PTR(-EIO);
>>>
>>> While this does seem correct, it's not really following the spec or I'm being too
>>> rigid, namely:
>>>
>>> Since TDI can change the state any time, TD can't fully rely on the Known RUN
>>> state in any scenario. However, TD can rely on the Known
>>> RUN state after TDG.TDI.START as a confirmation of the Interface Start.
>>
>> About TDI changing state any time, do you mean host changes TDI state,
>> e.g., through TDH.TDI.STOP? Or you mean TDI device goes into TDISP error state
>> due to hw error? Can't they happen even after TDG.TDI.START?
>
>I've taken this verbatim from the latest TDX connect spec, I'd assume
>what it refers to is the TDI/device getting into error state because the
>DSM deemed so because of whatever reason. I believe there is an inherent
>TOCTOU race here, because you can at best get a momentary snapshot of
>the state but the DSM, which is inherently an independent entry could
>for whatever reason decide to enter in error state (highly unlikely, but
>still possible!)

Agreed, it's a best effort check, error state can happen at any time.

>
>>
>>>
>>> But here we still haven't done a START, just a BIND, no ?
>>
>> Yes
>>
>
><snip>
>
>>
>> I understand this as a best-effort sanity check, not a guarantee that
>> the interface will remain in this state.
>>
>> For example, suppose the host creates the TDI but incorrectly reports
>> BIND complete without completing TDH.TDI.BIND. In the module
>> implementation I’m looking at, TDG.TDI.RD(INTERFACE_STATE) returns the
>> stored TDISP state without requiring the internal TDI state to be BOUND.
>> A successful read could therefore return CONFIG_UNLOCKED.
>>
>> At this point we have bound the interface but have not started it, so
>> CONFIG_LOCKED is the state we expect. Checking it catches that
>> inconsistency; checking only the TDCALL return code would not. This is
>> still a snapshot, not proof against a subsequent state change.
>
>Makes sense, IIUC it states the same thing I stated above i.e a
>snapshot, so perhaps this check could be carved in a different patch
>with the changelog containing this reasoning?

OK for me.