Re: [PATCH] configfs-tsm-report: Fix NULL dereference of tsm_ops

[Dan Williams <dan.j.williams@intel.com>]

Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/23/25 2:01 PM, Dan Williams wrote:
> > Unlike sysfs, the lifetime of configfs objects is controlled by
> > userspace. There is no mechanism for the kernel to find and delete all
> > created config-items. Instead, the configfs-tsm-report mechanism has an
> > expectation that tsm_unregister() can happen at any time and cause
> > established config-item access to start failing.
> >
> > That expectation is not fully satisfied. While tsm_report_read(),
> > tsm_report_{is,is_bin}_visible(), and tsm_report_make_item() safely fail
> > if tsm_ops have been unregistered, tsm_report_privlevel_store()
> > tsm_report_provider_show() fail to check for ops registration. Add the
> > missing checks for tsm_ops having been removed.
> >
> > Now, in supporting the ability for tsm_unregister() to always succeed,
> > it leaves the problem of what to do with lingering config-items. The
> > expectation is that the admin that arranges for the ->remove() (unbind)
> > of the ${tsm_arch}-guest driver is also responsible for deletion of all
> > open config-items. Until that deletion happens, ->probe() (reload /
> > bind) of the ${tsm_arch}-guest driver fails.
> >
> > This allows for emergency shutdown / revocation of attestation
> > interfaces, and requires coordinated restart.
> >
> > Fixes: 70e6f7e2b985 ("configfs-tsm: Introduce a shared ABI for attestation reports")
> > Cc: stable@vger.kernel.org
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Steven Price <steven.price@arm.com>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Borislav Petkov (AMD) <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Reported-by: Cedric Xing <cedric.xing@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> 
> Looks good to me
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan 
> <sathyanarayanan.kuppuswamy@linux.intel.com>

Thanks!

[..]
> >   static const struct config_item_type tsm_reports_type = {
> > @@ -459,6 +478,11 @@ int tsm_register(const struct tsm_ops *ops, void *priv)
> >   		return -EBUSY;
> >   	}
> >   
> > +	if (atomic_read(&provider.count)) {
> > +		pr_err("configfs/tsm not empty\n");
> 
> 
> Nit: I think adding the provider ops name will make the debug log clear.

Recall though that the ->name field is a tsm_ops property. At this point
tsm_ops is already unregistered. Even if we kept the name around by
strdup() at register time the name does not help solving the conflict,
only rmdir of the created configs-item unblocks the next registration.