Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Mon, Jan 16, 2017 at 06:58:44AM -0800, James Bottomley wrote:
> On Mon, 2017-01-16 at 16:48 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 16, 2017 at 06:24:48AM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-16 at 11:09 +0200, Jarkko Sakkinen wrote:
> > > > On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> > > > > On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > > > > > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct
> > > > > > device
> > > > > > *pdev,
> > > > > > chip->cdev.owner = THIS_MODULE;
> > > > > > chip->cdev.kobj.parent = &chip->dev.kobj;
> > > > > >
> > > > > > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > > > > > GFP_KERNEL);
> > > > > > + if (!chip->work_space.context_buf) {
> > > > > > + rc = -ENOMEM;
> > > > > > + goto out;
> > > > > > + }
> > > > > > +
> > > > >
> > > > > I think the work_buf handling can be greatly simplified by
> > > > > making it a pointer to the space: it's only usable between
> > > > > tpm2_prepare_space() and tpm2_commit_space() which are
> > > > > protected by the chip mutex, so there's no need for it to exist
> > > > > outside of these calls (i.e. it can be NULL).
> > > > >
> > > > > Doing it this way also saves the allocation and copying
> > > > > overhead of work_space.
> > > > >
> > > > > The patch below can be folded to effect this.
> > > >
> > > > Hey, I have to take my words back. There's a separate buffer for
> > > > space for a reason. If the transaction fails for example when RM
> > > > is doing its job, we can revert to the previous set of transient
> > > > objects.
> > > >
> > > > Your change would completely thrawt this. I tried varius ways to
> > > > heal when RM decorations fail and this is the most fail safe to
> > > > do it so lets stick with it.
> > >
> > > That's why I added the return code check in the other patch: if the
> > > command fails in the TPM, the space state isn't updated at all, the
> > > net result being that nothing changes in the space, thus you don't
> > > need the copy, because there's nothing to revert on a failure.
> >
> > You are right in what you say but what if you save lets say 5
> > transient contexts and ContextSave fails on 2nd. It's not for the
> > command itself but for falling back to a sane state when
> > tpm2_commit_space fails (to the previous set of transient objects).
> >
> > I've never meant it as a fallback for the command itself...
>
> Current error handling is to flush the entire space and abort ... and I
> think that's correct. There can't be any recovery because the TPM just
> signalled it's in an insane state (it failed a command that should
> succeed, either because our state is wrong and the TPM doesn't have the
> handle we think it does or the TPM has had some internal error). If
> I'm the user, I'm likely going to have to reset the TPM and restart and
> I likely wouldn't trust any state it gave me before this.
>
> James
The *current* fallback in the patch set is what I said. It keeps
previously saved set of transient objects and will try to load them next
time. Only when contexts are succesfully saved the code overwrite the
buffers with new stuff.
tpm2_flush_context() is only used to clean up remaining contexts after
ContextSave fails. It's not *the* fallback. Reverting to the previous
set of transient contexts is.
That's why I didn't check for TPM error in tpm2_commit_response (except
now I have the check in v3 because this wasn't bright in my mind at the
time).
The reasons for this fallback have nothing to do with mixed up TPM
state. There are legit reasons for tpm2_commit_response to fail like
running out of virtual handle slots or out of backing storage.
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Mon, 2017-01-16 at 16:48 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 16, 2017 at 06:24:48AM -0800, James Bottomley wrote:
> > On Mon, 2017-01-16 at 11:09 +0200, Jarkko Sakkinen wrote:
> > > On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> > > > On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > > > > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct
> > > > > device
> > > > > *pdev,
> > > > > chip->cdev.owner = THIS_MODULE;
> > > > > chip->cdev.kobj.parent = &chip->dev.kobj;
> > > > >
> > > > > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > > > > GFP_KERNEL);
> > > > > + if (!chip->work_space.context_buf) {
> > > > > + rc = -ENOMEM;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > >
> > > > I think the work_buf handling can be greatly simplified by
> > > > making it a pointer to the space: it's only usable between
> > > > tpm2_prepare_space() and tpm2_commit_space() which are
> > > > protected by the chip mutex, so there's no need for it to exist
> > > > outside of these calls (i.e. it can be NULL).
> > > >
> > > > Doing it this way also saves the allocation and copying
> > > > overhead of work_space.
> > > >
> > > > The patch below can be folded to effect this.
> > >
> > > Hey, I have to take my words back. There's a separate buffer for
> > > space for a reason. If the transaction fails for example when RM
> > > is doing its job, we can revert to the previous set of transient
> > > objects.
> > >
> > > Your change would completely thrawt this. I tried varius ways to
> > > heal when RM decorations fail and this is the most fail safe to
> > > do it so lets stick with it.
> >
> > That's why I added the return code check in the other patch: if the
> > command fails in the TPM, the space state isn't updated at all, the
> > net result being that nothing changes in the space, thus you don't
> > need the copy, because there's nothing to revert on a failure.
>
> You are right in what you say but what if you save lets say 5
> transient contexts and ContextSave fails on 2nd. It's not for the
> command itself but for falling back to a sane state when
> tpm2_commit_space fails (to the previous set of transient objects).
>
> I've never meant it as a fallback for the command itself...
Current error handling is to flush the entire space and abort ... and I
think that's correct. There can't be any recovery because the TPM just
signalled it's in an insane state (it failed a command that should
succeed, either because our state is wrong and the TPM doesn't have the
handle we think it does or the TPM has had some internal error). If
I'm the user, I'm likely going to have to reset the TPM and restart and
I likely wouldn't trust any state it gave me before this.
James
> > If you're thinking transaction being a sequence of TPM commands,
> > then we might need an ioctl to transfer the space state to/from
> > userspace, so it can do rollback for several commands, but that too
> > wouldn't need us to have a single prior command saved copy.
> >
> > James
>
> Here I refer to transaction as a single tpm_transmit.
>
> /Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Mon, Jan 16, 2017 at 06:24:48AM -0800, James Bottomley wrote:
> On Mon, 2017-01-16 at 11:09 +0200, Jarkko Sakkinen wrote:
> > On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> > > On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > > > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct
> > > > device
> > > > *pdev,
> > > > chip->cdev.owner = THIS_MODULE;
> > > > chip->cdev.kobj.parent = &chip->dev.kobj;
> > > >
> > > > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > > > GFP_KERNEL);
> > > > + if (!chip->work_space.context_buf) {
> > > > + rc = -ENOMEM;
> > > > + goto out;
> > > > + }
> > > > +
> > >
> > > I think the work_buf handling can be greatly simplified by making
> > > it a pointer to the space: it's only usable between
> > > tpm2_prepare_space() and tpm2_commit_space() which are protected by
> > > the chip mutex, so there's no need for it to exist outside of these
> > > calls (i.e. it can be NULL).
> > >
> > > Doing it this way also saves the allocation and copying overhead of
> > > work_space.
> > >
> > > The patch below can be folded to effect this.
> >
> > Hey, I have to take my words back. There's a separate buffer for
> > space for a reason. If the transaction fails for example when RM is
> > doing its job, we can revert to the previous set of transient
> > objects.
> >
> > Your change would completely thrawt this. I tried varius ways to heal
> > when RM decorations fail and this is the most fail safe to do it so
> > lets stick with it.
>
> That's why I added the return code check in the other patch: if the
> command fails in the TPM, the space state isn't updated at all, the net
> result being that nothing changes in the space, thus you don't need the
> copy, because there's nothing to revert on a failure.
You are right in what you say but what if you save lets say 5 transient
contexts and ContextSave fails on 2nd. It's not for the command itself
but for falling back to a sane state when tpm2_commit_space fails (to
the previous set of transient objects).
I've never meant it as a fallback for the command itself...
> If you're thinking transaction being a sequence of TPM commands, then
> we might need an ioctl to transfer the space state to/from userspace,
> so it can do rollback for several commands, but that too wouldn't need
> us to have a single prior command saved copy.
>
> James
Here I refer to transaction as a single tpm_transmit.
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Mon, 2017-01-16 at 11:09 +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> > On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct
> > > device
> > > *pdev,
> > > chip->cdev.owner = THIS_MODULE;
> > > chip->cdev.kobj.parent = &chip->dev.kobj;
> > >
> > > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > > GFP_KERNEL);
> > > + if (!chip->work_space.context_buf) {
> > > + rc = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> >
> > I think the work_buf handling can be greatly simplified by making
> > it a pointer to the space: it's only usable between
> > tpm2_prepare_space() and tpm2_commit_space() which are protected by
> > the chip mutex, so there's no need for it to exist outside of these
> > calls (i.e. it can be NULL).
> >
> > Doing it this way also saves the allocation and copying overhead of
> > work_space.
> >
> > The patch below can be folded to effect this.
>
> Hey, I have to take my words back. There's a separate buffer for
> space for a reason. If the transaction fails for example when RM is
> doing its job, we can revert to the previous set of transient
> objects.
>
> Your change would completely thrawt this. I tried varius ways to heal
> when RM decorations fail and this is the most fail safe to do it so
> lets stick with it.
That's why I added the return code check in the other patch: if the
command fails in the TPM, the space state isn't updated at all, the net
result being that nothing changes in the space, thus you don't need the
copy, because there's nothing to revert on a failure.
If you're thinking transaction being a sequence of TPM commands, then
we might need an ioctl to transfer the space state to/from userspace,
so it can do rollback for several commands, but that too wouldn't need
us to have a single prior command saved copy.
James
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Sat, Jan 14, 2017 at 12:53:15PM -0500, Ken Goldman wrote:
> On 1/13/2017 11:28 AM, Jarkko Sakkinen wrote:
>
> > > > +
> > > > + if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> > > > + /* should never happen */
> > > > + dev_err(&chip->dev, "TPM returned a different CC:
> > > > 0x%04x\n",
> > > > + cc);
> > > > + rc = -EFAULT;
> > > > + goto out_err;
> > > > + }
>
> Something is strange here. Is "CC" command code?
>
> The TPM does not return a command code. The mapping should use the
> command code from the command.
>
> It could be the code is correct - the command code mapped OK in the command
> but then failed in the response. But the error message is strange.
Thanks. I'll update that message.
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > *pdev,
> > chip->cdev.owner = THIS_MODULE;
> > chip->cdev.kobj.parent = &chip->dev.kobj;
> >
> > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > GFP_KERNEL);
> > + if (!chip->work_space.context_buf) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
>
> I think the work_buf handling can be greatly simplified by making it a
> pointer to the space: it's only usable between tpm2_prepare_space() and
> tpm2_commit_space() which are protected by the chip mutex, so there's
> no need for it to exist outside of these calls (i.e. it can be NULL).
>
> Doing it this way also saves the allocation and copying overhead of
> work_space.
>
> The patch below can be folded to effect this.
Hey, I have to take my words back. There's a separate buffer for space
for a reason. If the transaction fails for example when RM is doing its
job, we can revert to the previous set of transient objects.
Your change would completely thrawt this. I tried varius ways to heal
when RM decorations fail and this is the most fail safe to do it so lets
stick with it.
> James
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > *pdev,
> > chip->cdev.owner = THIS_MODULE;
> > chip->cdev.kobj.parent = &chip->dev.kobj;
> >
> > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > GFP_KERNEL);
> > + if (!chip->work_space.context_buf) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
>
> I think the work_buf handling can be greatly simplified by making it a
> pointer to the space: it's only usable between tpm2_prepare_space() and
> tpm2_commit_space() which are protected by the chip mutex, so there's
> no need for it to exist outside of these calls (i.e. it can be NULL).
>
> Doing it this way also saves the allocation and copying overhead of
> work_space.
>
> The patch below can be folded to effect this.
>
> James
>
> ---
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 13cac09..770a8c0 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -131,7 +131,6 @@ static void tpm_dev_release(struct device *dev)
> mutex_unlock(&idr_lock);
>
> kfree(chip->log.bios_event_log);
> - kfree(chip->work_space.context_buf);
> kfree(chip);
> }
>
> @@ -206,12 +205,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> chip->cdev.kobj.parent = &chip->dev.kobj;
> chip->cdevrm.kobj.parent = &chip->devrm.kobj;
>
> - chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> - if (!chip->work_space.context_buf) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> return chip;
>
> out:
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 8009ed4..adf7810 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -211,7 +211,7 @@ struct tpm_chip {
> char ppi_version[TPM_PPI_VERSION_LEN + 1];
> #endif /* CONFIG_ACPI */
>
> - struct tpm_space work_space;
> + struct tpm_space *work_space;
> u32 nr_commands;
> u32 *cc_attrs_tbl;
> };
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 44e5501..285361e 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -27,7 +27,7 @@ enum tpm2_handle_types {
>
> static void tpm2_flush_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> int i;
>
> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
> @@ -45,7 +45,7 @@ struct tpm2_context {
>
> static int tpm2_load_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> struct tpm2_context *ctx;
> struct tpm_buf buf;
> int i;
> @@ -99,7 +99,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
>
> static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t
> len)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> unsigned int nr_handles;
> u32 vhandle;
> u32 phandle;
> @@ -147,9 +147,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
> tpm_space *space,
> if (!space)
> return 0;
>
> - memcpy(&chip->work_space.context_tbl, &space->context_tbl,
> -sizeof(space->context_tbl));
> - memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
> + chip->work_space = space;
>
> rc = tpm2_load_space(chip);
> if (rc)
> @@ -164,7 +162,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
> tpm_space *space,
>
> static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t
> len)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> u32 phandle;
> u32 vhandle;
> u32 attrs;
> @@ -222,7 +220,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32
> cc, u8 *rsp, size_t len)
>
> static int tpm2_save_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> struct tpm_buf buf;
> int i;
> int j;
> @@ -295,9 +293,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct
> tpm_space *space,
> if (rc)
> return rc;
>
> - memcpy(&space->context_tbl, &chip->work_space.context_tbl,
> -sizeof(space->context_tbl));
> - memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
> + chip->work_space = NULL;
>
> return 0;
> }
[x]
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, Jan 12, 2017 at 12:38:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp,
> > size_t len)
> > +{
> > + struct tpm_space *space = &chip->work_space;
> > + u32 phandle;
> > + u32 vhandle;
> > + u32 attrs;
> > + int i;
> > + int rc;
> > +
> > + if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> > + /* should never happen */
> > + dev_err(&chip->dev, "TPM returned a different CC:
> > 0x%04x\n",
> > + cc);
> > + rc = -EFAULT;
> > + goto out_err;
> > + }
> > +
> > + if (!((attrs >> TPM2_CC_ATTR_RHANDLE) & 1))
> > + return 0;
> > +
> > + phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);
>
> I think we have to check the command return code here. We can't
> blindly fish handles out of the response if the TPM returned an error
> because they won't exist and we'll pull rubbish from the buffer.
> Incremental patch below.
>
> Note I think we should use get_unaligned_be32 because we're pulling a
> 32 bit word from something that's on byte 6 (so misaligned): some
> architectures will trigger an unaligned trap for this (it's not a
> problem: they trap handle it, it just slows down processing a lot).
>
> James
>
> ---
>
> commit d17ad905ff7b114f7efd23f930e9a541ccdf7621
> Author: James Bottomley
> Date: Wed Jan 11 22:01:29 2017 -0800
>
> check return code
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 61422e6..8009ed4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -90,6 +90,7 @@ enum tpm2_structures {
> };
>
> enum tpm2_return_codes {
> + TPM2_RC_SUCCESS = 0x,
> TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
> TPM2_RC_HANDLE = 0x008B,
> TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index ca55feb..44e5501 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -16,6 +16,7 @@
> */
>
> #include
> +#include
> #include "tpm.h"
>
> enum tpm2_handle_types {
> @@ -167,9 +168,13 @@ static int tpm2_map_response(struct tpm_chip *chip, u32
> cc, u8 *rsp, size_t len)
> u32 phandle;
> u32 vhandle;
> u32 attrs;
> + u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
> int i;
> int rc;
>
> + if (return_code != TPM2_RC_SUCCESS)
> + return 0;
> +
> if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> /* should never happen */
> dev_err(&chip->dev, "TPM returned a different CC: 0x%04x\n",
>
[x]
I'll squash this to the next patch set version.
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device
> *pdev,
> chip->cdev.owner = THIS_MODULE;
> chip->cdev.kobj.parent = &chip->dev.kobj;
>
> + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> GFP_KERNEL);
> + if (!chip->work_space.context_buf) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
I think the work_buf handling can be greatly simplified by making it a
pointer to the space: it's only usable between tpm2_prepare_space() and
tpm2_commit_space() which are protected by the chip mutex, so there's
no need for it to exist outside of these calls (i.e. it can be NULL).
Doing it this way also saves the allocation and copying overhead of
work_space.
The patch below can be folded to effect this.
James
---
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 13cac09..770a8c0 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -131,7 +131,6 @@ static void tpm_dev_release(struct device *dev)
mutex_unlock(&idr_lock);
kfree(chip->log.bios_event_log);
- kfree(chip->work_space.context_buf);
kfree(chip);
}
@@ -206,12 +205,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
chip->cdev.kobj.parent = &chip->dev.kobj;
chip->cdevrm.kobj.parent = &chip->devrm.kobj;
- chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
- if (!chip->work_space.context_buf) {
- rc = -ENOMEM;
- goto out;
- }
-
return chip;
out:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8009ed4..adf7810 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -211,7 +211,7 @@ struct tpm_chip {
char ppi_version[TPM_PPI_VERSION_LEN + 1];
#endif /* CONFIG_ACPI */
- struct tpm_space work_space;
+ struct tpm_space *work_space;
u32 nr_commands;
u32 *cc_attrs_tbl;
};
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 44e5501..285361e 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -27,7 +27,7 @@ enum tpm2_handle_types {
static void tpm2_flush_space(struct tpm_chip *chip)
{
- struct tpm_space *space = &chip->work_space;
+ struct tpm_space *space = chip->work_space;
int i;
for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
@@ -45,7 +45,7 @@ struct tpm2_context {
static int tpm2_load_space(struct tpm_chip *chip)
{
- struct tpm_space *space = &chip->work_space;
+ struct tpm_space *space = chip->work_space;
struct tpm2_context *ctx;
struct tpm_buf buf;
int i;
@@ -99,7 +99,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
{
- struct tpm_space *space = &chip->work_space;
+ struct tpm_space *space = chip->work_space;
unsigned int nr_handles;
u32 vhandle;
u32 phandle;
@@ -147,9 +147,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
tpm_space *space,
if (!space)
return 0;
- memcpy(&chip->work_space.context_tbl, &space->context_tbl,
- sizeof(space->context_tbl));
- memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
+ chip->work_space = space;
rc = tpm2_load_space(chip);
if (rc)
@@ -164,7 +162,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
tpm_space *space,
static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t
len)
{
- struct tpm_space *space = &chip->work_space;
+ struct tpm_space *space = chip->work_space;
u32 phandle;
u32 vhandle;
u32 attrs;
@@ -222,7 +220,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc,
u8 *rsp, size_t len)
static int tpm2_save_space(struct tpm_chip *chip)
{
- struct tpm_space *space = &chip->work_space;
+ struct tpm_space *space = chip->work_space;
struct tpm_buf buf;
int i;
int j;
@@ -295,9 +293,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct
tpm_space *space,
if (rc)
return rc;
- memcpy(&space->context_tbl, &chip->work_space.context_tbl,
- sizeof(space->context_tbl));
- memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
+ chip->work_space = NULL;
return 0;
}
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp,
> size_t len)
> +{
> + struct tpm_space *space = &chip->work_space;
> + u32 phandle;
> + u32 vhandle;
> + u32 attrs;
> + int i;
> + int rc;
> +
> + if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> + /* should never happen */
> + dev_err(&chip->dev, "TPM returned a different CC:
> 0x%04x\n",
> + cc);
> + rc = -EFAULT;
> + goto out_err;
> + }
> +
> + if (!((attrs >> TPM2_CC_ATTR_RHANDLE) & 1))
> + return 0;
> +
> + phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);
I think we have to check the command return code here. We can't
blindly fish handles out of the response if the TPM returned an error
because they won't exist and we'll pull rubbish from the buffer.
Incremental patch below.
Note I think we should use get_unaligned_be32 because we're pulling a
32 bit word from something that's on byte 6 (so misaligned): some
architectures will trigger an unaligned trap for this (it's not a
problem: they trap handle it, it just slows down processing a lot).
James
---
commit d17ad905ff7b114f7efd23f930e9a541ccdf7621
Author: James Bottomley
Date: Wed Jan 11 22:01:29 2017 -0800
check return code
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 61422e6..8009ed4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -90,6 +90,7 @@ enum tpm2_structures {
};
enum tpm2_return_codes {
+ TPM2_RC_SUCCESS = 0x,
TPM2_RC_HASH= 0x0083, /* RC_FMT1 */
TPM2_RC_HANDLE = 0x008B,
TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index ca55feb..44e5501 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -16,6 +16,7 @@
*/
#include
+#include
#include "tpm.h"
enum tpm2_handle_types {
@@ -167,9 +168,13 @@ static int tpm2_map_response(struct tpm_chip *chip, u32
cc, u8 *rsp, size_t len)
u32 phandle;
u32 vhandle;
u32 attrs;
+ u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
int i;
int rc;
+ if (return_code != TPM2_RC_SUCCESS)
+ return 0;
+
if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
/* should never happen */
dev_err(&chip->dev, "TPM returned a different CC: 0x%04x\n",
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, Jan 12, 2017 at 10:38:30AM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > @@ -435,17 +440,23 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > const u8 *buf, size_t bufsiz,
> > goto out;
> >
> > out_recv:
> > - rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
> > - if (rc < 0)
> > + len = chip->ops->recv(chip, (u8 *) buf, bufsiz);
> > + if (len < 0) {
> > dev_err(&chip->dev,
> > - "tpm_transmit: tpm_recv: error %zd\n", rc);
> > + "tpm_transmit: tpm_recv: error %d\n", rc);
> > + rc = len;
> > + goto out;
> > + }
> > +
> > + rc = tpm2_commit_space(chip, space, ordinal, buf, bufsiz);
>
> I think this should be
>
> rc = tpm2_commit_space(chip, space, ordinal, buf, len)
>
> because tpm2_commit_space wants to know the length of the response, not
> the length of the original command.
Yeah, would make sense.
> James
/Jarkko
Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces
On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> @@ -435,17 +440,23 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> const u8 *buf, size_t bufsiz,
> goto out;
>
> out_recv:
> - rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
> - if (rc < 0)
> + len = chip->ops->recv(chip, (u8 *) buf, bufsiz);
> + if (len < 0) {
> dev_err(&chip->dev,
> - "tpm_transmit: tpm_recv: error %zd\n", rc);
> + "tpm_transmit: tpm_recv: error %d\n", rc);
> + rc = len;
> + goto out;
> + }
> +
> + rc = tpm2_commit_space(chip, space, ordinal, buf, bufsiz);
I think this should be
rc = tpm2_commit_space(chip, space, ordinal, buf, len)
because tpm2_commit_space wants to know the length of the response, not
the length of the original command.
James
