Subject: Re: [tpmdd-devel] [PATCH] vTPM: Fix missing NULL check
From: "Hon Ching(Vicky) Lo" <honclo@linux.vnet.ibm.com>
To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: tpmdd-devel@lists.sourceforge.net, Ashley Lai <ashley@ashleylai.com>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Peter Huewe <PeterHuewe@gmx.de>
In-Reply-To: <20170306231946.GA23953@obsidianresearch.com>
References: <1488839535-11822-1-git-send-email-honclo@linux.vnet.ibm.com>
         <20170306231946.GA23953@obsidianresearch.com>
Content-Type: text/plain; charset="UTF-8"
Date: Tue, 07 Mar 2017 23:12:43 -0500
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Message-Id: <1488946363.15595.17.camel@vtpm2014.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3164
Lines: 91

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