Re: [PATCH v4 04/18] platform/x86/intel/pmt: Add register access callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In `pmt_crashlog_rc()`, an error from `pmt_crashlog_read_reg()` incorrectly returns `false` (which evaluates to integer `0`) instead of propagating the error code. This tricks callers into assuming the operation succeeded, leading to the use of uninitialized stack variables.
--

commit e8f90b7603dd0762e0fc5325b88546ddb36185c1
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

platform/x86/intel/pmt: Add register access callbacks

Some HW does not have direct MMIO access to PMT control and data
features. Augment the current callback infrastructure to allow a
registered driver to customize read/write access.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
[ ... ]
> @@ -147,7 +181,13 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  static int pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit, bool *state)
>  {
>  	const struct crashlog_status *status = &crashlog->info->status;
> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
> +	struct intel_pmt_entry *entry = &crashlog->entry;
> +	u32 reg;
> +	int ret;
> +
> +	ret = pmt_crashlog_read_reg(entry, &reg, status->offset);
> +	if (ret)
> +		return false;

[Severity: High]
Does returning false here incorrectly mask the error code as success?

The function pmt_crashlog_rc() returns an int. Returning false evaluates to
0, which tricks callers into thinking the register read was successful.

For example, when a sysfs attribute calls clear_show():

drivers/platform/x86/intel/pmt/crashlog.c:clear_show() {
    bool cleared;
    int ret;

    ret = pmt_crashlog_cleared(crashlog, &cleared);
    if (ret)
        return sysfs_emit(buf, "read error: %d\n", ret);
    else
        return sysfs_emit(buf, "%d\n", cleared);
}

If pmt_crashlog_read_reg() fails and returns an error code, ret will be 0,
and sysfs_emit() will leak the uninitialized stack variable cleared to
userspace.

A similar issue occurs in trigger_store(), where the code will branch on an
uninitialized disabled variable if the register read fails. Should this
instead propagate the ret value?

>  
>  	*state = !!(reg & bit);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901192736.626777-20-michael.j.ruhl@intel.com?part=4