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

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 [off-list ref]

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 (local)

quoted

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

[ ... ]

quoted

  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.

quoted

  	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.

quoted

-	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.

[ ... ]