Re: [PATCH 3/3] platform/x86/intel/pmt: Notify userspace when crashlogs complete

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pmt: UAF on crashlog->entry.kobj in uevent_work due to probe error race
--

commit e8839d628e8b60c46da853861c58cccbcc028be6
Author: David E. Box <david.e.box@linux.intel.com>

    platform/x86/intel/pmt: Notify userspace when crashlogs complete
    
    A crashlog may already be complete when the driver binds, or it may
    complete asynchronously after userspace requests a manual trigger.
    Userspace otherwise has to poll to discover that data is ready.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index f936daf99e4d2..21e8e2199bdcc 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c

[ ... ]

> @@ -227,6 +235,47 @@ static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
>  	pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm, true);
>  }
>  
> +static void pmt_crashlog_uevent_fn(struct work_struct *work)
> +{
> +	struct crashlog_entry *crashlog =
> +		container_of(work, struct crashlog_entry, uevent_work);
> +	char *envp[] = {
> +		"INTEL_PMT_CRASHLOG_EVENT=PRESENT",
> +		"INTEL_PMT_CRASHLOG_COMPLETE=1",
> +		NULL,
> +	};
> +
> +	if (crashlog->entry.kobj)
> +		kobject_uevent_env(crashlog->entry.kobj, KOBJ_CHANGE, envp);

[Severity: High]
Can this dereference a dangling pointer in the event of a probe failure?

If intel_pmt_dev_register() in class.c fails late (e.g. devm_ioremap_resource()
failure) after exposing sysfs attributes, the error path unregisters and frees
the device, leaving entry->kobj as a dangling pointer.

If a concurrent user writes to the 'trigger' sysfs file during this window,
trigger_store() queues uevent_poll_work for 100ms later.

If the thread running pmt_crashlog_probe() is delayed before calling
cancel_delayed_work_sync(), the timer fires and the work executes, queuing
uevent_work. The uevent_work then executes kobject_uevent_env() on
crashlog->entry.kobj, which points to the freed device.

> +}

[ ... ]

> @@ -572,15 +636,29 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>  	auxiliary_set_drvdata(auxdev, priv);
>  
>  	for (i = 0; i < intel_vsec_dev->num_resources; i++) {
> -		struct intel_pmt_entry *entry = &priv->entry[priv->num_entries].entry;
> +		struct crashlog_entry *crashlog = &priv->entry[priv->num_entries];
> +		struct intel_pmt_entry *entry = &crashlog->entry;
> +
> +		/* init before dev_create() exposes trigger sysfs to userspace */
> +		INIT_WORK(&crashlog->uevent_work, pmt_crashlog_uevent_fn);
> +		INIT_DELAYED_WORK(&crashlog->uevent_poll_work,
> +				  pmt_crashlog_uevent_poll_fn);
> +		crashlog->uevent_poll_tries = 0;
>  
>  		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, intel_vsec_dev, i);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			cancel_delayed_work_sync(&crashlog->uevent_poll_work);
> +			cancel_work_sync(&crashlog->uevent_work);

[Severity: High]
Does this cancellation happen too late?

Because the cancellation happens after the underlying device may have already
been destroyed by the error path of intel_pmt_dev_create(), it leaves a race
window where the work queue can use the freed device.

>  			goto abort_probe;
> -		if (ret)
> +		}
> +		if (ret) {
> +			cancel_delayed_work_sync(&crashlog->uevent_poll_work);
> +			cancel_work_sync(&crashlog->uevent_work);
>  			continue;
> +		}
>  
>  		priv->num_entries++;
> +		pmt_crashlog_notify_pending(crashlog);
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001220351.1592138-1-david.e.box@linux.intel.com?part=3