Re: [PATCH v01] mailbox/pcc.c: ignore errors on type 4 channels.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for Type 4 slave channels leaves a probe-time failure when the Error Status Register is properly zeroed by firmware.
- [High] Out-of-bounds read and memory corruption when parsing ACPI PCCT Type 5 channels.
--

commit 682ec44d669bd318031fe1a1eb86dca6cabd5735
Author: Adam Young <admiyo@os.amperecomputing.com>

mailbox/pcc.c: ignore errors on type 4 channels.

The ACPI spec states that the Error status register is ignored by the
OSPM on slave channels (Type 4). This commit skips checking and
clearing errors for ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE.

Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#hw-registers-based-communications-subspace-structure-type-5

[Severity: High]
This is a pre-existing issue, but does the driver handle Type 5 channels
safely here?

In pcc_parse_subspace_db_reg() and pcc_parse_subspace_shmem(), channels
with types greater than ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2 are
unconditionally cast to the Type 3/4 struct:

drivers/mailbox/pcc.c:pcc_parse_subspace_db_reg() {
        ...
        } else {
                struct acpi_pcct_ext_pcc_master *pcct_ext;

                pcct_ext = (struct acpi_pcct_ext_pcc_master *)pcct_entry;
        ...
}

The Type 5 structure (struct acpi_pcct_hw_reg) is 96 bytes long, while
the Type 3/4 struct is 164 bytes.

Can casting a Type 5 entry to the Type 3/4 structure cause the driver to
read misaligned garbage memory past the end of the Type 5 structure into
the next ACPI table? Could this populate GAS structures with arbitrary
data, leading pcc_chan_reg_init() to potentially map incorrect physical
addresses via acpi_os_ioremap(), and misread the shmem base_address and
length due to offset mismatches?

> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..0deaf7907ed66 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -270,6 +270,9 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>  	u64 val;
>  	int ret;
>  
> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but does this fix need to be applied to
the probe path as well?

While this skips reading the Error Status Register at runtime,
pcc_parse_subspace_db_reg() still unconditionally parses and initializes
it for Type 4 channels:

drivers/mailbox/pcc.c:pcc_parse_subspace_db_reg() {
        ...
        ret = pcc_chan_reg_init(&pchan->error,
                                &pcct_ext->error_status_register,
        ...
}

If compliant firmware provides a zero-filled Generic Address Structure
(GAS) for this ignored field (evaluating to System Memory with space_id
== 0 and bit_width == 0), pcc_chan_reg_init() will fail its validation:

drivers/mailbox/pcc.c:pcc_chan_reg_init() {
        ...
        if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
                if (!(gas->bit_width >= 8 && gas->bit_width <= 64 &&
                      is_power_of_2(gas->bit_width))) {
                        pr_err("Error: Cannot access register of %u bit width",
                               gas->bit_width);
                        return -EFAULT;
                }
        ...
}

Will this abort the mailbox controller initialization and leave the
channel unusable?

>  	ret = pcc_chan_reg_read(&pchan->error, &val);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604163306.160017-1-admiyo@os.amperecomputing.com?part=1