diff mbox

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

Message ID 20161119183255.GB22775@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Nov. 19, 2016, 6:32 p.m. UTC 
On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:

> >>Further, I had the impression that the error unwinding following -ENODEV has
> >>an issue related to sysfs.
> >I don't follow this comment..
> 
> I have encountered this error here, which gets masked when applying the
> previously shown patch.

If tpm_chip_register fails vtpm must not call tpm_chip_unregister:

> [   58.271017]  [<ffffffff8155bd32>] dpm_sysfs_remove+0x22/0x60
> [   58.271017]  [<ffffffff8154e438>] device_del+0x58/0x280
> [   58.271017]  [<ffffffffa024c020>] tpm_chip_unregister+0x40/0xb0 [tpm]
> [   58.271017]  [<ffffffffa0292360>] vtpm_proxy_fops_release+0x40/0x60 [tpm_vtpm_proxy]

So, this is a vtpm thing I missed for 'tpm: Get rid of TPM_CHIP_FLAG_REGISTERED'

Does this do the trick?



------------------------------------------------------------------------------

Comments

Stefan Berger Nov. 20, 2016, 2:46 p.m. UTC | #1 
On 11/19/2016 01:32 PM, Jason Gunthorpe wrote:
> On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
>
>>>> Further, I had the impression that the error unwinding following -ENODEV has
>>>> an issue related to sysfs.
>>> I don't follow this comment..
>> I have encountered this error here, which gets masked when applying the
>> previously shown patch.
> If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
>
>> [   58.271017]  [<ffffffff8155bd32>] dpm_sysfs_remove+0x22/0x60
>> [   58.271017]  [<ffffffff8154e438>] device_del+0x58/0x280
>> [   58.271017]  [<ffffffffa024c020>] tpm_chip_unregister+0x40/0xb0 [tpm]
>> [   58.271017]  [<ffffffffa0292360>] vtpm_proxy_fops_release+0x40/0x60 [tpm_vtpm_proxy]
> So, this is a vtpm thing I missed for 'tpm: Get rid of TPM_CHIP_FLAG_REGISTERED'
>
> Does this do the trick?

Yes, it does the trick.

I tested this now with your other fix-patch applied to Jarkko's tree. I 
injected a fault by returning -EIO from tpm_read_log_acpi(), which 
otherwise would cause the crash.

Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 3d6f6ca81def75..d3a41f9d65c85c 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -42,6 +42,7 @@ struct proxy_dev {
>   	long state;                  /* internal state */
>   #define STATE_OPENED_FLAG        BIT(0)
>   #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
> +#define STATE_REGISTERED_FLAG	 BIT(2)
>
>   	size_t req_len;              /* length of queued TPM request */
>   	size_t resp_len;             /* length of queued TPM response */
> @@ -372,6 +373,7 @@ static void vtpm_proxy_work(struct work_struct *work)
>   	if (rc)
>   		goto err;
>
> +	proxy_dev->state |= STATE_REGISTERED_FLAG;
>   	return;
>
>   err:
> @@ -516,7 +518,8 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev)
>   	 */
>   	vtpm_proxy_fops_undo_open(proxy_dev);
>
> -	tpm_chip_unregister(proxy_dev->chip);
> +	if (proxy_dev->state & STATE_REGISTERED_FLAG)
> +		tpm_chip_unregister(proxy_dev->chip);
>
>   	vtpm_proxy_delete_proxy_dev(proxy_dev);
>   }
>


------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 22, 2016, 6:07 a.m. UTC | #2 
On Sat, Nov 19, 2016 at 11:32:55AM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
> 
> > >>Further, I had the impression that the error unwinding following -ENODEV has
> > >>an issue related to sysfs.
> > >I don't follow this comment..
> > 
> > I have encountered this error here, which gets masked when applying the
> > previously shown patch.
> 
> If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
> 
> > [   58.271017]  [<ffffffff8155bd32>] dpm_sysfs_remove+0x22/0x60
> > [   58.271017]  [<ffffffff8154e438>] device_del+0x58/0x280
> > [   58.271017]  [<ffffffffa024c020>] tpm_chip_unregister+0x40/0xb0 [tpm]
> > [   58.271017]  [<ffffffffa0292360>] vtpm_proxy_fops_release+0x40/0x60 [tpm_vtpm_proxy]
> 
> So, this is a vtpm thing I missed for 'tpm: Get rid of TPM_CHIP_FLAG_REGISTERED'
> 
> Does this do the trick?

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 3d6f6ca81def75..d3a41f9d65c85c 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -42,6 +42,7 @@ struct proxy_dev {
>  	long state;                  /* internal state */
>  #define STATE_OPENED_FLAG        BIT(0)
>  #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
> +#define STATE_REGISTERED_FLAG	 BIT(2)
>  
>  	size_t req_len;              /* length of queued TPM request */
>  	size_t resp_len;             /* length of queued TPM response */
> @@ -372,6 +373,7 @@ static void vtpm_proxy_work(struct work_struct *work)
>  	if (rc)
>  		goto err;
>  
> +	proxy_dev->state |= STATE_REGISTERED_FLAG;
>  	return;
>  
>  err:
> @@ -516,7 +518,8 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev)
>  	 */
>  	vtpm_proxy_fops_undo_open(proxy_dev);
>  
> -	tpm_chip_unregister(proxy_dev->chip);
> +	if (proxy_dev->state & STATE_REGISTERED_FLAG)
> +		tpm_chip_unregister(proxy_dev->chip);
>  
>  	vtpm_proxy_delete_proxy_dev(proxy_dev);
>  }
> 

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 22, 2016, 6:09 a.m. UTC | #3 
On Tue, Nov 22, 2016 at 08:07:42AM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 19, 2016 at 11:32:55AM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
> > 
> > > >>Further, I had the impression that the error unwinding following -ENODEV has
> > > >>an issue related to sysfs.
> > > >I don't follow this comment..
> > > 
> > > I have encountered this error here, which gets masked when applying the
> > > previously shown patch.
> > 
> > If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
> > 
> > > [   58.271017]  [<ffffffff8155bd32>] dpm_sysfs_remove+0x22/0x60
> > > [   58.271017]  [<ffffffff8154e438>] device_del+0x58/0x280
> > > [   58.271017]  [<ffffffffa024c020>] tpm_chip_unregister+0x40/0xb0 [tpm]
> > > [   58.271017]  [<ffffffffa0292360>] vtpm_proxy_fops_release+0x40/0x60 [tpm_vtpm_proxy]
> > 
> > So, this is a vtpm thing I missed for 'tpm: Get rid of TPM_CHIP_FLAG_REGISTERED'
> > 
> > Does this do the trick?
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

------------------------------------------------------------------------------
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 3d6f6ca81def75..d3a41f9d65c85c 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -42,6 +42,7 @@  struct proxy_dev {
 	long state;                  /* internal state */
 #define STATE_OPENED_FLAG        BIT(0)
 #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
+#define STATE_REGISTERED_FLAG	 BIT(2)
 
 	size_t req_len;              /* length of queued TPM request */
 	size_t resp_len;             /* length of queued TPM response */
@@ -372,6 +373,7 @@  static void vtpm_proxy_work(struct work_struct *work)
 	if (rc)
 		goto err;
 
+	proxy_dev->state |= STATE_REGISTERED_FLAG;
 	return;
 
 err:
@@ -516,7 +518,8 @@  static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev)
 	 */
 	vtpm_proxy_fops_undo_open(proxy_dev);
 
-	tpm_chip_unregister(proxy_dev->chip);
+	if (proxy_dev->state & STATE_REGISTERED_FLAG)
+		tpm_chip_unregister(proxy_dev->chip);
 
 	vtpm_proxy_delete_proxy_dev(proxy_dev);
 }