Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

2017-01-30 Thread James Bottomley 
On Mon, 2017-01-30 at 23:58 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 30, 2017 at 08:04:55AM -0800, James Bottomley wrote:
> > On Sun, 2017-01-29 at 19:52 -0500, Ken Goldman wrote:
> > > On 1/27/2017 5:04 PM, James Bottomley wrote:
> > > 
> > > > > Beware the nasty corner case:
> > > > > 
> > > > > - Application asks for a session and gets 0200
> > > > > 
> > > > > - Time elapses and 0200 gets forcibly flushed
> > > > > 
> > > > > - Later, app comes back, asks for a second session and again
> > > > > gets
> > > > > 0200.
> > > > > 
> > > > > - App gets very confused.
> > > > > 
> > > > > May it be better to close the connection completely, which
> > > > > the
> > > > > application can detect, than flush a session and give this
> > > > > corner
> > > > > case?
> > > > 
> > > > if I look at the code I've written, I don't know what the
> > > > session
> > > > number is, I just save sessionHandle in a variable for later
> > > > use 
> > > > (lets say to v1).  If I got the same session number returned at
> > > > a 
> > > > later time and placed it in v2, all I'd notice is that an 
> > > > authorization using v1 would fail.  I'm not averse to killing
> > > > the 
> > > > entire connection but, assuming you have fallback, it might be 
> > > > kinder simply to ensure that the operations with the reclaimed 
> > > > session fail (which is what the code currently does).
> > > 
> > > My worry is that this session failure cannot be detected by the 
> > > application.  An HMAC failure could cause the app to tell a user
> > > that
> > > they entered the wrong password.  Misleading.  On the TPM, it
> > > could 
> > > trigger the dictionary attack lockout.  For a PIN index, it could
> > > consume a failure count.  Killing a policy session that has e.g.,
> > > a 
> > > policy signed term could cause the application to go back to some
> > > external entity for another authorization signature.
> > > 
> > > Let's go up to the stack.  What's the attack?
> > > 
> > > If we're worried about many simultaneous applications (wouldn't
> > > that 
> > > be wonderful), why not just let startauthsession fail?  The 
> > > application can just retry periodically.
> > 
> > How in that scenario do we ensure that a session becomes available?
> >  Once that's established, there's no real difference between
> > retrying
> > the startauthsession in the kernel when we know the session is
> > available and forcing userspace to do the retry except that the
> > former
> > has a far greater chance of success (and it's only about 6 lines of
> > code).
> > 
> > >   Just allocate them in triples so there's no deadlock.
> > 
> > Is this the application or the kernel?  If it's the kernel, that
> > adds a
> > lot of complexity.
> > 
> > > If we're worried about a DoS attack, killing a session just helps
> > > the
> > > attacker.  The attacker can create a few connections and spin on 
> > > startauthsession, locking everyone out anyway.
> > 
> > There are two considerations here: firstly we'd need to introduce a
> > mechanism to "kill" the connection.  Probably we'd simply error
> > every
> > command on the space until it was closed.  The second is which
> > scenario
> > is more reasonable: Say the application simply forgot to flush the
> > session and will never use it again.  Simply reclaiming the session
> > would produce no effect at all on the application in this scenario.
> >  However, I have no data to say what's likely.
> > 
> > > ~~
> > > 
> > > Also, let's remember that this is a rare application.  Sessions
> > > are 
> > > only needed for remote access (requiring encryption, HMAC or
> > > salt), 
> > > or policy sessions.
> > 
> > This depends what your threat model is.  For ssh keys, you worry
> > that
> > someone might be watching, so you use HMAC authority even for a
> > local
> > TPM.  In the cloud, you don't quite know where the TPM is, so again
> > you'd use HMAC sessions ... however, in both use cases the sessions
> > should be very short lived.
> > 
> > > ~~
> > > 
> > > Should the code also reserve a session for the kernel?  Mark it
> > > not 
> > > kill'able?
> > 
> > At the moment, the kernel doesn't use sessions, so let's worry
> > about
> > that problem at the point it arises (if it ever arises).
> > 
> > James
> 
> It does. My trusted keys implementation actually uses sessions.

But as I read the code, I can't find where the kernel creates a
session.  It looks like the session and hmac are passed in as option
arguments, aren't they?

> I'm kind dilating to an opinion that we would leave this commit out 
> from the first kernel release that will contain the resource manager 
> with similar rationale as Jason gave me for whitelisting: get the 
> basic stuff in and once it is used with some workloads whitelisting 
> and exhaustion will take eventually the right form.
> 
> How would you feel about this?

As long as we get patch 1/2 then applications using sessions will
actually work with spaces, so taking more time with

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

2017-01-30 Thread Jarkko Sakkinen 
On Mon, Jan 30, 2017 at 08:04:55AM -0800, James Bottomley wrote:
> On Sun, 2017-01-29 at 19:52 -0500, Ken Goldman wrote:
> > On 1/27/2017 5:04 PM, James Bottomley wrote:
> > 
> > > > Beware the nasty corner case:
> > > > 
> > > > - Application asks for a session and gets 0200
> > > > 
> > > > - Time elapses and 0200 gets forcibly flushed
> > > > 
> > > > - Later, app comes back, asks for a second session and again gets
> > > > 0200.
> > > > 
> > > > - App gets very confused.
> > > > 
> > > > May it be better to close the connection completely, which the
> > > > application can detect, than flush a session and give this corner
> > > > case?
> > > 
> > > if I look at the code I've written, I don't know what the session
> > > number is, I just save sessionHandle in a variable for later use 
> > > (lets say to v1).  If I got the same session number returned at a 
> > > later time and placed it in v2, all I'd notice is that an 
> > > authorization using v1 would fail.  I'm not averse to killing the 
> > > entire connection but, assuming you have fallback, it might be 
> > > kinder simply to ensure that the operations with the reclaimed 
> > > session fail (which is what the code currently does).
> > 
> > My worry is that this session failure cannot be detected by the 
> > application.  An HMAC failure could cause the app to tell a user that
> > they entered the wrong password.  Misleading.  On the TPM, it could 
> > trigger the dictionary attack lockout.  For a PIN index, it could 
> > consume a failure count.  Killing a policy session that has e.g., a 
> > policy signed term could cause the application to go back to some 
> > external entity for another authorization signature.
> > 
> > Let's go up to the stack.  What's the attack?
> > 
> > If we're worried about many simultaneous applications (wouldn't that 
> > be wonderful), why not just let startauthsession fail?  The 
> > application can just retry periodically.
> 
> How in that scenario do we ensure that a session becomes available? 
>  Once that's established, there's no real difference between retrying
> the startauthsession in the kernel when we know the session is
> available and forcing userspace to do the retry except that the former
> has a far greater chance of success (and it's only about 6 lines of
> code).
> 
> >   Just allocate them in triples so there's no deadlock.
> 
> Is this the application or the kernel?  If it's the kernel, that adds a
> lot of complexity.
> 
> > If we're worried about a DoS attack, killing a session just helps the
> > attacker.  The attacker can create a few connections and spin on 
> > startauthsession, locking everyone out anyway.
> 
> There are two considerations here: firstly we'd need to introduce a
> mechanism to "kill" the connection.  Probably we'd simply error every
> command on the space until it was closed.  The second is which scenario
> is more reasonable: Say the application simply forgot to flush the
> session and will never use it again.  Simply reclaiming the session
> would produce no effect at all on the application in this scenario. 
>  However, I have no data to say what's likely.
> 
> > ~~
> > 
> > Also, let's remember that this is a rare application.  Sessions are 
> > only needed for remote access (requiring encryption, HMAC or salt), 
> > or policy sessions.
> 
> This depends what your threat model is.  For ssh keys, you worry that
> someone might be watching, so you use HMAC authority even for a local
> TPM.  In the cloud, you don't quite know where the TPM is, so again
> you'd use HMAC sessions ... however, in both use cases the sessions
> should be very short lived.
> 
> > ~~
> > 
> > Should the code also reserve a session for the kernel?  Mark it not 
> > kill'able?
> 
> At the moment, the kernel doesn't use sessions, so let's worry about
> that problem at the point it arises (if it ever arises).
> 
> James

It does. My trusted keys implementation actually uses sessions.

I'm kind dilating to an opinion that we would leave this commit out from
the first kernel release that will contain the resource manager with
similar rationale as Jason gave me for whitelisting: get the basic stuff
in and once it is used with some workloads whitelisting and exhaustion
will take eventually the right form.

How would you feel about this?

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] Fwd: Re: [PATCH v9 2/2] tpm: add securityfs support, for TPM 2.0 firmware event log

2017-01-30 Thread Jarkko Sakkinen 
On Mon, Jan 30, 2017 at 03:08:42PM +0530, Nayna wrote:
> 
> > From: "Ken Goldman"  > >
> > Date: 26-Jan-2017 2:53 AM
> > Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> > support,for TPM 2.0 firmware event log
> > To:  > >,
> >  > >,
> >  > >
> > Cc:
> > 
> > You do not need to send a new patch set version as long as this
> > one gets peer tested. And it needs to be tested without hacks
> > like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> > be peer tested to be more specific.
> > 
> > For me the code itself looks good but I simply cannot take it in
> > in the current situation.
> > 
> > /Jarkko
> > 
> > 
> > Tested-by: Kenneth Goldman  > >
> > 
> > I validated a firmware event log taken from a Power 8 against PCR 0-7
> > values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> > that same platform.
> > 
> 
> Thank You Ken.
> 
> Jarkko, I hope now these patches can be accepted for 4.11.
> 
> Thanks & Regards,
>- Nayna

Thanks Ken. I somehow managed to miss that response.

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()

2017-01-30 Thread Jarkko Sakkinen 
On Mon, Jan 30, 2017 at 08:28:30AM +0530, Nayna wrote:
> 
> 
> On 01/30/2017 02:50 AM, Jarkko Sakkinen wrote:
> > On Sun, Jan 29, 2017 at 10:48:39PM +0530, Nayna wrote:
> > > 
> > > 
> > > On 01/29/2017 08:10 PM, Jarkko Sakkinen wrote:
> > > > On Fri, Jan 27, 2017 at 10:25:49AM -0500, Nayna Jain wrote:
> > > > > This patch add validation in tpm2_get_pcr_allocation to avoid
> > > > > access beyond response buffer length.
> > > > > 
> > > > > Suggested-by: Stefan Berger 
> > > > > Signed-off-by: Nayna Jain 
> > > > 
> > > > This validation looks broken to me.
> > > > 
> > > > > ---
> > > > >drivers/char/tpm/tpm2-cmd.c | 28 +++-
> > > > >1 file changed, 23 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > > > > index 4aad84c..02c1ea7 100644
> > > > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > > > @@ -1008,9 +1008,13 @@ static ssize_t tpm2_get_pcr_allocation(struct 
> > > > > tpm_chip *chip)
> > > > >   struct tpm2_pcr_selection pcr_selection;
> > > > >   struct tpm_buf buf;
> > > > >   void *marker;
> > > > > - unsigned int count = 0;
> > > > > + void *end;
> > > > > + void *pcr_select_offset;
> > > > > + unsigned int count;
> > > > > + u32 sizeof_pcr_selection;
> > > > > + u32 resp_len;
> > > > 
> > > > Very cosmetic but we almos almost universally use the acronym 'rsp' in
> > > > the TPM driver.
> > > 
> > > Sure will update.
> > > 
> > > > 
> > > > >   int rc;
> > > > > - int i;
> > > > > + int i = 0;
> > > > 
> > > > Why do you need to initialize it?
> > > 
> > > Because in out: count is replaced with i.
> > > And it is replaced because  now for loop can break even before reaching
> > > count, because of new buffer checks.
> > > > 
> > > > > 
> > > > >   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, 
> > > > > TPM2_CC_GET_CAPABILITY);
> > > > >   if (rc)
> > > > > @@ -1034,15 +1038,29 @@ static ssize_t tpm2_get_pcr_allocation(struct 
> > > > > tpm_chip *chip)
> > > > >   }
> > > > > 
> > > > >   marker = [TPM_HEADER_SIZE + 9];
> > > > > +
> > > > > + resp_len = be32_to_cpup((__be32 *)[2]);
> > > > > + end = [resp_len];
> > > > 
> > > > What if the response contains larger length than the buffer size?
> > > 
> > > Isn't this check need to be done in tpm_transmit_cmd for all responses ?
> > > Though, it seems it is not done there as well.
> > > 
> > > And to understand what do we expect max buffer length. PAGE_SIZE or
> > > TPM_BUFSIZE ?
> > 
> > Oops. You are correct it is done there:
> > 
> > if (len != be32_to_cpu(header->length))
> > return -EFAULT;
> > 
> > So need to do this.
> 
> To be sure, means nothing need to be done in this. Right ?

This is correct.

> And guess this was the only thing you meant by broken for this patch.
> 
> I will do other two smaller changes as I send the whole new patchset.
> 
> Thanks & Regards,
>   - Nayna

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 1/2] tpm2: add session handle context saving and restoring to the space code

2017-01-30 Thread Jarkko Sakkinen 
On Sun, Jan 29, 2017 at 07:35:32PM -0500, Ken Goldman wrote:
> On 1/27/2017 7:32 PM, James Bottomley wrote:
> >
> > Sessions are also isolated during each instance of a tpm space.  This
> > means that spaces shouldn't be able to see each other's sessions and
> > is enforced by ensuring that a space user may only refer to sessions
> > handles that are present in their own chip->session_tbl.  Finally when
> > a space is closed, all the sessions belonging to it should be flushed
> > so the handles may be re-used by other spaces.
> 
> This should be true for transient objects as well.

Transient objects do not need anything special. All transient objects
are flushed after command-response so you implicitly get the isolation.

/Jarkko

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v2 1/2] tpm2: add session handle context saving and restoring to the space code

2017-01-30 Thread Jarkko Sakkinen 
On Sun, Jan 29, 2017 at 02:36:58PM -0800, James Bottomley wrote:
> On Sun, 2017-01-29 at 23:39 +0200, Jarkko Sakkinen wrote:
> > On Fri, Jan 27, 2017 at 04:32:38PM -0800, James Bottomley wrote:
> > > sessions are different from transient objects in that their handles
> > > may not be virtualized (because they're used for some hmac
> > > calculations).  Additionally when a session is context saved, a
> > > vestigial memory remains in the TPM and if it is also flushed, that
> > > will be lost and the session context will refuse to load next time,
> > > so
> > > the code is updated to flush only transient objects after a context
> > > save.  Add a separate array (chip->session_tbl) to save and restore
> > > sessions by handle.  Use the failure of a context save or load to
> > > signal that the session has been flushed from the TPM and we can
> > > remove its memory from chip->session_tbl.
> > > 
> > > Sessions are also isolated during each instance of a tpm space. 
> > >  This
> > > means that spaces shouldn't be able to see each other's sessions
> > > and
> > > is enforced by ensuring that a space user may only refer to
> > > sessions
> > > handles that are present in their own chip->session_tbl.  Finally
> > > when
> > > a space is closed, all the sessions belonging to it should be
> > > flushed
> > > so the handles may be re-used by other spaces.
> > > 
> > > Note that if we get a session save or load error, all sessions are
> > > effectively flushed.  Even though we restore the session buffer,
> > > all
> > > the old sessions will refuse to load after the flush and they'll be
> > > purged from our session memory.  This means that while transient
> > > context handling is still soft in the face of errors, session
> > > handling
> > > is hard (any failure of the model means all sessions are lost).
> > > 
> > > Signed-off-by: James Bottomley <
> > > james.bottom...@hansenpartnership.com>
> > > 
> > > ---
> > > 
> > > v2: - add kill space to remove sessions on close
> > > - update changelog
> > > - reduce session table to 3 entries
> > > ---
> > >  drivers/char/tpm/tpm-chip.c   |   6 +++
> > >  drivers/char/tpm/tpm.h|   4 +-
> > >  drivers/char/tpm/tpm2-space.c | 112
> > > --
> > >  drivers/char/tpm/tpms-dev.c   |   2 +-
> > >  4 files changed, 118 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm
> > > -chip.c
> > > index ed4f887..6282ad0 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -130,6 +130,7 @@ static void tpm_dev_release(struct device *dev)
> > >  
> > >   kfree(chip->log.bios_event_log);
> > >   kfree(chip->work_space.context_buf);
> > > + kfree(chip->work_space.session_buf);
> > >   kfree(chip);
> > >  }
> > >  
> > > @@ -223,6 +224,11 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > > *pdev,
> > >   rc = -ENOMEM;
> > >   goto out;
> > >   }
> > > + chip->work_space.session_buf = kzalloc(PAGE_SIZE,
> > > GFP_KERNEL);
> > > + if (!chip->work_space.session_buf) {
> > > + rc = -ENOMEM;
> > > + goto out;
> > > + }
> > >  
> > >   return chip;
> > >  
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c4b8ec9..10c57b9 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -161,6 +161,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >   u32 context_tbl[3];
> > >   u8 *context_buf;
> > > + u32 session_tbl[3];
> > > + u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -583,7 +585,7 @@ unsigned long tpm2_calc_ordinal_duration(struct
> > > tpm_chip *chip, u32 ordinal);
> > >  int tpm2_probe(struct tpm_chip *chip);
> > >  int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
> > >  int tpm2_init_space(struct tpm_space *space);
> > > -void tpm2_del_space(struct tpm_space *space);
> > > +void tpm2_del_space(struct tpm_chip *chip, struct tpm_space
> > > *space);
> > >  int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space
> > > *space, u32 cc,
> > >  u8 *cmd);
> > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > *space,
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index d241c2a..2b3d1ad 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -32,18 +32,28 @@ struct tpm2_context {
> > >   __be16 blob_size;
> > >  } __packed;
> > >  
> > > +static void tpm2_kill_sessions(struct tpm_chip *chip, struct
> > > tpm_space *space);
> > > +
> > >  int tpm2_init_space(struct tpm_space *space)
> > >  {
> > >   space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > >   if (!space->context_buf)
> > >   return -ENOMEM;
> > >  
> > > + space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > + if (space->session_buf == NULL) {
> > > + kfree(space->context_buf);
> > > + return -ENOMEM;
>

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Tyrel Datwyler 
On 01/26/2017 12:22 PM, Michal Suchánek wrote:
> Hello,
> 
> building ibmvtpm I noticed gcc warning complaining that second word of
> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
> 
> The structure is defined as 
> 
> struct ibmvtpm_crq {
> u8 valid;
> u8 msg;
> __be16 len;
> __be32 data;
> __be64 reserved;
> } __attribute__((packed, aligned(8)));
> 
> initialized as
> 
> struct ibmvtpm_crq crq;
> u64 *buf = (u64 *) 
> ...
> crq.valid = (u8)IBMVTPM_VALID_CMD;
> crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
> 
> and submitted with
> 
> rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>   cpu_to_be64(buf[1]));

These should be be64_to_cpu() here. The underlying hcall made by
ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
RTAS interface which requires data in BE.

> 
> which means that the second word indeed contains purely garbage.
> 
> This is repeated a few times in the driver so I added memset to quiet
> gcc and make behavior deterministic in case the unused fields get some
> meaning in the future.
> 
> However, in tpm_ibmvtpm_send the structure is initialized as
> 
>   struct ibmvtpm_crq crq;
> __be64 *word = (__be64 *)
> ...
> crq.valid = (u8)IBMVTPM_VALID_CMD;
> crq.msg = (u8)VTPM_TPM_COMMAND;
> crq.len = cpu_to_be16(count);
> crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
> 
> and submitted with
> 
>   rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>   be64_to_cpu(word[1]));
> meaning it is swapped twice.
> 
> 
> Where is the interface defined? Are the command arguments passed as BE
> subfields (the second case was correct before adding the extra whole
> word swap) or BE words (the first case doing whole word swap is
> correct)?

The interface is defined in PAPR. The crq format is defined in BE terms.
However, when we break the crq apart into high and low words they need
to be in cpu endian as mentioned above.

-Tyrel

> 
> Thanks
> 
> Michal
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Michal Suchanek 
On 27 January 2017 at 02:50, Benjamin Herrenschmidt
 wrote:
> On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
>> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
>> > Hello,
>> >
>> > building ibmvtpm I noticed gcc warning complaining that second word
>> > of
>> > struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
>> >
>> > The structure is defined as
>> >
>> > struct ibmvtpm_crq {
>> > u8 valid;
>> > u8 msg;
>> > __be16 len;
>> > __be32 data;
>> > __be64 reserved;
>> > } __attribute__((packed, aligned(8)));
>> >
>> > initialized as
>> >
>> > struct ibmvtpm_crq crq;
>> > u64 *buf = (u64 *) 
>> > ...
>> > crq.valid = (u8)IBMVTPM_VALID_CMD;
>> > crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
>> >
>> > and submitted with
>> >
>> > rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>> >   cpu_to_be64(buf[1]));
>>
>> These should be be64_to_cpu() here. The underlying hcall made by
>> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
>> RTAS interface which requires data in BE.
>
> Hrm... an hcall takes register arguments. Register arguments don't have
> an endianness.
>
> The problem is that we are packing an in-memory structure into 2
> registers and it's expected that this structure is laid out in the
> registers as if it had been loaded by a BE CPU.
>
> So we have two things at play here:
>
>   - The >8-bit fields should be laid out BE in the memory image
>   - That whole 128-bit structure should be loaded into 2 64-bit
> registers MSB first.
>
> So the "double" swap is somewhat needed. The uglyness comes from the
> passing-by-register of the h-call but it should work.
>
> That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
> better result (though recent gcc's might not make a difference).

If this should work then the below case that swaps the fields separately is
broken.

Anyway, structures have no endianess so when they start with a byte they
start with that byte no matter the host endian.
crq.valid is the first byte always. And then each field is to be swapped
separately.

On the other hand, bitfields are part of an integer and the field should be
swapped as part of the integer.

That is,
#define CRQ_VALID ((buf[0] >> 56) & 0xff)
CRQ_VALID is part of an integer in buf and would be laid out differently
on start or end depending on the host being BE or LE.

And the question is what the PAPR actually defines because both ways are
used in the code. You can describe an in-memory layout either way.

>> >
>> > which means that the second word indeed contains purely garbage.
>> >
>> > This is repeated a few times in the driver so I added memset to
>> > quiet
>> > gcc and make behavior deterministic in case the unused fields get
>> > some
>> > meaning in the future.
>> >
>> > However, in tpm_ibmvtpm_send the structure is initialized as
>> >
>> > struct ibmvtpm_crq crq;
>> > __be64 *word = (__be64 *)
>> > ...
>> > crq.valid = (u8)IBMVTPM_VALID_CMD;
>> > crq.msg = (u8)VTPM_TPM_COMMAND;
>> > crq.len = cpu_to_be16(count);
>> > crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>> >
>> > and submitted with
>> >
>> > rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>> >   be64_to_cpu(word[1]));
>> > meaning it is swapped twice.
>> >
>> >
>> > Where is the interface defined? Are the command arguments passed as
>> > BE
>> > subfields (the second case was correct before adding the extra
>> > whole
>> > word swap) or BE words (the first case doing whole word swap is
>> > correct)?
>>
>> The interface is defined in PAPR. The crq format is defined in BE
>> terms.

Which exact PAPR? Where can I get it?
The PAPR document I found does not say anything about vtpm.

Thanks

Michal

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Michal Suchanek 
On 26 January 2017 at 23:05, Jason Gunthorpe
 wrote:
> On Thu, Jan 26, 2017 at 09:22:48PM +0100, Michal Such??nek wrote:
>
>> This is repeated a few times in the driver so I added memset to quiet
>> gcc and make behavior deterministic in case the unused fields get some
>> meaning in the future.
>
> Yep, reserved certainly needs to be zeroed.. Can you send a patch?

And len and data as well..

> memset is overkill...

Does not look so.

Michal

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread David Laight 
From: Tyrel Datwyler
> Sent: 27 January 2017 18:03
> On 01/26/2017 05:50 PM, Benjamin Herrenschmidt wrote:
> > On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
> >> On 01/26/2017 12:22 PM, Michal Suchnek wrote:
> >>> Hello,
> >>>
> >>> building ibmvtpm I noticed gcc warning complaining that second word
> >>> of
> >>> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
> >>>
> >>> The structure is defined as
> >>>
> >>> struct ibmvtpm_crq {
> >>> u8 valid;
> >>> u8 msg;
> >>> __be16 len;
> >>> __be32 data;
> >>> __be64 reserved;
> >>> } __attribute__((packed, aligned(8)));
> >>>
> >>> initialized as
> >>>
> >>> struct ibmvtpm_crq crq;
> >>> u64 *buf = (u64 *) 
> >>> ...
> >>> crq.valid = (u8)IBMVTPM_VALID_CMD;
> >>> crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
> >>>
> >>> and submitted with
> >>>
> >>> rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
> >>>   cpu_to_be64(buf[1]));

Isn't the real fubar here the use of that memory layout structure at all?
It would probably all be better if the call looked like:
rc = ibmvtpm_send_crq(ibmvtpm->vdev, MAKE_REQ(IBMVTPM_VALID_CMD,
VTPM_PREPARE_TO_SUSPEND, xxx_len, xxx_data), 0);
and MAKE_REQ() did all the required endian independant shifts to generate
the correct 32bit value.

David

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread msuchanek 
Hello,

On 2017-01-27 21:32, Tyrel Datwyler wrote:
> On 01/27/2017 11:58 AM, Benjamin Herrenschmidt wrote:
>> On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:
 The problem is that we are packing an in-memory structure into 2
 registers and it's expected that this structure is laid out in the
 registers as if it had been loaded by a BE CPU.
>>> 
>>> This is only the case if the cpu is BE. If the cpu is LE, regardless 
>>> of
>>> the fact that our in memory structure is laid out BE, when we break 
>>> it
>>> into 2 words each of those words needs to be loaded LE.
>> 
>> That doesn't make sense and doesn't match the code... The structure
>> needs to always have the same in-register layout regardless of the
>> endianness of the CPU, especially since the underlying hypervisor
>> will most likely be BE :-)
>> 
>> Thta's why the code does a be64_to_cpu() when loading it, this in
>> effect performs a "BE" load, which on a BE CPU is just a normal load
>> and on LE is a swap to compensate for the CPU loading it the "wrong 
>> way
>> around".
> 
> Its possible being the end of the week I'm just a little dense, but
> wouldn't be64_to_cpu() imply that we are byte-swapping something that 
> is
> already, or supposedly already, in BE format to cpu endianness? Which 
> on
> a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
> been swapped from BE --> LE?
> 
> In my eyes the code does seem to support what I've argued. The same
> thing is done in the scsi VIO drivers. The CRQ structure is laid out 
> and
> annotated BE. We use cpu_to_be() calls to load any non 8bit field.
> Finally, each word is swapped to cpu endian when we hand it off for the
> hcall.
> 
> from ibmvfc_send_event():
> 
> __be64 *crq_as_u64 = (__be64 *) >crq;
> 
>   <..snip..>
> 
> if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
>   be64_to_cpu(crq_as_u64[1] {
> 
> Again, maybe I'm missing something.
> 

Ok, so you perform really difficult operation for no good reason. You 
say
that the ppc dual-endian works like this: there is an internal in-cpu
representation of numbers which is always the same. What is affected by
switching endian is how memory loads and stores work.

If you pass these two words in registers you never need to swap 
anything.

> Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
> ---
> Word0 | Valid | Type  | Length|  Data
> ---
> Word1 |Reserved
> ---
> 
> The following definition looks to match:
> 
> struct ibmvtpm_crq {
> u8 valid;
> u8 msg;
> __be16 len;
> __be32 data;
> __be64 reserved;
> } __attribute__((packed, aligned(8)));

If under BE valid is first byte then it is MSB and you would get value
to pass in word 0 as (valid << 56) | (type << 48) | (length << 32 ) | 
data.

No swaps involved.

To achieve same with structure and swaps you would indeed first swap the
members and then the whole word. Much harder to read code that way, 
though.

Thanks

Michal

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Tyrel Datwyler 
On 01/27/2017 01:03 AM, Michal Suchanek wrote:
> On 27 January 2017 at 02:50, Benjamin Herrenschmidt
>  wrote:
>> On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
>>> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
 Hello,

 building ibmvtpm I noticed gcc warning complaining that second word
 of
 struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.

 The structure is defined as

 struct ibmvtpm_crq {
 u8 valid;
 u8 msg;
 __be16 len;
 __be32 data;
 __be64 reserved;
 } __attribute__((packed, aligned(8)));

 initialized as

 struct ibmvtpm_crq crq;
 u64 *buf = (u64 *) 
 ...
 crq.valid = (u8)IBMVTPM_VALID_CMD;
 crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;

 and submitted with

 rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
   cpu_to_be64(buf[1]));
>>>
>>> These should be be64_to_cpu() here. The underlying hcall made by
>>> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
>>> RTAS interface which requires data in BE.
>>
>> Hrm... an hcall takes register arguments. Register arguments don't have
>> an endianness.
>>
>> The problem is that we are packing an in-memory structure into 2
>> registers and it's expected that this structure is laid out in the
>> registers as if it had been loaded by a BE CPU.
>>
>> So we have two things at play here:
>>
>>   - The >8-bit fields should be laid out BE in the memory image
>>   - That whole 128-bit structure should be loaded into 2 64-bit
>> registers MSB first.
>>
>> So the "double" swap is somewhat needed. The uglyness comes from the
>> passing-by-register of the h-call but it should work.
>>
>> That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
>> better result (though recent gcc's might not make a difference).
> 
> If this should work then the below case that swaps the fields separately is
> broken.
> 
> Anyway, structures have no endianess so when they start with a byte they
> start with that byte no matter the host endian.
> crq.valid is the first byte always. And then each field is to be swapped
> separately.
> 
> On the other hand, bitfields are part of an integer and the field should be
> swapped as part of the integer.
> 
> That is,
> #define CRQ_VALID ((buf[0] >> 56) & 0xff)
> CRQ_VALID is part of an integer in buf and would be laid out differently
> on start or end depending on the host being BE or LE.
> 
> And the question is what the PAPR actually defines because both ways are
> used in the code. You can describe an in-memory layout either way.

Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
---
Word0 | Valid | Type  | Length|  Data
---
Word1 | Reserved
---

The following definition looks to match:

struct ibmvtpm_crq {
u8 valid;
u8 msg;
__be16 len;
__be32 data;
__be64 reserved;
} __attribute__((packed, aligned(8)));

> 

 which means that the second word indeed contains purely garbage.

 This is repeated a few times in the driver so I added memset to
 quiet
 gcc and make behavior deterministic in case the unused fields get
 some
 meaning in the future.

 However, in tpm_ibmvtpm_send the structure is initialized as

 struct ibmvtpm_crq crq;
 __be64 *word = (__be64 *)
 ...
 crq.valid = (u8)IBMVTPM_VALID_CMD;
 crq.msg = (u8)VTPM_TPM_COMMAND;
 crq.len = cpu_to_be16(count);
 crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);

 and submitted with

 rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
   be64_to_cpu(word[1]));
 meaning it is swapped twice.


 Where is the interface defined? Are the command arguments passed as
 BE
 subfields (the second case was correct before adding the extra
 whole
 word swap) or BE words (the first case doing whole word swap is
 correct)?
>>>
>>> The interface is defined in PAPR. The crq format is defined in BE
>>> terms.
> 
> Which exact PAPR? Where can I get it?
> The PAPR document I found does not say anything about vtpm. 

Unfortunately, vtpm doesn't appear to be covered in the publicly
available LoPAPR.

-Tyrel

> 
> Thanks
> 
> Michal
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread David Laight 
From: Michal Suchánek
> building ibmvtpm I noticed gcc warning complaining that second word of
> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
> 
> The structure is defined as
> 
> struct ibmvtpm_crq {
> u8 valid;
> u8 msg;
> __be16 len;
> __be32 data;
> __be64 reserved;
> } __attribute__((packed, aligned(8)));
> 
> initialized as
> 
> struct ibmvtpm_crq crq;
> u64 *buf = (u64 *) 
...

Hrummfff
What is that attribute for, seems pretty confusing and pointless to me.

I also suspect that if you want to access it as two 64bit words it
ought to be a union.

David


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Tyrel Datwyler 
On 01/26/2017 05:50 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
>> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
>>> Hello,
>>>
>>> building ibmvtpm I noticed gcc warning complaining that second word
>>> of
>>> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
>>>
>>> The structure is defined as 
>>>
>>> struct ibmvtpm_crq {
>>> u8 valid;
>>> u8 msg;
>>> __be16 len;
>>> __be32 data;
>>> __be64 reserved;
>>> } __attribute__((packed, aligned(8)));
>>>
>>> initialized as
>>>
>>> struct ibmvtpm_crq crq;
>>> u64 *buf = (u64 *) 
>>> ...
>>> crq.valid = (u8)IBMVTPM_VALID_CMD;
>>> crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
>>>
>>> and submitted with
>>>
>>> rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>>>   cpu_to_be64(buf[1]));
>>
>> These should be be64_to_cpu() here. The underlying hcall made by
>> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
>> RTAS interface which requires data in BE.
> 
> Hrm... an hcall takes register arguments. Register arguments don't have
> an endianness.

I wasn't suggesting that they do. However, I still believe my point is
valid that the arguments need to be loaded into the registers according
to the endianness of the cpu. We had several bugs during LE porting
where assumptions were made that parameters should be loaded BE
regardless of cpu endian. For example:

commit 3df76a9dcc74d5f012b94ea01ed6e7aaf8362c5a
Author: Cyril Bur 
Date:   Wed Jan 21 13:32:00 2015 +1100

powerpc/pseries: Fix endian problems with LE migration

RTAS events require arguments be passed in big endian while
hypercalls have their arguments passed in registers and the values
should therefore be in CPU endian.

> 
> The problem is that we are packing an in-memory structure into 2
> registers and it's expected that this structure is laid out in the
> registers as if it had been loaded by a BE CPU.

This is only the case if the cpu is BE. If the cpu is LE, regardless of
the fact that our in memory structure is laid out BE, when we break it
into 2 words each of those words needs to be loaded LE.

-Tyrel

> 
> So we have two things at play here:
> 
>   - The >8-bit fields should be laid out BE in the memory image
>   - That whole 128-bit structure should be loaded into 2 64-bit
> registers MSB first.
> 
> So the "double" swap is somewhat needed. The uglyness comes from the
> passing-by-register of the h-call but it should work.
> 
> That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
> better result (though recent gcc's might not make a difference).
>>>
>>> which means that the second word indeed contains purely garbage.
>>>
>>> This is repeated a few times in the driver so I added memset to
>>> quiet
>>> gcc and make behavior deterministic in case the unused fields get
>>> some
>>> meaning in the future.
>>>
>>> However, in tpm_ibmvtpm_send the structure is initialized as
>>>
>>> struct ibmvtpm_crq crq;
>>> __be64 *word = (__be64 *)
>>> ...
>>> crq.valid = (u8)IBMVTPM_VALID_CMD;
>>> crq.msg = (u8)VTPM_TPM_COMMAND;
>>> crq.len = cpu_to_be16(count);
>>> crq.data = cpu_to_be32(ibmvtpm->rtce_dma_handle);
>>>
>>> and submitted with
>>>
>>> rc = ibmvtpm_send_crq(ibmvtpm->vdev, be64_to_cpu(word[0]),
>>>   be64_to_cpu(word[1]));
>>> meaning it is swapped twice.
>>>
>>>
>>> Where is the interface defined? Are the command arguments passed as
>>> BE
>>> subfields (the second case was correct before adding the extra
>>> whole
>>> word swap) or BE words (the first case doing whole word swap is
>>> correct)?
>>
>> The interface is defined in PAPR. The crq format is defined in BE
>> terms.
>> However, when we break the crq apart into high and low words they
>> need
>> to be in cpu endian as mentioned above.
>>
>> -Tyrel
>>
>>>
>>> Thanks
>>>
>>> Michal
>>>
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Tyrel Datwyler 
On 01/29/2017 08:32 PM, Michael Ellerman wrote:
> Tyrel Datwyler  writes:
> 
>> On 01/27/2017 01:03 AM, Michal Suchanek wrote:
>>> On 27 January 2017 at 02:50, Benjamin Herrenschmidt
>>>  wrote:
 On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
>> Hello,
>>
>> building ibmvtpm I noticed gcc warning complaining that second word
>> of
>> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
>>
>> The structure is defined as
>>
>> struct ibmvtpm_crq {
>> u8 valid;
>> u8 msg;
>> __be16 len;
>> __be32 data;
>> __be64 reserved;
>> } __attribute__((packed, aligned(8)));
>>
>> initialized as
>>
>> struct ibmvtpm_crq crq;
>> u64 *buf = (u64 *) 
>> ...
>> crq.valid = (u8)IBMVTPM_VALID_CMD;
>> crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
>>
>> and submitted with
>>
>> rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>>   cpu_to_be64(buf[1]));
>
> These should be be64_to_cpu() here. The underlying hcall made by
> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
> RTAS interface which requires data in BE.

 Hrm... an hcall takes register arguments. Register arguments don't have
 an endianness.

 The problem is that we are packing an in-memory structure into 2
 registers and it's expected that this structure is laid out in the
 registers as if it had been loaded by a BE CPU.

 So we have two things at play here:

   - The >8-bit fields should be laid out BE in the memory image
   - That whole 128-bit structure should be loaded into 2 64-bit
 registers MSB first.

 So the "double" swap is somewhat needed. The uglyness comes from the
 passing-by-register of the h-call but it should work.

 That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
 better result (though recent gcc's might not make a difference).
>>>
>>> If this should work then the below case that swaps the fields separately is
>>> broken.
>>>
>>> Anyway, structures have no endianess so when they start with a byte they
>>> start with that byte no matter the host endian.
>>> crq.valid is the first byte always. And then each field is to be swapped
>>> separately.
>>>
>>> On the other hand, bitfields are part of an integer and the field should be
>>> swapped as part of the integer.
>>>
>>> That is,
>>> #define CRQ_VALID ((buf[0] >> 56) & 0xff)
>>> CRQ_VALID is part of an integer in buf and would be laid out differently
>>> on start or end depending on the host being BE or LE.
>>>
>>> And the question is what the PAPR actually defines because both ways are
>>> used in the code. You can describe an in-memory layout either way.
>>
>> Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
>> ---
>> Word0 | Valid | Type  | Length|  Data
>> ---
>> Word1 |  Reserved
>> ---
>>
>> The following definition looks to match:
>>
>> struct ibmvtpm_crq {
>> u8 valid;
>> u8 msg;
>> __be16 len;
>> __be32 data;
>> __be64 reserved;
>> } __attribute__((packed, aligned(8)));
> 
> Well it's a partial match.
> 
> Your layout above doesn't define which byte of Length or Data is the MSB
> or LSB. So going by that we still don't know the endianness of either

I should have been explicit that PAPR uses Big Endian bit and byte
numbering throughout the spec unless otherwise noted.

-Tyrel

> field.
> 
> cheers
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] ibmvtpm byteswapping inconsistency

2017-01-30 Thread Tyrel Datwyler 
On 01/27/2017 11:58 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2017-01-27 at 10:02 -0800, Tyrel Datwyler wrote:
>>> The problem is that we are packing an in-memory structure into 2
>>> registers and it's expected that this structure is laid out in the
>>> registers as if it had been loaded by a BE CPU.
>>
>> This is only the case if the cpu is BE. If the cpu is LE, regardless of
>> the fact that our in memory structure is laid out BE, when we break it
>> into 2 words each of those words needs to be loaded LE.
> 
> That doesn't make sense and doesn't match the code... The structure
> needs to always have the same in-register layout regardless of the
> endianness of the CPU, especially since the underlying hypervisor
> will most likely be BE :-)
> 
> Thta's why the code does a be64_to_cpu() when loading it, this in
> effect performs a "BE" load, which on a BE CPU is just a normal load
> and on LE is a swap to compensate for the CPU loading it the "wrong way
> around".

Its possible being the end of the week I'm just a little dense, but
wouldn't be64_to_cpu() imply that we are byte-swapping something that is
already, or supposedly already, in BE format to cpu endianness? Which on
a BE cpu I would expect a no-op, and on a LE cpu the 64bit word to have
been swapped from BE --> LE?

In my eyes the code does seem to support what I've argued. The same
thing is done in the scsi VIO drivers. The CRQ structure is laid out and
annotated BE. We use cpu_to_be() calls to load any non 8bit field.
Finally, each word is swapped to cpu endian when we hand it off for the
hcall.

from ibmvfc_send_event():

__be64 *crq_as_u64 = (__be64 *) >crq;

<..snip..>

if ((rc = ibmvfc_send_crq(vhost, be64_to_cpu(crq_as_u64[0]),
  be64_to_cpu(crq_as_u64[1] {

Again, maybe I'm missing something.

-Tyrel

> 
> Cheers,
> Ben.
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support, for TPM 2.0 firmware event log

2017-01-30 Thread Ken Goldman 
> You do not need to send a new patch set version as long as this
> one gets peer tested. And it needs to be tested without hacks
> like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> be peer tested to be more specific.
>
> For me the code itself looks good but I simply cannot take it in
> in the current situation.
>
> /Jarkko

Tested-by: Kenneth Goldman 

I validated a firmware event log taken from a Power 8 against PCR 0-7 
values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on 
that same platform.


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [RFC] tpm2-space: add handling for global session exhaustion

2017-01-30 Thread James Bottomley 
On Sun, 2017-01-29 at 19:52 -0500, Ken Goldman wrote:
> On 1/27/2017 5:04 PM, James Bottomley wrote:
> 
> > > Beware the nasty corner case:
> > > 
> > > - Application asks for a session and gets 0200
> > > 
> > > - Time elapses and 0200 gets forcibly flushed
> > > 
> > > - Later, app comes back, asks for a second session and again gets
> > > 0200.
> > > 
> > > - App gets very confused.
> > > 
> > > May it be better to close the connection completely, which the
> > > application can detect, than flush a session and give this corner
> > > case?
> > 
> > if I look at the code I've written, I don't know what the session
> > number is, I just save sessionHandle in a variable for later use 
> > (lets say to v1).  If I got the same session number returned at a 
> > later time and placed it in v2, all I'd notice is that an 
> > authorization using v1 would fail.  I'm not averse to killing the 
> > entire connection but, assuming you have fallback, it might be 
> > kinder simply to ensure that the operations with the reclaimed 
> > session fail (which is what the code currently does).
> 
> My worry is that this session failure cannot be detected by the 
> application.  An HMAC failure could cause the app to tell a user that
> they entered the wrong password.  Misleading.  On the TPM, it could 
> trigger the dictionary attack lockout.  For a PIN index, it could 
> consume a failure count.  Killing a policy session that has e.g., a 
> policy signed term could cause the application to go back to some 
> external entity for another authorization signature.
> 
> Let's go up to the stack.  What's the attack?
> 
> If we're worried about many simultaneous applications (wouldn't that 
> be wonderful), why not just let startauthsession fail?  The 
> application can just retry periodically.

How in that scenario do we ensure that a session becomes available? 
 Once that's established, there's no real difference between retrying
the startauthsession in the kernel when we know the session is
available and forcing userspace to do the retry except that the former
has a far greater chance of success (and it's only about 6 lines of
code).

>   Just allocate them in triples so there's no deadlock.

Is this the application or the kernel?  If it's the kernel, that adds a
lot of complexity.

> If we're worried about a DoS attack, killing a session just helps the
> attacker.  The attacker can create a few connections and spin on 
> startauthsession, locking everyone out anyway.

There are two considerations here: firstly we'd need to introduce a
mechanism to "kill" the connection.  Probably we'd simply error every
command on the space until it was closed.  The second is which scenario
is more reasonable: Say the application simply forgot to flush the
session and will never use it again.  Simply reclaiming the session
would produce no effect at all on the application in this scenario. 
 However, I have no data to say what's likely.

> ~~
> 
> Also, let's remember that this is a rare application.  Sessions are 
> only needed for remote access (requiring encryption, HMAC or salt), 
> or policy sessions.

This depends what your threat model is.  For ssh keys, you worry that
someone might be watching, so you use HMAC authority even for a local
TPM.  In the cloud, you don't quite know where the TPM is, so again
you'd use HMAC sessions ... however, in both use cases the sessions
should be very short lived.

> ~~
> 
> Should the code also reserve a session for the kernel?  Mark it not 
> kill'able?

At the moment, the kernel doesn't use sessions, so let's worry about
that problem at the point it arises (if it ever arises).

James


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v6 2/2] tpm: enhance TPM 2.0 PCR extend to, support multiple banks

2017-01-30 Thread Nayna 


On 01/26/2017 03:41 AM, Ken Goldman wrote:
>> The current TPM 2.0 device driver extends only the SHA1 PCR bank
>> but the TCG Specification[1] recommends extending all active PCR
>> banks, to prevent malicious users from setting unused PCR banks with
>> fake measurements and quoting them.
>>
>> The existing in-kernel interface(tpm_pcr_extend()) expects only a
>> SHA1 digest.  To extend all active PCR banks with differing
>> digest sizes, the SHA1 digest is padded with trailing 0's as needed.
>>
>> This patch reuses the defined digest sizes from the crypto subsystem,
>> adding a dependency on CRYPTO_HASH_INFO module.
>>
>> [1] TPM 2.0 Specification referred here is "TCG PC Client Specific
>> Platform Firmware Profile for TPM 2.0"
>
> Tested-by: Kenneth Goldman 
>
> I obtained an IMA event log from a Power platform, along with the PCR 10
> value from both the SHA-1 and SHA-256 banks of its Nuvoton TPM 2.0.  I
> independently validated that the event log matches the TPM PCR values.

Thank You Ken !!

Thanks & Regards,
- Nayna

>
>
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> ___
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v7 1/2] tpm: implement TPM 2.0 capability to get active PCR banks

2017-01-30 Thread Nayna Jain 
This patch implements the TPM 2.0 capability TPM_CAP_PCRS to
retrieve the active PCR banks from the TPM. This is needed
to enable extending all active banks as recommended by TPM 2.0
TCG Specification.

Signed-off-by: Nayna Jain 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Kenneth Goldman 
---
 drivers/char/tpm/tpm.h  |  5 +++
 drivers/char/tpm/tpm2-cmd.c | 77 +
 2 files changed, 82 insertions(+)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index dbe0c5a..db0398a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -97,6 +97,7 @@ enum tpm2_return_codes {
 };
 
 enum tpm2_algorithms {
+   TPM2_ALG_ERROR  = 0x,
TPM2_ALG_SHA1   = 0x0004,
TPM2_ALG_KEYEDHASH  = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
@@ -127,6 +128,7 @@ enum tpm2_permanent_handles {
 };
 
 enum tpm2_capabilities {
+   TPM2_CAP_PCRS   = 5,
TPM2_CAP_TPM_PROPERTIES = 6,
 };
 
@@ -187,6 +189,8 @@ struct tpm_chip {
 
const struct attribute_group *groups[3];
unsigned int groups_cnt;
+
+   u16 active_banks[7];
 #ifdef CONFIG_ACPI
acpi_handle acpi_dev_handle;
char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -540,4 +544,5 @@ int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
 #endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 42fe3dd..6fbd42c 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1030,3 +1030,80 @@ int tpm2_auto_startup(struct tpm_chip *chip)
rc = -ENODEV;
return rc;
 }
+
+struct tpm2_pcr_selection {
+   __be16  hash_alg;
+   u8  size_of_select;
+   u8  pcr_select[3];
+} __packed;
+
+/**
+ * tpm2_get_pcr_allocation() - get TPM active PCR banks.
+ *
+ * @chip: TPM chip to use.
+ *
+ * Return: Same as with tpm_transmit_cmd.
+ */
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
+{
+   struct tpm2_pcr_selection pcr_selection;
+   struct tpm_buf buf;
+   void *marker;
+   void *end;
+   void *pcr_select_offset;
+   unsigned int count;
+   u32 sizeof_pcr_selection;
+   u32 rsp_len;
+   int rc;
+   int i = 0;
+
+   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
+   if (rc)
+   return rc;
+
+   tpm_buf_append_u32(, TPM2_CAP_PCRS);
+   tpm_buf_append_u32(, 0);
+   tpm_buf_append_u32(, 1);
+
+   rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 9, 0,
+ "get tpm pcr allocation");
+   if (rc)
+   goto out;
+
+   count = be32_to_cpup(
+   (__be32 *)[TPM_HEADER_SIZE + 5]);
+
+   if (count > ARRAY_SIZE(chip->active_banks)) {
+   rc = -ENODEV;
+   goto out;
+   }
+
+   marker = [TPM_HEADER_SIZE + 9];
+
+   rsp_len = be32_to_cpup((__be32 *)[2]);
+   end = [rsp_len];
+
+   for (i = 0; i < count; i++) {
+   pcr_select_offset = marker +
+   offsetof(struct tpm2_pcr_selection, size_of_select);
+   if (pcr_select_offset >= end) {
+   rc = -EFAULT;
+   break;
+   }
+
+   memcpy(_selection, marker, sizeof(pcr_selection));
+   chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
+   sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
+   sizeof(pcr_selection.size_of_select) +
+   pcr_selection.size_of_select;
+   marker = marker + sizeof_pcr_selection;
+   }
+
+out:
+   if (i < ARRAY_SIZE(chip->active_banks))
+   chip->active_banks[i] = TPM2_ALG_ERROR;
+
+   tpm_buf_destroy();
+
+   return rc;
+}
-- 
2.5.0


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v7 0/2] enhance TPM 2.0 extend function to support multiple PCR banks

2017-01-30 Thread Nayna Jain 
IMA extends its hash measurements in the TPM PCRs, based on policy.
The existing in-kernel TPM extend function extends only the SHA1
PCR bank. TPM 2.0 defines multiple PCR banks, to support different
hash algorithms. The TCG TPM 2.0 Specification[1] recommends
extending all active PCR banks to prevent malicious users from
setting unused PCR banks with fake measurements and quoting them.
This patch set adds support for extending all active PCR banks,
as recommended.

The first patch implements the TPM 2.0 capability to retrieve
the list of active PCR banks.

The second patch modifies the tpm_pcr_extend() and tpm2_pcr_extend()
interface to support extending multiple PCR banks. The existing
tpm_pcr_extend() interface expects only a SHA1 digest. Hence, to
extend all active PCR banks with differing digest sizes for TPM 2.0,
the SHA1 digest is padded with 0's as needed.

[1] TPM 2.0 Specification referred here is "TCG PC Client Specific
Platform Firmware Profile for TPM 2.0"

Changelog v7:
- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
 - Fixed missing TPM error handling in tpm2_get_pcr_allocation().
 Thanks Jarkko for noticing it.
 - Included Stefan's suggestion on adding buffer access check.
- Patch "tpm: enchance TPM 2.0 PCR extend to support multiple banks"
 - Moved tpm2_digest and include for hash_info.h in tpm.h. tpm_eventlog.h
 has no modifications now.

Changelog v6:
- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
  - Fixed the regression - missing tpm_buf_destroy() in
  in tpm2_get_pcr_allocation(). Thanks Jarkko for noticing.
  - Added TPM2_ALG_ERROR = 0x to represent invalid algorithm.

Changelog v5:
- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
 - Included Jarkko's feedbacks
   - Moved variable declaration to start of function in
   tpm_pcr_extend()


Changelog v4:
- Updated cover letter as per Mimi's feedback.
- Rebased to Jarkko's latest master branch (4064b6b tpm_tis: use
  default timeout value if chip reports it as zero)
- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
 - Included Jarkko's feedbacks
   - Moved call to tpm2_get_pcr_allocation to Patch 2
   - Renamed struct tpm2_tpms_pcr_selection to struct tpm2_pcr_selection 
   and moved the struct to before tpm2_get_pcr_allocation()
   - Fixed code formatting
- Patch "tpm: enchance TPM 2.0 PCR extend to support multiple banks"
 - Included Jarkkos' feedbacks
   - Updated commit msg to mention dependency on CRYPTO_HASH_INFO
   - Renamed struct tpmt_hash to struct tpm2_digest 
   - Removed struct tpml_digest_values, tpm2_pcr_extend() now accepts
   count and digests list as two separate arguments. Added check for
   count of hashes passed.
 - Cleaned up struct tpm2_pcr_extend_in as not required anymore with
 use of tpm_buf
 - Moved struct tpm2_null_auth_area just before tpm2_pcr_extend() as
 it is the only function using it for now.
 - Fixed code formatting

Changelog v3:
- Rebased to the Jarkko's latest master branch (8e25809 tpm:
  Do not print an error message when doing TPM auto startup)
- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
  - Included Jarkko's feedbacks
 - Removed getcap_in, getcap_out and used tpm_buf for getting
 capability.
 - Used ARRAY_SIZE in place of TPM_MAX_PCR_BANKS and included
 other feedbacks.
- Patch "tpm: enhance TPM 2.0 PCR extend to support multiple banks"
 - Fixed kbuild errors
   - Fixed buf.data uninitialized warning.
   - Added TCG_TPM dependency on CONFIG_CRYPTO_HASH_INFO in Kconfig.

Changelog v2:

- Patch "tpm: implement TPM 2.0 capability to get active PCR banks"
  - defined structs definition in tpm2-cmd.c.
  - no_of_active_banks field is removed. Instead, constant
  TPM2_MAX_PCR_BANKS is defined.
  - renamed tpm2_get_active_pcr_banks() to tpm2_get_pcr_allocation()
  - removed generic function tpm2_get_capability().

- Patch "tpm: enchance TPM 2.0 PCR extend to support multiple banks"
 - Removed tpm2.h, and defined structs common for extend and event log
  in tpm_eventlog.h
 - uses tpm_buf in tpm2_pcr_extend().

Nayna Jain (2):
  tpm: implement TPM 2.0 capability to get active PCR banks
  tpm: enhance TPM 2.0 PCR extend to support multiple banks

 drivers/char/tpm/Kconfig |   1 +
 drivers/char/tpm/tpm-interface.c |  15 +++-
 drivers/char/tpm/tpm.h   |  14 +++-
 drivers/char/tpm/tpm2-cmd.c  | 168 +--
 4 files changed, 154 insertions(+), 44 deletions(-)

-- 
2.5.0


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH v7 2/2] tpm: enhance TPM 2.0 PCR extend to support multiple banks

2017-01-30 Thread Nayna Jain 
The current TPM 2.0 device driver extends only the SHA1 PCR bank
but the TCG Specification[1] recommends extending all active PCR
banks, to prevent malicious users from setting unused PCR banks with
fake measurements and quoting them.

The existing in-kernel interface(tpm_pcr_extend()) expects only a
SHA1 digest.  To extend all active PCR banks with differing
digest sizes, the SHA1 digest is padded with trailing 0's as needed.

This patch reuses the defined digest sizes from the crypto subsystem,
adding a dependency on CRYPTO_HASH_INFO module.

[1] TPM 2.0 Specification referred here is "TCG PC Client Specific
Platform Firmware Profile for TPM 2.0"

Signed-off-by: Nayna Jain 
---
 drivers/char/tpm/Kconfig |  1 +
 drivers/char/tpm/tpm-interface.c | 15 ++-
 drivers/char/tpm/tpm.h   |  9 +++-
 drivers/char/tpm/tpm2-cmd.c  | 91 +---
 4 files changed, 72 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 277186d..af985cc 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -6,6 +6,7 @@ menuconfig TCG_TPM
tristate "TPM Hardware Support"
depends on HAS_IOMEM
select SECURITYFS
+   select CRYPTO_HASH_INFO
---help---
  If you have a TPM security chip in your system, which
  implements the Trusted Computing Group's specification,
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 2ea16ab..7fa05a9 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -789,13 +789,26 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 
*hash)
struct tpm_cmd_t cmd;
int rc;
struct tpm_chip *chip;
+   int max_active_banks = ARRAY_SIZE(chip->active_banks);
+   struct tpm2_digest digest_list[max_active_banks];
+   u32 count = 0;
+   int i;
 
chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;
 
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-   rc = tpm2_pcr_extend(chip, pcr_idx, hash);
+   memset(digest_list, 0, sizeof(digest_list));
+
+   for (i = 0; (chip->active_banks[i] != TPM2_ALG_ERROR) &&
+(i < max_active_banks); i++) {
+   digest_list[i].alg_id = chip->active_banks[i];
+   memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
+   count++;
+   }
+
+   rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
tpm_put_ops(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index db0398a..4b7eca9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "tpm_eventlog.h"
 
@@ -380,6 +381,11 @@ struct tpm_cmd_t {
tpm_cmd_params  params;
 } __packed;
 
+struct tpm2_digest {
+   u16 alg_id;
+   u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
 /* A string buffer type for constructing TPM commands. This is based on the
  * ideas of string buffer code in security/keys/trusted.h but is heap based
  * in order to keep the stack usage minimal.
@@ -529,7 +535,8 @@ static inline inline u32 tpm2_rc_value(u32 rc)
 }
 
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
-int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
+int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
+   struct tpm2_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
 int tpm2_seal_trusted(struct tpm_chip *chip,
  struct trusted_key_payload *payload,
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 6fbd42c..60042f7 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -53,22 +53,6 @@ struct tpm2_pcr_read_out {
u8  digest[TPM_DIGEST_SIZE];
 } __packed;
 
-struct tpm2_null_auth_area {
-   __be32  handle;
-   __be16  nonce_size;
-   u8  attributes;
-   __be16  auth_size;
-} __packed;
-
-struct tpm2_pcr_extend_in {
-   __be32  pcr_idx;
-   __be32  auth_area_size;
-   struct tpm2_null_auth_area  auth_area;
-   __be32  digest_cnt;
-   __be16  hash_alg;
-   u8  digest[TPM_DIGEST_SIZE];
-} __packed;
-
 struct tpm2_get_tpm_pt_in {
__be32  cap_id;
__be32  property_id;
@@ -97,7 +81,6 @@ union tpm2_cmd_params {
struct  tpm2_self_test_in   selftest_in;
struct  tpm2_pcr_read_inpcrread_in;
struct  tpm2_pcr_read_out   pcrread_out;
-   struct

Re: [tpmdd-devel] Fwd: Re: [PATCH v9 2/2] tpm: add securityfs support, for TPM 2.0 firmware event log

2017-01-30 Thread Nayna 

> From: "Ken Goldman"  >
> Date: 26-Jan-2017 2:53 AM
> Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> support,for TPM 2.0 firmware event log
> To:  >,
>  >,
>  >
> Cc:
>
> You do not need to send a new patch set version as long as this
> one gets peer tested. And it needs to be tested without hacks
> like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> be peer tested to be more specific.
>
> For me the code itself looks good but I simply cannot take it in
> in the current situation.
>
> /Jarkko
>
>
> Tested-by: Kenneth Goldman  >
>
> I validated a firmware event log taken from a Power 8 against PCR 0-7
> values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> that same platform.
>

Thank You Ken.

Jarkko, I hope now these patches can be accepted for 4.11.

Thanks & Regards,
- Nayna


> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> 
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel