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

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

Huang, Kai wrote:
> On Wed, 2025-04-23 at 14:01 -0700, 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.
> 
> Still, is it better to print some message in tsm_unregister() to tell that some
> items have not been deleted?

Sounds reasonable.

> > 
> > 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>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 
> > ---
> >  drivers/virt/coco/tsm.c |   26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> > index 9432d4e303f1..096f4f7c0c11 100644
> > --- a/drivers/virt/coco/tsm.c
> > +++ b/drivers/virt/coco/tsm.c
> > @@ -15,6 +15,7 @@
> >  static struct tsm_provider {
> >  	const struct tsm_ops *ops;
> >  	void *data;
> > +	atomic_t count;
> >  } provider;
> >  static DECLARE_RWSEM(tsm_rwsem);
> >  
> > @@ -92,6 +93,10 @@ static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> >  	if (rc)
> >  		return rc;
> >  
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	if (!provider.ops)
> > +		return -ENXIO;
> 
> A minor thing:
> 
> I see tsm_report_read() returns -ENOTTY in the similar case:
> 
> static ssize_t tsm_report_read(struct tsm_report *report, void *buf,           
>                                size_t count, enum tsm_data_select select)      
> {       
> 	...
>         
>         /* slow path, report may need to be regenerated... */                  
>         guard(rwsem_write)(&tsm_rwsem);                                        
>         ops = provider.ops;                                                    
>         if (!ops)                                                              
>                 return -ENOTTY;
> 
> Should it be changed to -ENXIO for consistency?

Also seems reasonable, will fixup.