Question about the TPM driver

Question about the TPM driver

[Martin Galvan]

Hi all,

I noticed that, after a command is done, the TPM driver only allows
for a single read operation (for a limited time). If I wanted to e.g.
check a command's response code before attempting to read the rest of
the response, my next read would fail. Same happens if I take too long
to read a command's results.

I can understand the timeout, but I'm curious about this single-read
policy, especially since some commands such as GetCapability don't
guarantee how much data will actually be returned. Does anyone know
why it was implemented this way?

Thanks

Re: Question about the TPM driver

[James Bottomley]

On Thu, 2018-09-13 at 10:14 -0300, Martin Galvan wrote:
> Hi all,
> 
> I noticed that, after a command is done, the TPM driver only allows
> for a single read operation (for a limited time). If I wanted to e.g.
> check a command's response code before attempting to read the rest of
> the response, my next read would fail. Same happens if I take too
> long to read a command's results.
> 
> I can understand the timeout, but I'm curious about this single-read
> policy, especially since some commands such as GetCapability don't
> guarantee how much data will actually be returned. Does anyone know
> why it was implemented this way?

Internally the TPM has a single message buffer.  It can't be reused for
the next command until the last one is fully received.  For
efficiency's sake you should have a pending read as soon as the command
is sent so the return of the read tells you the TPM has finished and
the TPM can get on to the next user.  The timeout is merely a courtesy
for the case the command completes before you can queue a read.  It's
short by design otherwise you give users the ability to DoS the TPM.

Exact buffer size hasn't been thought to be an issue since you know
that any response will fit into MAX_RESPONSE_SIZE which the current
implementation defines to be 4096.

James

Re: Question about the TPM driver

[Martin Galvan]

Hi James, thanks for your answer.

El jue., 13 sept. 2018 a las 11:22, James Bottomley
(<James.Bottomley@hansenpartnership.com>) escribio:
>
> On Thu, 2018-09-13 at 10:14 -0300, Martin Galvan wrote:
> Exact buffer size hasn't been thought to be an issue since you know
> that any response will fit into MAX_RESPONSE_SIZE which the current
> implementation defines to be 4096.

Interesting, I didn't know about this max. I guess you're referring to
enum tpm_const's TPM_BUFSIZE constant, since I can't find this max in
the spec. In any case, what would be the effect of removing this
limitation and allowing a user to read the response in chunks rather
than having to allocate 4k every time? I don't think things would
break, though maybe I'm missing something.

Re: Question about the TPM driver

[Javier Martinez Canillas]

[adding Tadeusz Struk to CC list]

On 9/13/18 4:31 PM, Martin Galvan wrote:
> Hi James, thanks for your answer.
> 
> El jue., 13 sept. 2018 a las 11:22, James Bottomley
> (<James.Bottomley@hansenpartnership.com>) escribio:
>>
>> On Thu, 2018-09-13 at 10:14 -0300, Martin Galvan wrote:
>> Exact buffer size hasn't been thought to be an issue since you know
>> that any response will fit into MAX_RESPONSE_SIZE which the current
>> implementation defines to be 4096.
> 
> Interesting, I didn't know about this max. I guess you're referring to
> enum tpm_const's TPM_BUFSIZE constant, since I can't find this max in
> the spec. In any case, what would be the effect of removing this
> limitation and allowing a user to read the response in chunks rather
> than having to allocate 4k every time? I don't think things would
> break, though maybe I'm missing something.
> 

There was an attempt to add partial reads support some time ago [0],
but it was nacked due being an ABI break IIRC.

[0]: https://lkml.org/lkml/2018/7/19/618

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: Question about the TPM driver

[Tadeusz Struk]

On 09/13/2018 08:11 AM, Javier Martinez Canillas wrote:
> There was an attempt to add partial reads support some time ago [0],
> but it was nacked due being an ABI break IIRC.
> 
> [0]: https://lkml.org/lkml/2018/7/19/618

I'm planning to give it another try, based on the feedback,
after the async patch is merged.
Thanks,
-- 
Tadeusz

Re: Question about the TPM driver

[Martin Galvan]

El jue., 13 sept. 2018 a las 12:21, Tadeusz Struk
(<tadeusz.struk@intel.com>) escribio:
> I'm planning to give it another try, based on the feedback,
> after the async patch is merged.
> Thanks,

That's great, thanks. Let me know if I can be of assistance.

Re: Question about the TPM driver

[Ken Goldman]

On 9/13/2018 10:22 AM, James Bottomley wrote:
> 
> Exact buffer size hasn't been thought to be an issue since you know
> that any response will fit into MAX_RESPONSE_SIZE which the current
> implementation defines to be 4096.

If a caller wishes to optimize, TPM 2.0 has these two getcapabilities.

TPM_PT_MAX_COMMAND_SIZE - the maximum value for commandSize in a command
TPM_PT_MAX_RESPONSE_SIZE - the maximum value for responseSize in a response

Re: Question about the TPM driver

[Jarkko Sakkinen]

On Thu, Sep 13, 2018 at 10:14:30AM -0300, Martin Galvan wrote:
> Hi all,
> 
> I noticed that, after a command is done, the TPM driver only allows
> for a single read operation (for a limited time). If I wanted to e.g.
> check a command's response code before attempting to read the rest of
> the response, my next read would fail. Same happens if I take too long
> to read a command's results.
> 
> I can understand the timeout, but I'm curious about this single-read
> policy, especially since some commands such as GetCapability don't
> guarantee how much data will actually be returned. Does anyone know
> why it was implemented this way?
> 
> Thanks

I understand your concerns but without a concrete workload there is no
problem with this behavior.

/Jarkko

Re: Question about the TPM driver

[Martin Galvan]

El dom., 16 sept. 2018 a las 16:16, Jarkko Sakkinen
(<jarkko.sakkinen@linux.intel.com>) escribio:
> I understand your concerns but without a concrete workload there is no
> problem with this behavior.

IMHO it's a bit excessive to allocate 4k to end up storing than 100
bytes. Beyond that, it's a pretty big gotcha for someone who's writing
software which talks to the driver :)

Re: Question about the TPM driver

[James Bottomley]

On Mon, 2018-09-17 at 10:32 -0300, Martin Galvan wrote:
> El dom., 16 sept. 2018 a las 16:16, Jarkko Sakkinen
> (<jarkko.sakkinen@linux.intel.com>) escribio:
> > I understand your concerns but without a concrete workload there is
> > no
> > problem with this behavior.
> 
> IMHO it's a bit excessive to allocate 4k to end up storing than 100
> bytes. Beyond that, it's a pretty big gotcha for someone who's
> writing software which talks to the driver :)

It's what we do in the kernel, which is one of our most memory
constrained environments.

You have to remember that sub page size buffers aren't always managed
the best at any level (they usually fragment the heap) so even in a
constrained memory environment, a 4k buffer (4k aligned) is usually
preferable.

James

Re: Question about the TPM driver

[Jarkko Sakkinen]

On Mon, Sep 17, 2018 at 07:45:32AM -0700, James Bottomley wrote:
> On Mon, 2018-09-17 at 10:32 -0300, Martin Galvan wrote:
> > El dom., 16 sept. 2018 a las 16:16, Jarkko Sakkinen
> > (<jarkko.sakkinen@linux.intel.com>) escribio:
> > > I understand your concerns but without a concrete workload there is
> > > no
> > > problem with this behavior.
> > 
> > IMHO it's a bit excessive to allocate 4k to end up storing than 100
> > bytes. Beyond that, it's a pretty big gotcha for someone who's
> > writing software which talks to the driver :)
> 
> It's what we do in the kernel, which is one of our most memory
> constrained environments.
> 
> You have to remember that sub page size buffers aren't always managed
> the best at any level (they usually fragment the heap) so even in a
> constrained memory environment, a 4k buffer (4k aligned) is usually
> preferable.
> 
> James

If the commit is not too interfering I still could consider merging it as
even if only for simplifying implementing a TPM user space.

/Jarkko

Re: Question about the TPM driver

[Jarkko Sakkinen]

On Mon, Sep 17, 2018 at 10:32:18AM -0300, Martin Galvan wrote:
> El dom., 16 sept. 2018 a las 16:16, Jarkko Sakkinen
> (<jarkko.sakkinen@linux.intel.com>) escribio:
> > I understand your concerns but without a concrete workload there is no
> > problem with this behavior.
> 
> IMHO it's a bit excessive to allocate 4k to end up storing than 100
> bytes. Beyond that, it's a pretty big gotcha for someone who's writing
> software which talks to the driver :)

I remember now the patch from Tadeus. Yeah, it does make sense assuming
we can land a solution that does not cause ABI breaks.

/Jarkko