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

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

[Jiale Yao]

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),
+};
+
+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;
+
+	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;
+
 	if (ss->header.type < ACPI_PCCT_TYPE_RESERVED)
 		return 0;
 
@@ -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;
+		}
+
 		pcc_mbox_channels[i].con_priv = pchan;
 		pchan->chan.mchan = &pcc_mbox_channels[i];
 
-- 
2.34.1

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

[Sudeep Holla]

(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

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

[jiale yao]

At 2026-10-23 22:24:28, "Sudeep Holla" <sudeep.holla@kernel.org> 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@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.

Thanks for your review, the update one is here,

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

>
>-- 
>Regards,
>Sudeep