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

[jiale yao]

At 2026-10-22 10:42:19, "jiale yao" <19888972804@xxxxxxx> wrote:
>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.

v2 is in, https://lore.kernel.org/all/20261023093839.715037-1-yaojiale02@xxxxxxx/

>
>Thanks for your review.
>
>>
>>return entry->length >= pcc_subspace_min_len[entry->type];
>>
>>Cheers
>>Jassi