* Re: [PATCH] PCI: rpaphp: Fix stack overflows when constructing DRC names
2026-09-09 17:33 [PATCH] PCI: rpaphp: Fix stack overflows when constructing DRC names Yudi Yang
@ 2026-09-09 17:46 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-09 17:46 UTC (permalink / raw)
To: Yudi Yang; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues:
- [High] Uninitialized stack buffer `cell_drc_name` is passed to `strcmp` if `my_index` is not found.
- [High] PCI devices added in `rpaphp_enable_slot` are not removed if `rpaphp_register_slot` fails.
- [High] `rpaphp_drc_info_add_slot` fails to iterate over all DRC entries and sequential elements, registering only the first slot.
- [High] Incorrect logical index calculation in `rpaphp_check_drc_props_v2` when `drc.sequential_inc` is greater than 1.
- [High] The return value of `of_read_drc_info_cell()` is ignored, which can lead to uninitialized stack memory being passed to subsequent functions.
- [High] Missing cleanup of dynamically added PCI Host Bridge (PHB) or PCI bus when `rpaphp_add_slot()` fails.
- [High] Uninitialized struct member `drc.drc_type` is dereferenced if the `ibm,drc-info` property contains 0 entries.
- [High] Out-of-bounds read of device tree properties in `rpaphp_check_drc_props_v1` when `my_index` is not found.
--
commit 47e69de3a2a757ee69366336ebc5deb176f42fd1
Author: Yudi Yang <2000jedi@gmail.com>
PCI: rpaphp: Fix stack overflows when constructing DRC names
The drc-name-prefix field of ibm,drc-info is supplied by firmware and
is not limited to fit within MAX_DRC_NAME_LEN. Using sprintf() to append
a suffix can therefore overflow the stack buffers used for DRC names.
Use snprintf() and reject names that would be truncated.
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index 2316de0fd1982..39109a50fee68 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -252,8 +252,11 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> /* Found it */
> if (my_index >= drc.drc_index_start && my_index <= drc.last_drc_index) {
> int index = my_index - drc.drc_index_start;
[Severity: High]
This is a pre-existing issue, but does this calculation correctly handle
drc.sequential_inc?
If sequential_inc is greater than 1, shouldn't index be divided by
sequential_inc to find the true index offset before adding it to
drc_name_suffix_start?
> - sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
> - drc.drc_name_suffix_start + index);
> +
> + if (snprintf(cell_drc_name, sizeof(cell_drc_name), "%s%u",
> + drc.drc_name_prefix,
> + drc.drc_name_suffix_start + index) >= sizeof(cell_drc_name))
> + return -EINVAL;
> break;
> }
> }
[Severity: High]
This isn't a bug introduced by this patch, but if my_index is not found,
the loop over entries completes without initializing cell_drc_name.
Doesn't this pass an uninitialized stack buffer to strcmp() right below?
drivers/pci/hotplug/rpaphp_core.c:rpaphp_check_drc_props_v2() {
...
if (((drc_name == NULL) ||
(drc_name && !strcmp(drc_name, cell_drc_name))) &&
...
}
[Severity: High]
Also a pre-existing issue, but if the ibm,drc-info property contains 0
entries, the loop over entries doesn't execute at all, leaving drc.drc_type
uninitialized.
Can this lead to a crash when strcmp(drc_type, drc.drc_type) is called at
the end of this function?
[ ... ]
> @@ -355,7 +358,9 @@ static int rpaphp_drc_info_add_slot(struct device_node *dn)
[Severity: High]
This is a pre-existing issue, but of_read_drc_info_cell() can fail and
return -EINVAL.
If it fails, drc.drc_type is uninitialized. Can this lead to a crash
when is_php_type(drc.drc_type) is called here?
> if (!is_php_type(drc.drc_type))
> return 0;
[Severity: High]
This is a pre-existing issue, but shouldn't this function iterate over all
the hotplug slots?
It reads 'count' but only registers the very first slot in the first cell,
completely ignoring 'count' and 'drc.num_sequential_elems'.
>
> - sprintf(drc_name, "%s%d", drc.drc_name_prefix, drc.drc_name_suffix_start);
> + if (snprintf(drc_name, sizeof(drc_name), "%s%u", drc.drc_name_prefix,
> + drc.drc_name_suffix_start) >= sizeof(drc_name))
> + return -EINVAL;
>
> slot = alloc_slot_struct(dn, drc.drc_index_start, drc_name, drc.drc_power_domain);
> if (!slot)
[Severity: High]
This is a pre-existing issue, but if rpaphp_register_slot() fails, the error
path calls dealloc_slot_struct() but doesn't undo the device additions made
by rpaphp_enable_slot().
Does this leak the PCI devices and leave the system in an inconsistent state?
drivers/pci/hotplug/rpaphp_core.c:rpaphp_drc_info_add_slot() {
...
retval = rpaphp_enable_slot(slot);
if (!retval)
retval = rpaphp_register_slot(slot);
if (retval)
dealloc_slot_struct(slot);
return retval;
}
The same resource leak appears to happen in the v1 loop implementation within
rpaphp_drc_add_slot().
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but out-of-bounds reads can occur
in rpaphp_check_drc_props_v1() when my_index is not found:
drivers/pci/hotplug/rpaphp_core.c:rpaphp_check_drc_props_v1() {
...
for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
if (be32_to_cpu(indexes[i + 1]) == my_index)
break;
name_tmp += (strlen(name_tmp) + 1);
type_tmp += (strlen(type_tmp) + 1);
}
if (((drc_name == NULL) || (drc_name && !strcmp(drc_name, name_tmp))) &&
((drc_type == NULL) || (drc_type && !strcmp(drc_type, type_tmp))))
return 0;
...
}
If the loop finishes without finding my_index, name_tmp and type_tmp advance
past the end of their property buffers. Doesn't this pass out-of-bounds
pointers to strcmp()?
[ ... ]
[Severity: High]
This is a pre-existing issue, but if rpaphp_add_slot() fails, does this
leak the newly added PHB because remove_phb_dynamic() is not called?
drivers/pci/hotplug/rpadlpar_core.c:dlpar_add_phb() {
...
phb = init_phb_dynamic(dn);
if (!phb)
return -EIO;
if (rpaphp_add_slot(dn)) {
printk(KERN_ERR "%s: unable to add hotplug slot %s\n",
__func__, drc_name);
return -EIO;
}
return 0;
}
Does the same leak happen in dlpar_add_pci_slot(), where the newly added
PCI bus is never removed if rpaphp_add_slot() fails?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909173300.1144301-1-2000jedi@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread