Re: [PATCH v2] mailbox: pcc: validate PCCT subspaces before parsing

[Sudeep Holla <sudeep.holla@kernel.org>]

(For some reason you domain gets blocked for me, so please wait until
I check response manually on lore)

On Fri, Oct 23, 2026 at 05:38:39PM +0800, Jiale Yao wrote:
> The PCC mailbox probe walks PCCT entries by their declared lengths
> without checking that each entry is contained in the table. It also
> passes generic headers to type-specific parsers without establishing the
> required record size. Malformed firmware can therefore move the cursor
> outside the PCCT or cause out-of-bounds field reads.
> 
> Validate the table header, each subspace range, and the minimum size for
> supported subspace types during both the counting and probe passes.
> 
> Fixes: 86c22f8c9a3b ("Mailbox: Add support for Platform Communication Channel")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
> 
> Notes:
>     Changes in v2:
>     - Replace the type switch with a minimum-length lookup table.
>     - Remove the redundant remaining-header-size check.
>     - Keep an explicit type bound check before indexing the table.
> 
>  drivers/mailbox/pcc.c | 49 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 9888dab64639..91cef0ee3391 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -553,6 +553,38 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>  	.last_tx_done = pcc_last_tx_done,
>  };
>  
> +static const u16 pcc_subspace_min_len[ACPI_PCCT_TYPE_RESERVED] = {
> +	[ACPI_PCCT_TYPE_GENERIC_SUBSPACE] =
> +		sizeof(struct acpi_pcct_subspace),
> +	[ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE] =
> +		sizeof(struct acpi_pcct_hw_reduced),
> +	[ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2] =
> +		sizeof(struct acpi_pcct_hw_reduced_type2),
> +	[ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE] =
> +		sizeof(struct acpi_pcct_ext_pcc_master),
> +	[ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE] =
> +		sizeof(struct acpi_pcct_ext_pcc_slave),

Please add Type 5 details here as well so that it doesn't escape the
size validation. As is, its zero-initialized and lets every non 0-byte
record pass validation.

> +};
> +
> +static bool pcc_subspace_is_valid(struct acpi_subtable_header *entry,
> +				  unsigned long end)
> +{
> +	unsigned long start = (unsigned long)entry;
> +	size_t length;
> +
> +	if (start >= end)
> +		return false;
> +
> +	length = entry->length;
> +	if (length < sizeof(*entry) || length > end - start)
> +		return false;
> +
> +	if (entry->type >= ACPI_PCCT_TYPE_RESERVED)
> +		return false;
> +

You can drop the above check if parse_pcc_subspace() does this first.

> +	return length >= pcc_subspace_min_len[entry->type];
> +}
> +
>  /**
>   * parse_pcc_subspace - Count PCC subspaces defined
>   * @header: Pointer to the ACPI subtable header under the PCCT.
> @@ -568,6 +600,9 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header,
>  {
>  	struct acpi_pcct_subspace *ss = (struct acpi_pcct_subspace *) header;
>  
> +	if (!pcc_subspace_is_valid(&ss->header, end))
> +		return -EINVAL;
> +

So this will take care of validation as part of acpi_probe() itself which is
good but see my question below.

>  	if (ss->header.type < ACPI_PCCT_TYPE_RESERVED)
>  		return 0;
>  

Make this the first check(need to reverse it to return error now) and
drop the above check as mentioned.

> @@ -812,6 +847,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>  	struct acpi_table_header *pcct_tbl;
>  	struct acpi_subtable_header *pcct_entry;
>  	struct acpi_table_pcct *acpi_pcct_tbl;
> +	unsigned long table_end;
>  	acpi_status status = AE_OK;
>  	int i, rc, count = pcc_chan_count;
>  
> @@ -821,6 +857,13 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>  	if (ACPI_FAILURE(status) || !pcct_tbl)
>  		return -ENODEV;
>  
> +	acpi_pcct_tbl = (struct acpi_table_pcct *)pcct_tbl;
> +	if (pcct_tbl->length < sizeof(*acpi_pcct_tbl)) {
> +		rc = -EINVAL;
> +		goto err;
> +	}
> +	table_end = (unsigned long)pcct_tbl + pcct_tbl->length;
> +
>  	pcc_mbox_channels = devm_kcalloc(dev, count, sizeof(*pcc_mbox_channels),
>  					 GFP_KERNEL);
>  	if (!pcc_mbox_channels) {
> @@ -844,7 +887,6 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>  	pcct_entry = (struct acpi_subtable_header *) (
>  		(unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>  
> -	acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>  	if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL) {
>  		pcc_mbox_ctrl->txdone_irq = true;
>  		pcc_mbox_ctrl->txdone_poll = false;
> @@ -856,6 +898,11 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>  	for (i = 0; i < count; i++) {
>  		struct pcc_chan_info *pchan = chan_info + i;
>  
> +		if (!pcc_subspace_is_valid(pcct_entry, table_end)) {
> +			rc = -EINVAL;
> +			goto err;
> +		}
> +

Why do we need to re-validate the subspace if it is already taken care
before pcc_mbox_probe() as part of acpi_probe(). IMO the changes in
pcc_mbox_probe() can be dropped.

-- 
Regards,
Sudeep