| Message ID | 20240425092135.13348-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v2] PCI: dwc: keystone: Fix potential NULL dereference | expand |
From: Aleksandr Mishin <amishin@t-argos.ru> 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 <amishin@t-argos.ru> Thanks, Olek
On Thu, Apr 25, 2024 at 03:00:14PM +0200, Alexander Lobakin wrote: > From: Aleksandr Mishin <amishin@t-argos.ru> > 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 <amishin@t-argos.ru> > > Thanks, > Olek
On Thu, Apr 25, 2024 at 03:00:14PM +0200, Alexander Lobakin wrote: > From: Aleksandr Mishin <amishin@t-argos.ru> > 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 <amishin@t-argos.ru> > --- > 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 <amishin@t-argos.ru> > --- > 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
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);
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 <amishin@t-argos.ru> --- v2: Add return code processing drivers/pci/controller/dwc/pci-keystone.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)