Re: [Qemu-devel] [PATCH V20 6/8] Add support for cancelling of a TPM command

[Stefan Berger]

On 02/04/2013 11:02 AM, Corey Bryant wrote:
> @@ -221,7 +243,24 @@ static void 
> tpm_passthrough_deliver_request(TPMBackend *tb)
> >   static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
> >   {
> > -    /* cancelling an ongoing command is known not to work with some 
> > TPMs */
> > +    TPMPassthruState *tpm_pt = tb->s.tpm_pt;
> > +    int n;
> > +
> > +    /*
> > +     * As of Linux 3.7 the tpm_tis driver does not properly cancel
> > +     * commands for all TPMs.
>
> Any idea what the plan is for this issue?  Is it an ongoing matter of 
> adding kernel support as unsupported TPMs are identified?


I submitted a patch to the tpmdd-devel mailing list fixing the 
cancellation issue with the particular TPM I have on one of my machines. 
Kent Yoder modified it to add a fix for yet another TPM (from a 
different vendor).

https://github.com/shpedoikal/linux/commit/9f11986de7280f999cad342389b48c29002c0f37

The spec seems not be clear enough as to what bits must be set in the 
status register when a TPM command is canceled so that the so far 
implemented cancellation detection is insufficient and needs to be 
adapted to TPM vendors' implementations. All TIS interfaces are supposed 
to support cancellation, though.

> > +     * Only cancel if we're busy so we don't cancel someone else's
> > +     * command, e.g., a command executed on the host.
> > +     */
> > +    if (tpm_pt->cancel_fd >= 0 && tpm_pt->tpm_executing) {
> > +        n = write(tpm_pt->cancel_fd, "-", 1);
> > +        if (n != 1) {
> > +            error_report("Canceling TPM command failed: %s\n",
> > +                         strerror(errno));
> > +        } else {
> > +            tpm_pt->tpm_op_canceled = true;
> > +        }
> > +    }
>
> Would an informational message make sense here for unsupported TPMs 
> (when tpm_pt->cancel_fd < 0)?

Sure, I can add that.


> > +        snprintf(path, sizeof(path), "/sys/class/misc/tpm%u/device", 
> > i);
> > +        if (!tpm_passthrough_check_sysfs_cancel(path, sizeof(path))) {
> > +            continue;
> > +        }
> > +        fd = open(path, O_WRONLY);
> > +        break;
> > +    }
> > +
> > +exit:
> > +    if (fd >= 0) {
> > +        tb->cancel_path = g_strdup(path);
> > +    }
> > +
> > +    return fd;
> > +}
> > +
>
> A general question about the function above.  I see that 
> "/sys/class/misc/tpm%u/device" will be explained in kernel 
> documentation here:
>
> https://github.com/shpedoikal/linux/commit/4e21d66c9efbe740b5655bcf66e39873e354f8e9 
>
>
> And the following paths apparently have the same behavior.  But are 
> these paths defined somewhere else?
> "/sys/devices/pnp%u/%s"
> "/sys/devices/platform/tpm_tis"
>
> Also I think a comment pointing to documentation would be a 
> worth-while addition to the code.

The /sys/class/misc/tpm%u/device path seems to be always there 
independent of what type of device the TPM is registered as (pnp or 
platform). So I'll modify above code to always use that path instead.

  Stefan