Re: [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()

[Alex Elder <elder@riscstar.com>]

On 9/4/26 8:59 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The local variable `bufp` is incorrectly declared as `char` instead of `char *`. This truncates the stack array pointer `buf` to an 8-bit integer, leading to severe memory corruption and an immediate kernel panic during runtime.
> - [Low] The local variable `i` is declared but never used after the patch.

I'm very sorry about this; I'm not sure how it even happened.
I must have included the wrong patch when I prepared the series
(but I normally check builds as part of that process).  This
should not have even compiled...

I will fix this and will send out a new version tonight or
tomorrow.

					-Alex

> --
> 
> commit 0153310b625600febac7ea5aec9fbd9871d0af03
> Author: Alex Elder <elder@riscstar.com>
> 
> PCI: of: avoid allocations in of_pci_prop_compatible()
> 
> This patch replaces dynamic allocations for three compatible strings with a
> single stack-allocated buffer. This avoids subsequent kernel crashes where
> allocation failures would unconditionally trigger strlen() during device tree
> properties initialization.
> 
> Link: https://lore.kernel.org/sashiko-reviews/a647bd56-7dc8-4fec-9d96-834622054cdf@riscstar.com
> 
>> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
>> --- a/drivers/pci/of_property.c
>> +++ b/drivers/pci/of_property.c
> [ ... ]
>>   static int of_pci_prop_compatible(struct pci_dev *pdev,
>>   				  struct of_changeset *ocs,
>>   				  struct device_node *np)
>>   {
>>   	const char *compat_strs[PROP_COMPAT_NUM] = { 0 };
>> +	char buf[PROP_COMPAT_NUM * PROP_SIZE] = { };
>> +	char bufp = buf;
> 
> [Severity: High]
> Does this incorrectly declare bufp as a single char instead of a pointer?

Yes this will be:

	char *bufp = buf;

> Assigning the array address buf to an 8-bit char truncates the memory address.
> 
>>   	int i, ret;

This will be:

	int ret;

> [Severity: Low]
> Is the variable i still needed here? The loop that used it to free the
> allocated strings was removed by this patch, which will leave this as an
> unused variable and cause a compiler warning.
> 
>> -	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
>> -		kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
>> -	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
>> -		kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
>> -	compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
>> -		kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);
>> +	ret = snprintf(bufp, PROP_SIZE, "pci%x,%x", pdev->vendor, pdev->device);
> 
> [Severity: High]
> Will this snprintf call trigger a kernel panic upon dereference? Because bufp
> is truncated to an 8-bit integer, passing it as the output buffer pointer to
> snprintf() will result in an out-of-bounds memory access (unmapped low-memory
> dereference) when evaluated during system boot or PCI device enumeration.

This will be fixed by using a proper type for bufp.

> 
> [ ... ]
>