Re: [tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 02:06:33PM -0800, Jarkko Sakkinen wrote:
> > 
> > Nope, this must be is_err_or_null, we store err ptrs in this array.
> 
> "
> err:
> chip->bios_dir[cnt] = NULL;
> "
> 
> There is assignment to NULL so this should be fine.

Oh good, I'm glad that made it in

Jason

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: drop chip->is_open and chip->duration_adjusted

[Jarkko Sakkinen]

On Tue, Nov 15, 2016 at 10:28:32PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 14, 2016 at 09:11:54PM -0800, Jarkko Sakkinen wrote:
> 
> > How strong is your opposition here? I do not see any exceptional damage
> > done but see some subtle but still significant benefits.
> 
> It seems OK, but I never like seeing locking made less clear - this
> should be manageable, and there isn't a performance concern with tpm
> either..

OK good to hear. I'm using this as part of my RM patch set. Just wanted
to get some feedback for this patch. Not for the next rel.

/Jarkko

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Jarkko Sakkinen]

On Wed, Nov 16, 2016 at 02:06:33PM -0800, Jarkko Sakkinen wrote:
> On Wed, Nov 16, 2016 at 01:38:53PM -0700, Jason Gunthorpe wrote:
> > On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote:
> > > Check the bios_dir entry for NULL before accessing it. Currently
> > > this crashes the driver when a TPM 2 is attached and the entries
> > > are NULL.
> > 
> > Yep
> > 
> > >   for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > > - inode = d_inode(chip->bios_dir[i]);
> > > - inode_lock(inode);
> > > - inode->i_private = NULL;
> > > - inode_unlock(inode);
> > > - securityfs_remove(chip->bios_dir[i]);
> > > + if (chip->bios_dir[i]) {
> > 
> > Nope, this must be is_err_or_null, we store err ptrs in this array.
> 
> "
> err:
> chip->bios_dir[cnt] = NULL;
> "
> 
> There is assignment to NULL so this should be fine.

Applied. Not yet squashed. I'll save that for the next week and possible
other squashes to the point when I prepare the pull request.

/Jarkko

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote:
> Check the bios_dir entry for NULL before accessing it. Currently
> this crashes the driver when a TPM 2 is attached and the entries
> are NULL.

Yep

>   for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> - inode = d_inode(chip->bios_dir[i]);
> - inode_lock(inode);
> - inode->i_private = NULL;
> - inode_unlock(inode);
> - securityfs_remove(chip->bios_dir[i]);
> + if (chip->bios_dir[i]) {

Nope, this must be is_err_or_null, we store err ptrs in this array.

Jason

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: vtpm_proxy: Do not access host's event log

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 12:07:23PM -0500, Stefan Berger wrote:
> The culprit seems to be 'tpm: fix the missing .owner in
> tpm_bios_measurements_ops'

That is unlikely, it is probably the patch before which calls read_log
unconditionally now. That suggests the crashing is a little random..

tpm_read_log_acpi should check if the chip has a acpi_dev_handle
before running, but it also shouldn't crash - so there are two bugs
here I guess.. Please do that instead of the checking the virual flag.

Jarkko, you know acpi better, we switched ppi to search starting from
the acpi_dev_handle for its data - can we do the same for event log?

> The crash looks like this:

> [  173.608722]  [] dump_stack+0x63/0x82
> [  173.608722]  [] iounmap.part.1+0x7f/0x90
> [  173.608722]  [] iounmap+0x2c/0x30
> [  173.608722]  [] acpi_os_map_cleanup.part.10+0x31/0x3e
> [  173.608722]  [] acpi_os_unmap_iomem+0xcb/0xd2
> [  173.608722]  [] read_log+0xc8/0x19e [tpm]

This seems really strange ACPI should not crash like this - yes it
will wrongly read the log for the system into the vtpm, but that
*should* work.

Are you doing anything special with this test like high parallism or
something?  Any chance you can look at little more? The tpm code looks
OK to me, the map and unmap are properly paired - but the bad address
from the iounmap suggests the refcounting in acpi is not working or
something along those lines?

Jason

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: vtpm_proxy: Do not access host's event log

[Stefan Berger]

On 11/16/2016 10:41 AM, Stefan Berger wrote:
> On 11/16/2016 10:37 AM, Jarkko Sakkinen wrote:
>> On Wed, Nov 16, 2016 at 09:24:05AM -0500, Stefan Berger wrote:
>>> The virtual TPM driver must not access the hosts's event log,
>>> otherwise we get crashes from that.
>>>
>>> Signed-off-by: Stefan Berger 
>> Can you give me a "Fixes" line (no need to send a new patch)?
>
> I haven't bisected, yet but will do that today.

The culprit seems to be 'tpm: fix the missing .owner in 
tpm_bios_measurements_ops'

'Something' now can only have a single owner?

The crash looks like this:

[  173.597916] iounmap: bad address c9000d8c
[  173.599149] CPU: 10 PID: 686 Comm: kworker/10:2 Not tainted 
4.9.0-rc5+ #578
[  173.600051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS rel-1.9.0-156-g3560877 04/01/2014
[  173.600137] Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
[  173.600137]  c900027b7c78 8140ca11 8802ad548300 
c9000d8c
[  173.600137]  c900027b7c98 8106b99f 8802ad548300 
c9000d8c
[  173.605189]  c900027b7ca8 8106b9dc c900027b7cc8 
81496c75
[  173.608722] Call Trace:
[  173.608722]  [] dump_stack+0x63/0x82
[  173.608722]  [] iounmap.part.1+0x7f/0x90
[  173.608722]  [] iounmap+0x2c/0x30
[  173.608722]  [] acpi_os_map_cleanup.part.10+0x31/0x3e
[  173.608722]  [] acpi_os_unmap_iomem+0xcb/0xd2
[  173.608722]  [] read_log+0xc8/0x19e [tpm]
[  173.608722]  [] tpm_bios_log_setup+0x31/0x170 [tpm]
[  173.608722]  [] tpm_chip_register+0x4c/0x200 [tpm]
[  173.608722]  [] vtpm_proxy_work+0x19/0x30 
[tpm_vtpm_proxy]
[  173.608722]  [] process_one_work+0x1f3/0x560
[  173.608722]  [] ? process_one_work+0x171/0x560
[  173.608722]  [] worker_thread+0x4e/0x480
[  173.608722]  [] ? process_one_work+0x560/0x560
[  173.608722]  [] ? process_one_work+0x560/0x560
[  173.608722]  [] kthread+0xf4/0x110
[  173.608722]  [] ? kthread_park+0x60/0x60
[  173.608722]  [] ret_from_fork+0x25/0x30


 Stefan

>
> Also I am wondering whether we should introduce a flag 
> TPM_CHIP_NO_FIRMWARE_LOG that is checked below. The 
> TPM_CHIP_FLAG_VIRTUAL may not be ideal, also because it is set due to 
> the device not having a parent device, which may not be related. 
> Thoughts? That new flag would only be set by the vtpm proxy driver.
>
>Stefan
>
>>
>>> ---
>>>   drivers/char/tpm/tpm_eventlog.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/char/tpm/tpm_eventlog.c 
>>> b/drivers/char/tpm/tpm_eventlog.c
>>> index fb603a7..e0abf40 100644
>>> --- a/drivers/char/tpm/tpm_eventlog.c
>>> +++ b/drivers/char/tpm/tpm_eventlog.c
>>> @@ -369,6 +369,9 @@ static int tpm_read_log(struct tpm_chip *chip)
>>>   {
>>>   int rc;
>>>   +if (chip->flags & TPM_CHIP_FLAG_VIRTUAL)
>>> +return -EFAULT;
>>> +
>>>   if (chip->log.bios_event_log != NULL) {
>>>   dev_dbg(>dev,
>>>   "%s: ERROR - event log already initialized\n",
>>> -- 
>>> 2.4.3
>>>
>> /Jarkko
>>
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH 2/4 v2] tpm/tpm2-chip: fix kdoc errors

[Tomas Winkler]

Use correct kdoc format, describe correct parameters and return values.

Signed-off-by: Tomas Winkler 
---
V2: rework and rebase of tpm_transmit_cmd kdoc fix

 drivers/char/tpm/tpm2-cmd.c | 104 
 1 file changed, 57 insertions(+), 47 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index da5b782a9731..6f9eea7eda7a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -258,11 +258,9 @@ static const struct tpm_input_header tpm2_pcrread_header = 
{
  * tpm2_pcr_read() - read a PCR value
  * @chip:  TPM chip to use.
  * @pcr_idx:   index of the PCR to read.
- * @ref_buf:   buffer to store the resulting hash,
+ * @res_buf:   buffer to store the resulting hash,
  *
- * 0 is returned when the operation is successful. If a negative number is
- * returned it remarks a POSIX error code. If a positive number is returned
- * it remarks a TPM error.
+ * Return: same as with tpm_transmit_cmd
  */
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 {
@@ -304,13 +302,12 @@ static const struct tpm_input_header 
tpm2_pcrextend_header = {
 
 /**
  * tpm2_pcr_extend() - extend a PCR value
+ *
  * @chip:  TPM chip to use.
  * @pcr_idx:   index of the PCR.
  * @hash:  hash value to use for the extend operation.
  *
- * 0 is returned when the operation is successful. If a negative number is
- * returned it remarks a POSIX error code. If a positive number is returned
- * it remarks a TPM error.
+ * Return: same as with tpm_transmit_cmd
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
@@ -348,13 +345,14 @@ static const struct tpm_input_header 
tpm2_getrandom_header = {
 
 /**
  * tpm2_get_random() - get random bytes from the TPM RNG
+ *
  * @chip: TPM chip to use
  * @out: destination buffer for the random bytes
  * @max: the max number of bytes to write to @out
  *
- * 0 is returned when the operation is successful. If a negative number is
- * returned it remarks a POSIX error code. If a positive number is returned
- * it remarks a TPM error.
+ * Return:
+ *size of the output buffer;
+ *-EIO on error
  */
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 {
@@ -404,15 +402,16 @@ static const struct tpm_input_header 
tpm2_get_tpm_pt_header = {
 };
 
 /**
- * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with
- * tpm_buf_alloc().
- *
- * @param buf: an allocated tpm_buf instance
- * @param nonce: the session nonce, may be NULL if not used
- * @param nonce_len: the session nonce length, may be 0 if not used
- * @param attributes: the session attributes
- * @param hmac: the session HMAC or password, may be NULL if not used
- * @param hmac_len: the session HMAC or password length, maybe 0 if not used
+ * tpm_buf_alloc() - Append TPMS_AUTH_COMMAND to the buffer.
+ *  The buffer must be allocated with
+ *
+ * @buf: an allocated tpm_buf instance
+ * @session_handle: session handle
+ * @nonce: the session nonce, may be NULL if not used
+ * @nonce_len: the session nonce length, may be 0 if not used
+ * @attributes: the session attributes
+ * @hmac: the session HMAC or password, may be NULL if not used
+ * @hmac_len: the session HMAC or password length, maybe 0 if not used
  */
 static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
 const u8 *nonce, u16 nonce_len,
@@ -435,7 +434,8 @@ static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 
session_handle,
 
 /**
  * tpm2_seal_trusted() - seal the payload of a trusted key
- * @chip_num: TPM chip to use
+ *
+ * @chip: TPM chip to use
  * @payload: the key data in clear and encrypted form
  * @options: authentication values and other options
  *
@@ -540,11 +540,17 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 
 /**
  * tpm2_load_cmd() - execute a TPM2_Load command
- * @chip_num: TPM chip to use
+ *
+ * @chip: TPM chip to use
  * @payload: the key data in clear and encrypted form
  * @options: authentication values and other options
+ * @blob_handle: returned blob handle
+ * @flags: tpm transmit flags
  *
- * Return: same as with tpm_transmit_cmd
+ * Return: 0 on success
+ *-E2BIG on wrong payload size
+ *-EPERM on tpm error status
+ *< 0 error from tpm_transmit_cmd
  */
 static int tpm2_load_cmd(struct tpm_chip *chip,
 struct trusted_key_payload *payload,
@@ -600,9 +606,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 
 /**
  * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
- * @chip_num: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
+ *
+ * @chip: TPM chip to use
+ * @handle: the key data in clear and encrypted form
+ * @flags: tpm transmit flags
  *
  * Return: same as with tpm_transmit_cmd
  */
@@ -632,11 +639,16 @@ static void 

Re: [tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Jarkko Sakkinen]

On Wed, Nov 16, 2016 at 08:28:30PM +0530, Nayna wrote:
> 
> 
> On 11/16/2016 07:26 PM, Stefan Berger wrote:
> > Check the bios_dir entry for NULL before accessing it. Currently
> > this crashes the driver when a TPM 2 is attached and the entries
> > are NULL.
> 
> Thanks Stefan !! I think it would be good to also add Fixes in commit
> description here.
> 
> Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files)

Not mandatory as this is not in the mainline tree but doesn't hurt
either.

/Jarkko

> 
> Thanks & Regards,
>   - Nayna
> 
> > 
> > Signed-off-by: Stefan Berger 
> > ---
> >   drivers/char/tpm/tpm_eventlog.c | 12 +++-
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_eventlog.c 
> > b/drivers/char/tpm/tpm_eventlog.c
> > index ebec4ac..fb603a7 100644
> > --- a/drivers/char/tpm/tpm_eventlog.c
> > +++ b/drivers/char/tpm/tpm_eventlog.c
> > @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
> >  * This design ensures that open() either safely gets kref or fails.
> >  */
> > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > -   inode = d_inode(chip->bios_dir[i]);
> > -   inode_lock(inode);
> > -   inode->i_private = NULL;
> > -   inode_unlock(inode);
> > -   securityfs_remove(chip->bios_dir[i]);
> > +   if (chip->bios_dir[i]) {
> > +   inode = d_inode(chip->bios_dir[i]);
> > +   inode_lock(inode);
> > +   inode->i_private = NULL;
> > +   inode_unlock(inode);
> > +   securityfs_remove(chip->bios_dir[i]);
> > +   }
> > }
> >   }
> > 
> 

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [patch] tpm: uninitialized variable in read_log_of()

[Jarkko Sakkinen]

On Wed, Nov 16, 2016 at 05:12:21PM +0300, Dan Carpenter wrote:
> "np" is supposed to be set to NULL at the start.
> 
> Fixes: 4a45d9669ac1 ("tpm: replace of_find_node_by_name() with dev of_node 
> propert")
> Signed-off-by: Dan Carpenter 

Colid King submitted a patch to fix this a couple of days ago so
applied that. Anyway, thank you for noting this.

> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 3af829f..904ed4a 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -23,7 +23,7 @@
>  
>  int read_log_of(struct tpm_chip *chip)
>  {
> - struct device_node *np;
> + struct device_node *np = NULL;
>   const u32 *sizep;
>   const u64 *basep;
>   struct tpm_bios_log *log;

/Jarkko

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Nayna]

On 11/16/2016 07:26 PM, Stefan Berger wrote:
> Check the bios_dir entry for NULL before accessing it. Currently
> this crashes the driver when a TPM 2 is attached and the entries
> are NULL.

Thanks Stefan !! I think it would be good to also add Fixes in commit 
description here.

Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files)

Thanks & Regards,
   - Nayna

>
> Signed-off-by: Stefan Berger 
> ---
>   drivers/char/tpm/tpm_eventlog.c | 12 +++-
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index ebec4ac..fb603a7 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
>* This design ensures that open() either safely gets kref or fails.
>*/
>   for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> - inode = d_inode(chip->bios_dir[i]);
> - inode_lock(inode);
> - inode->i_private = NULL;
> - inode_unlock(inode);
> - securityfs_remove(chip->bios_dir[i]);
> + if (chip->bios_dir[i]) {
> + inode = d_inode(chip->bios_dir[i]);
> + inode_lock(inode);
> + inode->i_private = NULL;
> + inode_unlock(inode);
> + securityfs_remove(chip->bios_dir[i]);
> + }
>   }
>   }
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH] tpm: vtpm_proxy: Do not access host's event log

[Stefan Berger]

The virtual TPM driver must not access the hosts's event log,
otherwise we get crashes from that.

Signed-off-by: Stefan Berger 
---
 drivers/char/tpm/tpm_eventlog.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index fb603a7..e0abf40 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -369,6 +369,9 @@ static int tpm_read_log(struct tpm_chip *chip)
 {
int rc;
 
+   if (chip->flags & TPM_CHIP_FLAG_VIRTUAL)
+   return -EFAULT;
+
if (chip->log.bios_event_log != NULL) {
dev_dbg(>dev,
"%s: ERROR - event log already initialized\n",
-- 
2.4.3


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[tpmdd-devel] [PATCH] tpm: Check the bios_dir entry for NULL before accessing it

[Stefan Berger]

Check the bios_dir entry for NULL before accessing it. Currently
this crashes the driver when a TPM 2 is attached and the entries
are NULL.

Signed-off-by: Stefan Berger 
---
 drivers/char/tpm/tpm_eventlog.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index ebec4ac..fb603a7 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
 * This design ensures that open() either safely gets kref or fails.
 */
for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
-   inode = d_inode(chip->bios_dir[i]);
-   inode_lock(inode);
-   inode->i_private = NULL;
-   inode_unlock(inode);
-   securityfs_remove(chip->bios_dir[i]);
+   if (chip->bios_dir[i]) {
+   inode = d_inode(chip->bios_dir[i]);
+   inode_lock(inode);
+   inode->i_private = NULL;
+   inode_unlock(inode);
+   securityfs_remove(chip->bios_dir[i]);
+   }
}
 }
-- 
2.4.3


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel