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

[jiale yao]

Hi Jassi,
At 2026-10-22 10:26:27, "Jassi Brar" <jassisinghbrar@xxxxxxxxx> wrote:
>On Fri, Oct 16, 2026 at 9:15 AM Jiale Yao <yaojiale02@xxxxxxx> 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.
>>
>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>> ---
>>  drivers/mailbox/pcc.c | 53 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 9888dab64639..37b2ddb44bda 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -553,6 +553,42 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>>         .last_tx_done = pcc_last_tx_done,
>>  };
>>
>> +static bool pcc_subspace_is_valid(struct acpi_subtable_header *entry,
>> +                                 unsigned long end)
>> +{
>> +       unsigned long start = (unsigned long)entry;
>> +       size_t length, min_length;
>> +
>> +       if (start >= end || end - start < sizeof(*entry))
>> +               return false;
>
>(end - start < sizeof(*entry)) is implied by the two length  checks
>below, so only "start >= end" is needed here.
>
>> +       length = entry->length;
>> +       if (length < sizeof(*entry) || length > end - start)
>> +               return false;
>> +
>> +       switch (entry->type) {
>> +       case ACPI_PCCT_TYPE_GENERIC_SUBSPACE:
>> +               min_length = sizeof(struct acpi_pcct_subspace);
>> +               break;
>> +       case ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE:
>> +               min_length = sizeof(struct acpi_pcct_hw_reduced);
>> +               break;
>> +       case ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2:
>> +               min_length = sizeof(struct acpi_pcct_hw_reduced_type2);
>> +               break;
>> +       case ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE:
>> +               min_length = sizeof(struct acpi_pcct_ext_pcc_master);
>> +               break;
>> +       case ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE:
>> +               min_length = sizeof(struct acpi_pcct_ext_pcc_slave);
>> +               break;
>> +       default:
>> +               return false;
>> +       }
>> +
>> +       return length >= min_length;
>
>How about something like
>
>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),
>};

Sounds great, I will submit v2.

Thanks for your review.

>
>return entry->length >= pcc_subspace_min_len[entry->type];
>
>Cheers
>Jassi