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


Reply via email to