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

[Nayna]

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 ?

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
>
> /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

[James Bottomley]

On Sun, 2017-01-29 at 19:35 -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.

It is ... it's just this patch only covers sessions.

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] [RFC] tpm2-space: add handling for global session exhaustion

[Ken Goldman]

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.  Just allocate them in triples so there's no 
deadlock.

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.

~~

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.

~~

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




--
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

[Ken Goldman]

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.




--
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] Fixing TPM_RC_CONTEXT_GAP

[Ken Goldman]

On 1/29/2017 10:25 AM, Jarkko Sakkinen wrote:
>
> We should probably block the following from /dev/tpms0 by default:
>
> 1. ContextLoad for sessions.
> 2. ContextSave for sessions.
> 3. Vendor specific commands.

This is reasonable to me.  The only vendor specific commands I currently 
know of are for low level device configuration.  It's done in OEM 
manufacturing and then locked.

Eventually, we'll want to block extends to certain PCRs.  E.g., the IMA 
PCR (10) should be accessibly to the kernel but not user space.





--
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

[James Bottomley]

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;
> > +   }
> > +
> > return 0;
> >  }
> >  
> > -void tpm2_del_space(struct tpm_space *space)
> > +void tpm2_del_space(struct tpm_chip *chip, struct tpm_space
> > *space)
> >  {
> > +   tpm2_kill_sessions(chip, space);
> > kfree(space->context_buf);
> > +   

Re: [tpmdd-devel] [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Fri, Jan 27, 2017 at 04:33:54PM -0800, James Bottomley wrote:
> In a TPM2, sessions can be globally exhausted once there are
> TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context saved).
> The Strategy for handling this is to keep a global count of all the
> sessions along with their creation time.  Then if we see the TPM run
> out of sessions (via the TPM_RC_SESSION_HANDLES) we first wait for one
> to become free, but if it doesn't, we forcibly evict an existing one.
> The eviction strategy waits until the current command is repeated to
> evict the session which should guarantee there is an available slot.
> 
> On the force eviction case, we make sure that the victim session is at
> least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue for
> session slots is a FIFO one, ensuring that once we run out of
> sessions, everyone will get a session in a bounded time and once they
> get one, they'll have SESSION_TIMEOUT to use it before it may be
> subject to eviction.
> 
> Signed-off-by: James Bottomley 
> ---
>  drivers/char/tpm/tpm-chip.c   |   1 +
>  drivers/char/tpm/tpm.h|  39 +++-
>  drivers/char/tpm/tpm2-cmd.c   |  15 +++
>  drivers/char/tpm/tpm2-space.c | 209 
> --
>  drivers/char/tpm/tpms-dev.c   |  17 +++-
>  5 files changed, 271 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 6282ad0..150c6b8 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -164,6 +164,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  
>   mutex_init(>tpm_mutex);
>   init_rwsem(>ops_sem);
> + init_waitqueue_head(>session_wait);
>  
>   chip->ops = ops;
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 10c57b9..658e5e2 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -95,7 +95,8 @@ enum tpm2_return_codes {
>   TPM2_RC_HANDLE  = 0x008B,
>   TPM2_RC_INITIALIZE  = 0x0100, /* RC_VER1 */
>   TPM2_RC_DISABLED= 0x0120,
> - TPM2_RC_TESTING = 0x090A, /* RC_WARN */
> + TPM2_RC_SESSION_HANDLES = 0x0905, /* RC_WARN */
> + TPM2_RC_TESTING = 0x090A,
>   TPM2_RC_REFERENCE_H0= 0x0910,
>  };
>  
> @@ -139,7 +140,8 @@ enum tpm2_capabilities {
>  };
>  
>  enum tpm2_properties {
> - TPM_PT_TOTAL_COMMANDS   = 0x0129,
> + TPM_PT_TOTAL_COMMANDS   = 0x0129,
> + TPM_PT_ACTIVE_SESSIONS_MAX  = 0x0111,
>  };
>  
>  enum tpm2_startup_types {
> @@ -163,8 +165,24 @@ struct tpm_space {
>   u8 *context_buf;
>   u32 session_tbl[3];
>   u8 *session_buf;
> + u32 reserved_handle;
>  };
>  
> +#define TPM2_HANDLE_FORCE_EVICT 0x
> +
> +static inline void tpm2_session_force_evict(struct tpm_space *space)
> +{
> + /* if reserved handle is not empty, we already have a
> +  * session for eviction, so no need to force one
> +  */
> + if (space->reserved_handle == 0)
> + space->reserved_handle = TPM2_HANDLE_FORCE_EVICT;
> +}
> +static inline bool tpm2_is_session_force_evict(struct tpm_space *space)
> +{
> + return space->reserved_handle == TPM2_HANDLE_FORCE_EVICT;
> +}
> +
>  enum tpm_chip_flags {
>   TPM_CHIP_FLAG_TPM2  = BIT(1),
>   TPM_CHIP_FLAG_IRQ   = BIT(2),
> @@ -177,6 +195,12 @@ struct tpm_chip_seqops {
>   const struct seq_operations *seqops;
>  };
>  
> +struct tpm_sessions {
> + struct tpm_space *space;
> + u32 handle;
> + unsigned long created;
> +};

I would rethink this a bit. I kind of dislike this structure as it

I would rather have 

struct tpm_session {
u32 handle;
unsigned long created;
};

and in struct tpm_space:

struct tpm_session session_tbl[3];
struct list_head session_list;

and keep those instances that have sessions in that linked list.

What do you think? 

I'll study the actual functionality in this patch properly later.

/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

[Jarkko Sakkinen]

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 
> 
> ---
> 
> 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;
> + }
> +
>   return 0;
>  }
>  
> -void tpm2_del_space(struct tpm_space *space)
> +void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>  {
> + tpm2_kill_sessions(chip, space);
>   kfree(space->context_buf);
> + kfree(space->session_buf);
>  }
>  
>  static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> @@ -69,6 +79,10 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 
> *buf,
>__func__, rc);
>   tpm_buf_destroy();
>   return -EFAULT;
> + } 

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

[Jarkko Sakkinen]

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.

/Jarkko

/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] Fixing TPM_RC_CONTEXT_GAP

[Jarkko Sakkinen]

On Fri, Jan 27, 2017 at 04:49:36PM -0500, Ken Goldman wrote:
> On 1/25/2017 8:04 PM, James Bottomley wrote:
> 
> > This leads to a problem: we have to have access to the session context
> > to pull this trick, and that means we have to disallow TPM users from
> > calling ContextSave on a session otherwise they could DoS us by
> > inducing an unremediable TPM_RC_CONTEXT_GAP error (simply by keeping
> > the saved session and never loading it).
> 
> I think it's perfectly acceptable to block applications from calling 
> context save for sessions.  I don't know of any use case that would 
> require it.
> 
> (There are definitely use cases for context save on transient objects, 
> but they don't have the replay / gap issue.)

We should probably block the following from /dev/tpms0 by default:

1. ContextLoad for sessions.
2. ContextSave for sessions.
3. Vendor specific commands.

/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 v6 0/2] enhance TPM 2.0 extend function to support multiple PCR banks

[Jarkko Sakkinen]

On Fri, Jan 27, 2017 at 10:53:11PM +0530, Nayna wrote:
> 
> 
> On 01/26/2017 02:15 AM, Jarkko Sakkinen wrote:
> > On Fri, Jan 20, 2017 at 12:05:11PM -0500, Nayna Jain wrote:
> > > 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"
> > 
> > I pushed these patches. I had to resolve merge conflicts caused
> > by the min_rsp_body_length parameter in tpm_transmit_cmd. Can you
> > verify that I didn't break anything?
> > 
> 
> Yes, it looks fine, also tested with it.
> 
> Just to understand.. how did you decide min_rsp_body_length to be 9.
> If I understood correctly, I think it is the size after header, till last
> fixed parameter i.e.  till count.
> Is the assumption is that count can be zero, such that there is no active
> bank for PCR and so no struct tpm2_pcr_selection ?
> 
> Thanks & Regards,
>- Nayna

Hmm.. I got it from "marker = [TPM_HEADER_SIZE + 9];" i.e. must
have at last 9 bytes after the header.

/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()

[Jarkko Sakkinen]

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.

>   int rc;
> - int i;
> + int i = 0;

Why do you need to initialize it?

>  
>   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?

> +
>   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);
> - marker = marker + sizeof(struct tpm2_pcr_selection);
> + sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
> + sizeof(pcr_selection.size_of_select) +
> + sizeof(u8) * pcr_selection.size_of_select;

Remove "sizeof(u8) * ".

> + marker = marker + sizeof_pcr_selection;
>   }
>  
>  out:
> - if (count < ARRAY_SIZE(chip->active_banks))
> - chip->active_banks[count] = TPM2_ALG_ERROR;
> + if (i < ARRAY_SIZE(chip->active_banks))
> + chip->active_banks[i] = TPM2_ALG_ERROR;
>  
>   tpm_buf_destroy();
>  
> -- 
> 2.5.0
> 

I'm sorry but this commit is changing too much. You need to redo the
whole commit and resend the patch set with these fixes. You can keep
Reviewed-by and Tested-by in 1/2 but have to remove them from 2/2.

/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