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
