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

["Huang, Kai" <kai.huang@intel.com>]

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?

> 
> 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?