Re: [PATCH 12/17] platform/x86/intel/ifs: Add new sysfs interfaces {status, details, cpus_ran}_list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86/intel/ifs: premature grp_cpumask update in do_core_test()
- [Medium] platform/x86/intel/ifs: unlocked grp_cpumask reads in status_list_show()
--

commit bf9eae2e7ab2791a6f3b06b483c7daa541ce8c99
Author: Jithu Joseph <jithu.joseph@intel.com>

platform/x86/intel/ifs: Add new sysfs interfaces {status, details, cpus_ran}_list

This commit adds the status_list, details_list, and cpus_ran_list sysfs
attributes to report per-CPU scan statuses, details, and the group of CPUs
tested together during an In-Field Scan run.

> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index 287f96bb033ea..82a603cd1fd34 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -135,6 +135,59 @@ static ssize_t image_version_show(struct device *dev,
>  
>  static DEVICE_ATTR_RO(image_version);
>  
> +static ssize_t status_list_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	struct ifs_test_output *pcpu_state;
> +	int lcpu, len = 0;
> +
> +	if (cpumask_weight(&ifsd->grp_cpumask) == 0)
> +		return sysfs_emit(buf, "%s\n", "none");
> +
> +	for_each_cpu(lcpu, &ifsd->grp_cpumask) {
> +		pcpu_state = per_cpu_ptr(ifsd->result_ptr, lcpu);
> +		len += sysfs_emit_at(buf, len, "%s%s", (len > 0) ? " " : "",
> +				     status_msg[pcpu_state->test_result]);
> +	}
> +	len += sysfs_emit_at(buf, len, "\n");
> +
> +	return len;
> +}

[Severity: High]
Can status_list_show(), details_list_show(), and cpus_ran_list_show() report
bogus state or dereference invalid per-CPU pointers when a write to run_test
fails validation in do_core_test()?

In drivers/platform/x86/intel/ifs/runtest.c, run_test_store() only checks
cpu < nr_cpu_ids before calling do_core_test(), which unconditionally calls
build_cpugroup_mask() before checking cpu_online(cpu) and ifsd->loaded:

drivers/platform/x86/intel/ifs/runtest.c:do_core_test() {
	cpus_read_lock();
	build_cpugroup_mask(dev, cpu);

	if (!cpu_online(cpu)) {
		dev_info(dev, "cannot test on the offline CPU %d\n", cpu);
		ret = -EINVAL;
		goto out;
	}

	switch (test->test_num) {
	case IFS_TYPE_SAF:
		if (!ifsd->loaded)
			ret = -EPERM;
		else
			ifs_test_core(cpu, dev);
		break;
}

drivers/platform/x86/intel/ifs/runtest.c:build_cpugroup_mask() {
	cpumask_clear(&ifsd->grp_cpumask);
	...
	if (!ifsd->all_lp_join) {
		cpumask_set_cpu(cpu, &ifsd->grp_cpumask);
		return;
	}

	cpumask_copy(&ifsd->grp_cpumask, topology_cluster_cpumask(cpu));
}

When do_core_test() fails with -EPERM because !ifsd->loaded,
build_cpugroup_mask() has already populated ifsd->grp_cpumask. Reading
cpus_ran_list, status_list, and details_list afterwards reports that group as
having run (or overwrites the group mask from a prior valid test while legacy
status and details keep the old results).

Similarly, when an offline or non-possible CPU < nr_cpu_ids is written to
run_test:
- If ifsd->all_lp_join is true and cpu is offline, topology_cluster_cpumask()
  is empty, wiping out ifsd->grp_cpumask from any previous valid test run.
- If ifsd->all_lp_join is true and cpu is not in cpu_possible_mask on a
  system with CONFIG_CPUMASK_OFFSTACK=y, topology_cluster_cpumask(cpu) is
  NULL, causing a NULL pointer dereference in cpumask_copy().
- If ifsd->all_lp_join is false, build_cpugroup_mask() sets the offline or
  non-possible CPU in ifsd->grp_cpumask, causing status_list_show() and
  details_list_show() to dereference per_cpu_ptr(ifsd->result_ptr, lcpu) for
  that CPU.

Should do_core_test() validate cpu_online(cpu) and ifsd->loaded before
updating ifsd->grp_cpumask in build_cpugroup_mask()?

> +
> +static DEVICE_ATTR_RO(status_list);
> +
> +static ssize_t details_list_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	struct ifs_test_output *pcpu_state;
> +	int lcpu, len = 0;
> +
> +	if (cpumask_weight(&ifsd->grp_cpumask) == 0)
> +		return sysfs_emit(buf, "%s\n", "none");
> +
> +	for_each_cpu(lcpu, &ifsd->grp_cpumask) {
> +		pcpu_state = per_cpu_ptr(ifsd->result_ptr, lcpu);
> +		len += sysfs_emit_at(buf, len, "%s%#llx", (len > 0) ? " " : "",
> +				     pcpu_state->test_details);
> +	}
> +	len += sysfs_emit_at(buf, len, "\n");
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(details_list);
> +
> +static ssize_t cpus_ran_list_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +
> +	if (cpumask_weight(&ifsd->grp_cpumask) == 0)
> +		return sysfs_emit(buf, "%s\n", "none");
> +
> +	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(&ifsd->grp_cpumask));
> +}

[Severity: Medium]
Should status_list_show(), details_list_show(), and cpus_ran_list_show()
serialize with ifs_sem when reading ifsd->grp_cpumask and the per-CPU results?

In run_test_store(), test execution is serialized under ifs_sem while
do_core_test() -> build_cpugroup_mask() clears and updates ifsd->grp_cpumask
and ifs_test_core() updates per-CPU test_details and test_result across chunk
retries.

Because status_list_show(), details_list_show(), and cpus_ran_list_show() read
ifsd->grp_cpumask twice without holding ifs_sem (first in cpumask_weight() and
then in for_each_cpu() or cpumask_pr_args()), a concurrent
build_cpugroup_mask() clearing ifsd->grp_cpumask between the two reads causes
these attributes to emit an empty line ("\n") instead of "none\n". In
addition, reading status_list or details_list while a test is in progress can
pair the newly updated ifsd->grp_cpumask with stale test_result values from a
previous run and intermediate per-chunk test_details.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026183820.129459-1-ehab.ababneh@intel.com?part=12