Re: [PATCH] fix(macro): interpret responses dynamically

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Sep 02, 2025 at 07:54:55PM +0300, Jarkko Sakkinen wrote:
> Here's the failing case:
> 
> 8001000000300000000002000000002000647915de6106c955b26456b8b8a3b10546fa446405d4eb2e1fb0247fb52080
> 
> What happens is that `tpm_parse_response`  returns `ParserUnderflow`, and
> parsing fails (gracefully).
> 
> The root cause is incorrect interpretation of commands being applicable
> both with and without sessions. In the failing case the response is
> captured from a run of `tpm2sh start-session`.
> 
> It has hard to define the failing commit since this project barely has
> kicked off, and the recent history is encompassed of major overhauls in
> order to get where it is right now but most likely the failure was caused
> by when refactoring away from having `tpm_response!` in favor of universal
> `tpm_struct!`.
> 
> I highlight the parts of the diff that are relevant for addressing the
> issue:
> 
> First an analogous trait to `TpmCommandBodyParse` is defined for delivering
> the "discovered" tag in the different layers of the response parser:
> 
> pub(crate) trait TpmResponseBodyParse: Sized {
>     /// Parses the response body from a buffer, using the response tag to determine
>     /// the structure.
>     fn parse_body(tag: data::TpmSt, buf: &[u8]) -> TpmResult<(Self, &[u8])>;
> }
> 
> Then the trait call is patched to `tpm_dispatch!`:
> 
> -                    |buf| {
> -                        <$rsp_ty>::parse(buf)
> +                    |tag, buf| {
> +                        <$rsp_ty as $crate::message::TpmResponseBodyParse>::parse_body(tag, buf)
> 
> The dispatcher generated is orchestrated from the "entry function" i.e,
> `tpm_parse_response`, so that's where the tag is originally supplied
> after doing a binary search to `PARSE_RESPONSE_MAP`:
> 
> -    let (body, mut session_area) = (dispatch.2)(body_buf)?;
> +    let (body, mut session_area) = (dispatch.2)(tag, body_buf)?;
> 
> Finally, `tpm_struct!` is mirgated to provide the trait implementation:
> 
> -            fn parse(buf: &[u8]) -> $crate::TpmResult<(Self, &[u8])> {
> +            fn parse_body(
> +                tag: $crate::data::TpmSt,
> +                buf: &[u8],
> 
> And that's how tag is delivered to the new response body parser for
> implementing the actual upgraded logic:
> 
> -                if $with_sessions {
> -                    let (size, buf_after_size) = u32::parse(cursor)?;
> +                if $with_sessions && tag == $crate::data::TpmSt::Sessions {
> 
> This fixes the bug and as is proven against the failing case by
> `test_response_start_auth_session_no_sessions_2`.
> 
> However, this revealed a "symmetrical bug" in tpm_build_response()
> leading to a failure in the final comparison of the test case.
> 
> This led to the new `TpmResponseBodyBuild`, which matches the logic
> with already pre-existing `TpmCommandBodyBuild`, and fixes essentially
> `fn len()`.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Since tpm2sh is a separate project I was a bit stressed whether the
problem is fixed after releasing 0.10.21 of the stable 0.10.x branch
but it clearly does:

❯ sudo RUST_TRACE=full RUST_LOG=trace target/debug/tpm2sh  start-session
[sudo] password for jarkko:
[2025-09-02T17:05:31.895688Z TRACE cli::device] Command: 80010000003b0000017640000007400000070020c74f3cf4a9a62efe747457f8e8af8cd0bfb90a3725d62944a73002600617fd0e0000000010000b
[2025-09-02T17:05:31.929485Z TRACE cli::device] Response: 800100000030000000000200000000208da38751c181fb84262d4e2dac59610fe5f14d5d177ecc3544afcb0ac8b47671
tpm://0x02000000

This also verifies that `test_response_start_auth_session_no_sessions_2`
is capturing the bug realistically, and it is factors less likely that
the bug will retrigger.

BR, Jarkko