Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 5 Feb 2021 21:02:23 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     James Bottomley <James.Bottomley@HansenPartnership.com>
Cc:     Jarkko Sakkinen <jarkko@kernel.org>,
        Lino Sanfilippo <LinoSanfilippo@gmx.de>, peterhuewe@gmx.de,
        stefanb@linux.vnet.ibm.com, stable@vger.kernel.org,
        linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lino Sanfilippo <l.sanfilippo@kunbus.com>
Subject: Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is
 still valid
Message-ID: <20210206010223.GS4718@ziepe.ca>
References: <1612482643-11796-1-git-send-email-LinoSanfilippo@gmx.de>
 <1612482643-11796-3-git-send-email-LinoSanfilippo@gmx.de>
 <7308e5e9f51501bd92cced8f28ff6130c976b3ed.camel@HansenPartnership.com>
 <YByrCnswkIlz1w1t@kernel.org>
 <ee4adfbb99273e1bdceca210bc1fa5f16a50c415.camel@HansenPartnership.com>
 <20210205172528.GP4718@ziepe.ca>
 <89892a6152826e89276126fd2688b7c767484f41.camel@HansenPartnership.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <89892a6152826e89276126fd2688b7c767484f41.camel@HansenPartnership.com>
Precedence: bulk

On Fri, Feb 05, 2021 at 09:54:29AM -0800, James Bottomley wrote:
> On Fri, 2021-02-05 at 13:25 -0400, Jason Gunthorpe wrote:
> > On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
> > > > Thanks for pointing this out. I'd strongly support Jason's
> > > > proposal:
> > > > 
> > > > https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
> > > > 
> > > > It's the best long-term way to fix this.
> > > 
> > > Really, no it's not.  It introduces extra mechanism we don't need.
> > > To recap the issue: character devices already have an automatic
> > > mechanism which holds a reference to the struct device while the
> > > character node is open so the default is to release resources on
> > > final
> > > put of the struct device.
> > 
> > The refcount on the struct device only keeps the memory alive, it
> > doesn't say anything about the ops. We still need to lock and check
> > the ops each and every time they are used.
> 
> I think this is the crux of our disagreement: I think the ops doesn't
> matter because to call try_get_ops you have to have a chip structure
> and the only way you get a chip structure is if you hold a device
> containing it, in which case the device hold guarantees the chip can't
> be freed.  

The get_device() only guarentees the chip memory hasn't been kfree'd.

It doesn't mean tpm_chip_unregister() hasn't already run, completed
and set ops == NULL.

In the file path we have the get_device implicitly by the file's
i_cdev pointing to that chain of refcounts that ends on the chip's
main struct device. So we know the chip memory cannot be kfreed while
the struct file exists.

However, there is nothing preventing the struct file from living past
tpm_chip_unregister(). cdev_device_del() does not wait for all files's
to be closed, it only removes the ability to open new files. Open
files do prevent removal of the module, but it does not prevent
hot-unplug of the underling device, eg with sysfs unbind.

In fact, nothing about tpm_chip_unregister() excludes open files.

So it is perfectly legal for tpm_chip_unregister() to return, the devm
put_device to be called, and the refcount of the chip to still be
positive - held by open files.

In this situation ops will be NULL when file operations are called and
eg doing a tpm_chip_start will crash on:

	if (chip->ops->clk_enable)

To use the TPM driver, the rules are you must hold a get_device() on a
chip, and then upgrade it to a 'tpm_try_get_ops' before calling any
driver functions. 

Only the critical region formed by tpm_try_get_ops() will prevent
tpm_chip_unregister() from completing. It is the thing that ensures
the driver is actually present.

> In either case, I think you get returned a device to which you hold a
> reference.  Is there any other case where you can get a chip without
> also getting a device reference?

There should be no case where there is a struct chip pointer without
something owning a reference for that pointer.

> I'll answer the other point in a separate email, but I think the
> principle sounds OK: we could do the final put right after we del the
> char devices because that's called in the module release routine and
> thus not have to rely on the devm actions which, as you say, are an
> annoying complication.

I think tpm_alloc() should have an error unwind that is only
put_device(chip->dev), anything else breaks the basic programming
pattern of alloc/register.

Jason