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

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

[Adam Young]

THE ACPI spec states:

"[The Error status register] Contains the processor relative address,
represented in Generic Address Structure (GAS) format, of the Error status
register. This field is ignored by the OSPM on slave channels"

Which Refers to type 4 channels.

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

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/mailbox/pcc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 636879ae1db7..0deaf7907ed6 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;
+
 	ret = pcc_chan_reg_read(&pchan->error, &val);
 	if (ret)
 		return ret;
-- 
2.43.0

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

[sashiko-bot]

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

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

[Adam Young]

On 6/4/26 12:52, sashiko-bot@kernel.org wrote:
> 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;


While these all look plausable, none of them are caused by or fixed by 
this patch.