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

[jiale yao]

At 2026-10-23 22:24:28, "Sudeep Holla" <sudeep.holla@xxxxxxxxxx> wrote:
>(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@xxxxxxxxxxxxxxx
>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>> ---
>> 
>> 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.

Thanks for your review, the update one is here,

https://lore.kernel.org/all/20261027064835.2264459-1-yaojiale02@xxxxxxx

>
>-- 
>Regards,
>Sudeep