Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
@@ -556,33 +556,24 @@ static const struct vdpa_config_ops
snet_config_ops = {
static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
*psnet)
{
char name[50];
- int ret, i, mask = 0;
+ int i;
+
+ snprintf(name, sizeof(name), "psnet[%s]-bars",
pci_name(pdev));
+
/* We don't know which BAR will be used to communicate..
* We will map every bar with len > 0.
*
* Later, we will discover the BAR and unmap all other
BARs.
*/
for (i = 0; i < PCI_STD_NUM_BARS; i++) {
- if (pci_resource_len(pdev, i))
- mask |= (1 << i);
- }
-
- /* No BAR can be used.. */
- if (!mask) {
- SNET_ERR(pdev, "Failed to find a PCI BAR\n");
- return -ENODEV;
- }
-
- snprintf(name, sizeof(name), "psnet[%s]-bars",
pci_name(pdev));
- ret = pcim_iomap_regions(pdev, mask, name);
- if (ret) {
- SNET_ERR(pdev, "Failed to request and map PCI
BARs\n");
- return ret;
- }
+ if (pci_resource_len(pdev, i)) {
+ psnet->bars[i] = pcim_iomap_region(pdev,
i, name);
Hi,
Unrelated to the patch, but is is safe to have 'name' be on the
stack?
pcim_iomap_region()
--> __pcim_request_region()
--> __pcim_request_region_range()
--> request_region() or __request_mem_region()
--> __request_region()
--> __request_region_locked()
--> res->name = name;
So an address on the stack ends in the 'name' field of a "struct
resource".
Oh oh...
According to a few grep, it looks really unusual.
I don't know if it is used, but it looks strange to me.
I have seen it used in the kernel ringbuffer log when you try to
request something that's already owned. I think it's here, right in
__request_region_locked():
/*
* mm/hmm.c reserves physical addresses which then
* become unavailable to other users. Conflicts are
* not expected. Warn to aid debugging if encountered.
*/
if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
pr_warn("Unaddressable device %s %pR conflicts with %pR",
conflict->name, conflict, res);
}
Assuming I interpret the code correctly:
The conflicting resource is found when a new caller (e.g. another
driver) tries to get the same region. So conflict->name on the original
requester's stack is by now gone and you do get UB.
Very unlikely UB, since only rarely drivers race for the same resource,
but still UB.
But there's also a few other places. Grep for "conflict->name".
If it is an issue, it was apparently already there before this patch.
I think this has to be fixed.
Question would just be whether one wants to fix it locally in this
driver, or prevent it from happening globally by making the common
infrastructure copy the string.
P.
Not a perfect script, but the below coccinelle script only find this
place, so I would +1 only fixing things here only.
Agree?
CJ
@@
identifier name;
expression x;
constant N;
@@
char name[N];
...
(
* pcim_iomap_region(..., name, ...);
|
* pcim_iomap_regions(..., name, ...);
|
* request_region(..., name, ...);
|
* x = pcim_iomap_region(..., name, ...);
|
* x = pcim_iomap_regions(..., name, ...);
|
* x = request_region(..., name, ...);
)
