[PATCH] vTPM: Fix missing NULL check

[PATCH] vTPM: Fix missing NULL check

[Hon Ching(Vicky) Lo]

The current code passes the address of tpm_chip as the argument to
dev_get_drvdata() without prior NULL check in
tpm_ibmvtpm_get_desired_dma.  This resulted an oops during kernel
boot when vTPM is enabled in Power partition configured in active
memory sharing mode.

The vio_driver's get_desired_dma() is called before the probe(), which
for vtpm is tpm_ibmvtpm_probe, and it's this latter function that
initializes the driver and set data.  Attempting to get data before
the probe() caused the problem.

This patch adds a NULL check to the tpm_ibmvtpm_get_desired_dma.

fixes: 9e0d39d8a6a0 ("tpm: Remove useless priv field in struct tpm_vendor_specific")
Cc: <stable@vger.kernel.org>
Signed-off-by: Hon Ching(Vicky) Lo <honclo@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 1b9d61f..a88ee25 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -313,7 +313,10 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
 static unsigned long tpm_ibmvtpm_get_desired_dma(struct vio_dev *vdev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(&vdev->dev);
-	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	struct ibmvtpm_dev *ibmvtpm = NULL;
+
+	if (chip)
+		ibmvtpm = dev_get_drvdata(&chip->dev);
 
 	/*
 	 * ibmvtpm initializes at probe time, so the data we are
-- 
1.7.1

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Jason Gunthorpe]

On Mon, Mar 06, 2017 at 05:32:15PM -0500, Hon Ching(Vicky) Lo wrote:
> The current code passes the address of tpm_chip as the argument to
> dev_get_drvdata() without prior NULL check in
> tpm_ibmvtpm_get_desired_dma.  This resulted an oops during kernel
> boot when vTPM is enabled in Power partition configured in active
> memory sharing mode.
> 
> The vio_driver's get_desired_dma() is called before the probe(), which
> for vtpm is tpm_ibmvtpm_probe, and it's this latter function that
> initializes the driver and set data.  Attempting to get data before
> the probe() caused the problem.
> 
> This patch adds a NULL check to the tpm_ibmvtpm_get_desired_dma.

Does this also need a hunk in tpm_ibmvtpm_remove to null the drvdata
after removal, or does something in the driver code guarentee it is
null'd after remove?

We don't want to use-after-free chip on the next probe cycle.

>  static unsigned long tpm_ibmvtpm_get_desired_dma(struct vio_dev *vdev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(&vdev->dev);
> -	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
> +	struct ibmvtpm_dev *ibmvtpm = NULL;
> +
> +	if (chip)
> +		ibmvtpm = dev_get_drvdata(&chip->dev);

Maybe just do this, clearer that it is chip that can be null. We do
not want to see drivers testing their chip drvdata against null.

Also, how does locking work here? Does the vio core prevent
tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
concurrently?

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 946025a7413b6b..ced6b9f0008dc2 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -294,6 +294,8 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
 		kfree(ibmvtpm->rtce_buf);
 	}
 
+	/* For tpm_ibmvtpm_get_desired_dma */
+	dev_set_drvdata(&vdev->dev, NULL);
 	kfree(ibmvtpm);
 
 	return 0;
@@ -309,15 +311,16 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
 static unsigned long tpm_ibmvtpm_get_desired_dma(struct vio_dev *vdev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(&vdev->dev);
-	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	struct ibmvtpm_dev *ibmvtpm;
 
 	/* ibmvtpm initializes at probe time, so the data we are
 	* asking for may not be set yet. Estimate that 4K required
 	* for TCE-mapped buffer in addition to CRQ.
 	*/
-	if (!ibmvtpm)
+	if (!chip)
 		return CRQ_RES_BUF_SIZE + PAGE_SIZE;
 
+	ibmvtpm = dev_get_drvdata(&chip->dev);
 	return CRQ_RES_BUF_SIZE + ibmvtpm->rtce_size;
 }
 

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Hon Ching(Vicky) Lo]

On Mon, 2017-03-06 at 16:19 -0700, Jason Gunthorpe wrote:
> On Mon, Mar 06, 2017 at 05:32:15PM -0500, Hon Ching(Vicky) Lo wrote:
> > The current code passes the address of tpm_chip as the argument to
> > dev_get_drvdata() without prior NULL check in
> > tpm_ibmvtpm_get_desired_dma.  This resulted an oops during kernel
> > boot when vTPM is enabled in Power partition configured in active
> > memory sharing mode.
> > 
> > The vio_driver's get_desired_dma() is called before the probe(), which
> > for vtpm is tpm_ibmvtpm_probe, and it's this latter function that
> > initializes the driver and set data.  Attempting to get data before
> > the probe() caused the problem.
> > 
> > This patch adds a NULL check to the tpm_ibmvtpm_get_desired_dma.
> 
> Does this also need a hunk in tpm_ibmvtpm_remove to null the drvdata
> after removal, or does something in the driver code guarentee it is
> null'd after remove?

The driver does not ganrantee it is null'd after remove.

> 
> We don't want to use-after-free chip on the next probe cycle.
> 
> >  static unsigned long tpm_ibmvtpm_get_desired_dma(struct vio_dev *vdev)
> >  {
> >  	struct tpm_chip *chip = dev_get_drvdata(&vdev->dev);
> > -	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
> > +	struct ibmvtpm_dev *ibmvtpm = NULL;
> > +
> > +	if (chip)
> > +		ibmvtpm = dev_get_drvdata(&chip->dev);
> 
> Maybe just do this, clearer that it is chip that can be null. We do
> not want to see drivers testing their chip drvdata against null.
> 
That should do it.


> Also, how does locking work here? Does the vio core prevent
> tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
> concurrently?

No, vio core doesn't prevent tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove
from running concurrently.

vio_bus_probe calls vio_cmo_bus_probe which calls tpm_ibmvtpm_get_desired_dma.
tpm_ibmvtpm_get_desired_dma is called before the code enters critical section.

There is no locking mechanism around tpm_ibmvtpm_remove in vio_bus_remove.

What's the concern here?


> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 946025a7413b6b..ced6b9f0008dc2 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -294,6 +294,8 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
>  		kfree(ibmvtpm->rtce_buf);
>  	}
> 
> +	/* For tpm_ibmvtpm_get_desired_dma */
> +	dev_set_drvdata(&vdev->dev, NULL);
>  	kfree(ibmvtpm);
> 
>  	return 0;
> @@ -309,15 +311,16 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
>  static unsigned long tpm_ibmvtpm_get_desired_dma(struct vio_dev *vdev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(&vdev->dev);
> -	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
> +	struct ibmvtpm_dev *ibmvtpm;
> 
>  	/* ibmvtpm initializes at probe time, so the data we are
>  	* asking for may not be set yet. Estimate that 4K required
>  	* for TCE-mapped buffer in addition to CRQ.
>  	*/
> -	if (!ibmvtpm)
> +	if (!chip)
>  		return CRQ_RES_BUF_SIZE + PAGE_SIZE;
> 
> +	ibmvtpm = dev_get_drvdata(&chip->dev);
>  	return CRQ_RES_BUF_SIZE + ibmvtpm->rtce_size;
>  }
> 
> 
Thanks,
Vicky

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Jason Gunthorpe]

On Tue, Mar 07, 2017 at 11:12:43PM -0500, Hon Ching(Vicky) Lo wrote:
> On Mon, 2017-03-06 at 16:19 -0700, Jason Gunthorpe wrote:

> > Also, how does locking work here? Does the vio core prevent
> > tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
> > concurrently?
> 
> No, vio core doesn't prevent tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove
> from running concurrently.
> 
> vio_bus_probe calls vio_cmo_bus_probe which calls tpm_ibmvtpm_get_desired_dma.
> tpm_ibmvtpm_get_desired_dma is called before the code enters critical section.
> 
> There is no locking mechanism around tpm_ibmvtpm_remove in vio_bus_remove.
> 
> What's the concern here?

tpm_ibmvtpm_remove makes the pointer that tpm_ibmvtpm_get_desired_dma
is accessing invalid, so some kind of locking is technically required
so that the two things do not create a use after free race:

> > +	/* For tpm_ibmvtpm_get_desired_dma */
> > +	dev_set_drvdata(&vdev->dev, NULL);
> >  	kfree(ibmvtpm);

Eg with the kfree above.

It may be that the driver core prevents probe/remove from running
concurrently and things are fine, but this is something to confirm..

Jason

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Hon Ching(Vicky) Lo]

On Wed, 2017-03-08 at 10:17 -0700, Jason Gunthorpe wrote:
> On Tue, Mar 07, 2017 at 11:12:43PM -0500, Hon Ching(Vicky) Lo wrote:
> > On Mon, 2017-03-06 at 16:19 -0700, Jason Gunthorpe wrote:
> 
> > > Also, how does locking work here? Does the vio core prevent
> > > tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
> > > concurrently?
> > 
> > No, vio core doesn't prevent tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove
> > from running concurrently.
> > 
> > vio_bus_probe calls vio_cmo_bus_probe which calls tpm_ibmvtpm_get_desired_dma.
> > tpm_ibmvtpm_get_desired_dma is called before the code enters critical section.
> > 
> > There is no locking mechanism around tpm_ibmvtpm_remove in vio_bus_remove.
> > 
> > What's the concern here?
> 
> tpm_ibmvtpm_remove makes the pointer that tpm_ibmvtpm_get_desired_dma
> is accessing invalid, so some kind of locking is technically required
> so that the two things do not create a use after free race:
> 

I don't think we need to worry about locking in this specific case. 
tpm_ibmvtpm_get_desired_dma was designed to return a default value
in the case when the chip is not available.

There is a locking mechanism between the probe and the remove at vio
level.  The 'get_desired_dma' is called before acquiring a lock 
within the probe code is rather a design than a bug.  



Vicky

> > > +	/* For tpm_ibmvtpm_get_desired_dma */
> > > +	dev_set_drvdata(&vdev->dev, NULL);
> > >  	kfree(ibmvtpm);
> 
> Eg with the kfree above.
> 
> It may be that the driver core prevents probe/remove from running
> concurrently and things are fine, but this is something to confirm..
> 
> Jason
> 

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Jason Gunthorpe]

On Wed, Mar 08, 2017 at 03:28:11PM -0500, Hon Ching(Vicky) Lo wrote:
> On Wed, 2017-03-08 at 10:17 -0700, Jason Gunthorpe wrote:
> > On Tue, Mar 07, 2017 at 11:12:43PM -0500, Hon Ching(Vicky) Lo wrote:
> > > On Mon, 2017-03-06 at 16:19 -0700, Jason Gunthorpe wrote:
> > 
> > > > Also, how does locking work here? Does the vio core prevent
> > > > tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
> > > > concurrently?
> > > 
> > > No, vio core doesn't prevent tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove
> > > from running concurrently.
> > > 
> > > vio_bus_probe calls vio_cmo_bus_probe which calls tpm_ibmvtpm_get_desired_dma.
> > > tpm_ibmvtpm_get_desired_dma is called before the code enters critical section.
> > > 
> > > There is no locking mechanism around tpm_ibmvtpm_remove in vio_bus_remove.
> > > 
> > > What's the concern here?
> > 
> > tpm_ibmvtpm_remove makes the pointer that tpm_ibmvtpm_get_desired_dma
> > is accessing invalid, so some kind of locking is technically required
> > so that the two things do not create a use after free race:
> 
> I don't think we need to worry about locking in this specific case. 
> tpm_ibmvtpm_get_desired_dma was designed to return a default value
> in the case when the chip is not available.

You have to worry about it to prevent a use after free race:

          CPU0                                CPU1
tpm_ibmvtpm_remove()                 tpm_ibmvtpm_get_desired_dma()

				     chip = dev_get_drvdata(dev);
dev_set_drvdata(&vdev->dev, NULL);
                                     if (chip)
                 		        ibmvtpm = dev_get_drvdata(&chip->dev);
kfree(ibmvtpm);
                                        // *ibmvtpm is now a use-after-free

Jason

Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check

[Hon Ching(Vicky) Lo]

On Wed, 2017-03-08 at 13:52 -0700, Jason Gunthorpe wrote:
> On Wed, Mar 08, 2017 at 03:28:11PM -0500, Hon Ching(Vicky) Lo wrote:
> > On Wed, 2017-03-08 at 10:17 -0700, Jason Gunthorpe wrote:
> > > On Tue, Mar 07, 2017 at 11:12:43PM -0500, Hon Ching(Vicky) Lo wrote:
> > > > On Mon, 2017-03-06 at 16:19 -0700, Jason Gunthorpe wrote:
> > > 
> > > > > Also, how does locking work here? Does the vio core prevent
> > > > > tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove from running
> > > > > concurrently?
> > > > 
> > > > No, vio core doesn't prevent tpm_ibmvtpm_get_desired_dma and tpm_ibmvtpm_remove
> > > > from running concurrently.
> > > > 
> > > > vio_bus_probe calls vio_cmo_bus_probe which calls tpm_ibmvtpm_get_desired_dma.
> > > > tpm_ibmvtpm_get_desired_dma is called before the code enters critical section.
> > > > 
> > > > There is no locking mechanism around tpm_ibmvtpm_remove in vio_bus_remove.
> > > > 
> > > > What's the concern here?
> > > 
> > > tpm_ibmvtpm_remove makes the pointer that tpm_ibmvtpm_get_desired_dma
> > > is accessing invalid, so some kind of locking is technically required
> > > so that the two things do not create a use after free race:
> > 
> > I don't think we need to worry about locking in this specific case. 
> > tpm_ibmvtpm_get_desired_dma was designed to return a default value
> > in the case when the chip is not available.
> 
> You have to worry about it to prevent a use after free race:
> 
>           CPU0                                CPU1
> tpm_ibmvtpm_remove()                 tpm_ibmvtpm_get_desired_dma()
> 
> 				     chip = dev_get_drvdata(dev);
> dev_set_drvdata(&vdev->dev, NULL);  
>                                      if (chip)
>                  		        ibmvtpm = dev_get_drvdata(&chip->dev);
> kfree(ibmvtpm);
>                                         // *ibmvtpm is now a use-after-free
> 
> Jason
> 
I have dug further up along the call stack of
tpm_ibmvtpm_get_desired_dma() and found that there is a locking
mechanism in place at the bus probe level.  'probe' and 'remove'
callbacks are both surrounded by mutex_lock and mutex_unlock on the
device.  The code is in the really_probe() and
device_release_driver_internal() accordingly.  

Thanks for pointing this out!  

Vicky