In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
NULL which is later dereferenced. Fix this bug by adding NULL check.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Signed-off-by: Aleksandr Mishin <[email protected]>
---
drivers/pci/controller/dwc/pci-keystone.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 844de4418724..00d616654171 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -392,7 +392,11 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
struct resource *mem;
int i;
- mem = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM)->res;
+ struct resource_entry *ft = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
+ if (!ft)
+ return;
+
+ mem = ft->res;
start = mem->start;
end = mem->end;
--
2.30.2
On Fri, Mar 29, 2024 at 08:19:47AM +0300, Aleksandr Mishin wrote:
> In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> NULL which is later dereferenced. Fix this bug by adding NULL check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
> Signed-off-by: Aleksandr Mishin <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-keystone.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 844de4418724..00d616654171 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -392,7 +392,11 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> struct resource *mem;
> int i;
>
> - mem = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM)->res;
> + struct resource_entry *ft = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> + if (!ft)
> + return;
Like the other one
(https://lore.kernel.org/all/[email protected]/),
I think this potentially avoids a NULL pointer dereference (though I
didn't do the analysis in this case to see whether that's actually
possible), but fails to consider the implication of simply skipping
the rest of ks_pcie_setup_rc_app_regs().
"start" and "end" are used only for the loop about "Using Direct 1:1
mapping of RC <-> PCI memory space". If there's no IORESOURCE_MEM
resource, obviously that loop makes no sense. The function would
probably be improved by moving the resource_list_first_type() so it's
next to the loop that uses the result.
But the rest of the function doesn't depend on that IORESOURCE_MEM
resource, and it's not at all clear that the rest of the function
should be skipped.
> + mem = ft->res;
> start = mem->start;
> end = mem->end;
>
> --
> 2.30.2
>
In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
NULL which is later dereferenced. Fix this bug by adding NULL check.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Signed-off-by: Aleksandr Mishin <[email protected]>
---
v2: Add return code processing
drivers/pci/controller/dwc/pci-keystone.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 844de4418724..5c6786d9f3e9 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -382,17 +382,22 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
} while (val & DBI_CS2);
}
-static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
+static int ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
{
u32 val;
u32 num_viewport = ks_pcie->num_viewport;
struct dw_pcie *pci = ks_pcie->pci;
struct dw_pcie_rp *pp = &pci->pp;
- u64 start, end;
+ struct resource_entry *ft;
struct resource *mem;
+ u64 start, end;
int i;
- mem = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM)->res;
+ ft = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
+ if (!ft)
+ return -EINVAL;
+
+ mem = ft->res;
start = mem->start;
end = mem->end;
@@ -403,7 +408,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
ks_pcie_clear_dbi_mode(ks_pcie);
if (ks_pcie->is_am6)
- return;
+ return 0;
val = ilog2(OB_WIN_SIZE);
ks_pcie_app_writel(ks_pcie, OB_SIZE, val);
@@ -420,6 +425,8 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
val |= OB_XLAT_EN_VAL;
ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
+
+ return 0;
}
static void __iomem *ks_pcie_other_map_bus(struct pci_bus *bus,
@@ -814,7 +821,10 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
return ret;
ks_pcie_stop_link(pci);
- ks_pcie_setup_rc_app_regs(ks_pcie);
+ ret = ks_pcie_setup_rc_app_regs(ks_pcie);
+ if (ret < 0)
+ return ret;
+
writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
pci->dbi_base + PCI_IO_BASE);
--
2.30.2
From: Aleksandr Mishin <[email protected]>
Date: Thu, 25 Apr 2024 12:21:35 +0300
> In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> NULL which is later dereferenced. Fix this bug by adding NULL check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
Please stop spamming with "potential fixes" made mechanically from
static analyzer reports without looking into the code flow. These
patches are mostly incorrect and may hurt.
Either have a stable repro and then fix the real bug or don't touch
anything at all.
>
> Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
> Signed-off-by: Aleksandr Mishin <[email protected]>
Thanks,
Olek
On Thu, Apr 25, 2024 at 03:00:14PM +0200, Alexander Lobakin wrote:
> From: Aleksandr Mishin <[email protected]>
> Date: Thu, 25 Apr 2024 12:21:35 +0300
>
> > In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> > NULL which is later dereferenced. Fix this bug by adding NULL check.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Please stop spamming with "potential fixes" made mechanically from
> static analyzer reports without looking into the code flow. These
> patches are mostly incorrect and may hurt.
> Either have a stable repro and then fix the real bug or don't touch
> anything at all.
Did you look at the actual patch? I'm not a keystone expert, but this
patch looks reasonable to me.
It might still be the case that we're guaranteed to have an
IORESOURCE_MEM window by other code, but it looks like a real hassle
to prove that.
> > Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
> > Signed-off-by: Aleksandr Mishin <[email protected]>
>
> Thanks,
> Olek
On Thu, Apr 25, 2024 at 03:00:14PM +0200, Alexander Lobakin wrote:
> From: Aleksandr Mishin <[email protected]>
> Date: Thu, 25 Apr 2024 12:21:35 +0300
>
> > In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> > NULL which is later dereferenced. Fix this bug by adding NULL check.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Please stop spamming with "potential fixes" made mechanically from
> static analyzer reports without looking into the code flow. These
> patches are mostly incorrect and may hurt.
> Either have a stable repro and then fix the real bug or don't touch
> anything at all.
>
This patch obviously fixes the potential issue where resource_list_first_type()
may return NULL if the MEM range is not provided in DT.
pci_parse_request_of_pci_ranges() will just emit a warning in that case and this
code path will cause a NULL pointer dereference.
Even though this situation means that the DT is broken, it still makes sense to
have the checks in place.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Thu, Apr 25, 2024 at 12:21:35PM +0300, Aleksandr Mishin wrote:
> In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> NULL which is later dereferenced. Fix this bug by adding NULL check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
> Signed-off-by: Aleksandr Mishin <[email protected]>
> ---
> v2: Add return code processing
>
> drivers/pci/controller/dwc/pci-keystone.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 844de4418724..5c6786d9f3e9 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -382,17 +382,22 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
> } while (val & DBI_CS2);
> }
>
> -static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> +static int ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> {
> u32 val;
> u32 num_viewport = ks_pcie->num_viewport;
> struct dw_pcie *pci = ks_pcie->pci;
> struct dw_pcie_rp *pp = &pci->pp;
> - u64 start, end;
> + struct resource_entry *ft;
> struct resource *mem;
> + u64 start, end;
> int i;
>
> - mem = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM)->res;
> + ft = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> + if (!ft)
> + return -EINVAL;
-ENODEV please.
- Mani
> +
> + mem = ft->res;
> start = mem->start;
> end = mem->end;
>
> @@ -403,7 +408,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> ks_pcie_clear_dbi_mode(ks_pcie);
>
> if (ks_pcie->is_am6)
> - return;
> + return 0;
>
> val = ilog2(OB_WIN_SIZE);
> ks_pcie_app_writel(ks_pcie, OB_SIZE, val);
> @@ -420,6 +425,8 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
> val |= OB_XLAT_EN_VAL;
> ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
> +
> + return 0;
> }
>
> static void __iomem *ks_pcie_other_map_bus(struct pci_bus *bus,
> @@ -814,7 +821,10 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> return ret;
>
> ks_pcie_stop_link(pci);
> - ks_pcie_setup_rc_app_regs(ks_pcie);
> + ret = ks_pcie_setup_rc_app_regs(ks_pcie);
> + if (ret < 0)
> + return ret;
> +
> writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
> pci->dbi_base + PCI_IO_BASE);
>
> --
> 2.30.2
>
--
மணிவண்ணன் சதாசிவம்
On Thu, Apr 25, 2024 at 12:21:35PM +0300, Aleksandr Mishin wrote:
> In ks_pcie_setup_rc_app_regs() resource_list_first_type() may return
> NULL which is later dereferenced. Fix this bug by adding NULL check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
> Signed-off-by: Aleksandr Mishin <[email protected]>
> ---
> v2: Add return code processing
>
> drivers/pci/controller/dwc/pci-keystone.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 844de4418724..5c6786d9f3e9 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -382,17 +382,22 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
> } while (val & DBI_CS2);
> }
>
> -static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> +static int ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> {
> u32 val;
> u32 num_viewport = ks_pcie->num_viewport;
> struct dw_pcie *pci = ks_pcie->pci;
> struct dw_pcie_rp *pp = &pci->pp;
> - u64 start, end;
> + struct resource_entry *ft;
> struct resource *mem;
> + u64 start, end;
> int i;
>
> - mem = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM)->res;
> + ft = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> + if (!ft)
> + return -EINVAL;
> +
> + mem = ft->res;
> start = mem->start;
> end = mem->end;
>
> @@ -403,7 +408,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> ks_pcie_clear_dbi_mode(ks_pcie);
>
> if (ks_pcie->is_am6)
> - return;
> + return 0;
>
> val = ilog2(OB_WIN_SIZE);
> ks_pcie_app_writel(ks_pcie, OB_SIZE, val);
> @@ -420,6 +425,8 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
> val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
> val |= OB_XLAT_EN_VAL;
> ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
> +
> + return 0;
> }
>
> static void __iomem *ks_pcie_other_map_bus(struct pci_bus *bus,
> @@ -814,7 +821,10 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> return ret;
>
> ks_pcie_stop_link(pci);
> - ks_pcie_setup_rc_app_regs(ks_pcie);
> + ret = ks_pcie_setup_rc_app_regs(ks_pcie);
Since ks_pcie_setup_rc_app_regs() returns 0 on success,
I suggest you do:
if (ret)
return ret;
Instead.
> + if (ret < 0)
> + return ret;
> +
Kind regards,
Niklas