Re: New Defects reported by Coverity Scan for Linux

Re: New Defects reported by Coverity Scan for Linux

[Bjorn Helgaas]

FYI

On Mon, Feb 08, 2021 at 04:46:46AM +0000, scan-admin@coverity.com wrote:
> 3 new defect(s) introduced to Linux found with Coverity Scan.
> ...

> *** CID 1472841:  Error handling issues  (CHECKED_RETURN)
> /drivers/pci/controller/dwc/pci-exynos.c: 263 in exynos_pcie_host_init()
> 257     
> 258     	pp->bridge->ops = &exynos_pci_ops;
> 259     
> 260     	exynos_pcie_assert_core_reset(ep);
> 261     
> 262     	phy_reset(ep->phy);
> >>>     CID 1472841:  Error handling issues  (CHECKED_RETURN)
> >>>     Calling "phy_power_on" without checking return value (as is done elsewhere 40 out of 50 times).
> 263     	phy_power_on(ep->phy);
> 264     	phy_init(ep->phy);
> 265     
> 266     	exynos_pcie_deassert_core_reset(ep);
> 267     	exynos_pcie_enable_irq_pulse(ep);
> 268     

Re: New Defects reported by Coverity Scan for Linux

[Krzysztof Wilczyński]

[+cc Fabio]

Hi Bjorn, Lorenzo and Rob,

[...]
> > *** CID 1472841:  Error handling issues  (CHECKED_RETURN)
> > /drivers/pci/controller/dwc/pci-exynos.c: 263 in exynos_pcie_host_init()
> > 257     
> > 258     	pp->bridge->ops = &exynos_pci_ops;
> > 259     
> > 260     	exynos_pcie_assert_core_reset(ep);
> > 261     
> > 262     	phy_reset(ep->phy);
> > >>>     CID 1472841:  Error handling issues  (CHECKED_RETURN)
> > >>>     Calling "phy_power_on" without checking return value (as is done elsewhere 40 out of 50 times).
> > 263     	phy_power_on(ep->phy);
> > 264     	phy_init(ep->phy);
> > 265     
> > 266     	exynos_pcie_deassert_core_reset(ep);
> > 267     	exynos_pcie_enable_irq_pulse(ep);
> > 268     

We also have the following defect detected in the same file, and it's of
an identical nature - lack of error checking.  The reported defect:

263        phy_power_on(ep->phy);
CID 1471267 (#1 of 1): Unchecked return value (CHECKED_RETURN)
2. check_return: Calling phy_init without checking return value (as is done elsewhere 41 out of 49 times).
264        phy_init(ep->phy);

This would also be quite trivial to fix, but I don't know much about
Exons, thus I am not sure if there is anything special it would need
aside of perhaps phy_power_off() and phy_exit(), etc.

Krzysztof

Re: New Defects reported by Coverity Scan for Linux

[Bjorn Helgaas]

FYI.  Looks like we rely directy on the result of a read from the
device to index an array, probably not a great idea.

On Mon, Jun 21, 2021 at 07:45:30AM +0000, scan-admin@coverity.com wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.
> 
> 7 new defect(s) introduced to Linux found with Coverity Scan.
> 4 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.


> ** CID 1475616:  Memory - illegal accesses  (OVERRUN)
> /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1475616:  Memory - illegal accesses  (OVERRUN)
> /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
> 988     		retry = false;
> 989     		goto retry_link;
> 990     	}
> 991     
> 992     	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
> 993     		PCI_EXP_LNKSTA_CLS;
> >>>     CID 1475616:  Memory - illegal accesses  (OVERRUN)
> >>>     Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
> 994     	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> 995     
> 996     	tegra_pcie_enable_interrupts(pp);
> 997     
> 998     	return 0;
> 999     }
> 
> ** CID 1475402:  Memory - illegal accesses  (OVERRUN)
> /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1475402:  Memory - illegal accesses  (OVERRUN)
> /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
> 451     	struct tegra_pcie_dw *pcie = arg;
> 452     	struct dw_pcie *pci = &pcie->pci;
> 453     	u32 val, speed;
> 454     
> 455     	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
> 456     		PCI_EXP_LNKSTA_CLS;
> >>>     CID 1475402:  Memory - illegal accesses  (OVERRUN)
> >>>     Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
> 457     	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> 458     
> 459     	/* If EP doesn't advertise L1SS, just return */
> 460     	val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
> 461     	if (!(val & (PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2)))
> 462     		return IRQ_HANDLED;

Re: New Defects reported by Coverity Scan for Linux

[Bjorn Helgaas]

[+cc Om, just noticed your series of pcie-tegra194 updates]

On Mon, Jun 21, 2021 at 07:44:26AM -0500, Bjorn Helgaas wrote:
> FYI.  Looks like we rely directy on the result of a read from the
> device to index an array, probably not a great idea.
> 
> On Mon, Jun 21, 2021 at 07:45:30AM +0000, scan-admin@coverity.com wrote:
> > Hi,
> > 
> > Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.
> > 
> > 7 new defect(s) introduced to Linux found with Coverity Scan.
> > 4 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> 
> > ** CID 1475616:  Memory - illegal accesses  (OVERRUN)
> > /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 1475616:  Memory - illegal accesses  (OVERRUN)
> > /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
> > 988     		retry = false;
> > 989     		goto retry_link;
> > 990     	}
> > 991     
> > 992     	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
> > 993     		PCI_EXP_LNKSTA_CLS;
> > >>>     CID 1475616:  Memory - illegal accesses  (OVERRUN)
> > >>>     Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
> > 994     	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> > 995     
> > 996     	tegra_pcie_enable_interrupts(pp);
> > 997     
> > 998     	return 0;
> > 999     }
> > 
> > ** CID 1475402:  Memory - illegal accesses  (OVERRUN)
> > /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 1475402:  Memory - illegal accesses  (OVERRUN)
> > /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
> > 451     	struct tegra_pcie_dw *pcie = arg;
> > 452     	struct dw_pcie *pci = &pcie->pci;
> > 453     	u32 val, speed;
> > 454     
> > 455     	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
> > 456     		PCI_EXP_LNKSTA_CLS;
> > >>>     CID 1475402:  Memory - illegal accesses  (OVERRUN)
> > >>>     Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
> > 457     	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> > 458     
> > 459     	/* If EP doesn't advertise L1SS, just return */
> > 460     	val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
> > 461     	if (!(val & (PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2)))
> > 462     		return IRQ_HANDLED;

Re: New Defects reported by Coverity Scan for Linux

[Om Prakash Singh]

Thanks Bjorn for sharing the result.

We will work on the issue

Thanks,
Om


On 6/21/2021 6:35 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> [+cc Om, just noticed your series of pcie-tegra194 updates]
> 
> On Mon, Jun 21, 2021 at 07:44:26AM -0500, Bjorn Helgaas wrote:
>> FYI.  Looks like we rely directy on the result of a read from the
>> device to index an array, probably not a great idea.
>>
>> On Mon, Jun 21, 2021 at 07:45:30AM +0000, scan-admin@coverity.com wrote:
>>> Hi,
>>>
>>> Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.
>>>
>>> 7 new defect(s) introduced to Linux found with Coverity Scan.
>>> 4 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
>>
>>
>>> ** CID 1475616:  Memory - illegal accesses  (OVERRUN)
>>> /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
>>>
>>>
>>> ________________________________________________________________________________________________________
>>> *** CID 1475616:  Memory - illegal accesses  (OVERRUN)
>>> /drivers/pci/controller/dwc/pcie-tegra194.c: 994 in tegra_pcie_dw_start_link()
>>> 988                 retry = false;
>>> 989                 goto retry_link;
>>> 990         }
>>> 991
>>> 992         speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
>>> 993                 PCI_EXP_LNKSTA_CLS;
>>>>>>      CID 1475616:  Memory - illegal accesses  (OVERRUN)
>>>>>>      Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
>>> 994         clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>> 995
>>> 996         tegra_pcie_enable_interrupts(pp);
>>> 997
>>> 998         return 0;
>>> 999     }
>>>
>>> ** CID 1475402:  Memory - illegal accesses  (OVERRUN)
>>> /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
>>>
>>>
>>> ________________________________________________________________________________________________________
>>> *** CID 1475402:  Memory - illegal accesses  (OVERRUN)
>>> /drivers/pci/controller/dwc/pcie-tegra194.c: 457 in tegra_pcie_ep_irq_thread()
>>> 451         struct tegra_pcie_dw *pcie = arg;
>>> 452         struct dw_pcie *pci = &pcie->pci;
>>> 453         u32 val, speed;
>>> 454
>>> 455         speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
>>> 456                 PCI_EXP_LNKSTA_CLS;
>>>>>>      CID 1475402:  Memory - illegal accesses  (OVERRUN)
>>>>>>      Overrunning array "pcie_gen_freq" of 4 4-byte elements at element index 4294967295 (byte offset 17179869183) using index "speed - 1U" (which evaluates to 4294967295).
>>> 457         clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>> 458
>>> 459         /* If EP doesn't advertise L1SS, just return */
>>> 460         val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
>>> 461         if (!(val & (PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2)))
>>> 462                 return IRQ_HANDLED;

New Defects reported by Coverity Scan for Linux

[Bjorn Helgaas]

New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
("PCI: dwc: Restore ATU memory resource setup to use last entry").

I didn't try to figure out if this is real or a false positive, so
just FYI.

----- Forwarded message from scan-admin@coverity.com -----

Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
From: scan-admin@coverity.com
To: bjorn@helgaas.com
Subject: New Defects reported by Coverity Scan for Linux
Message-ID: <5fa924618fb3b_a62932acac7322f5033088@prd-scan-dashboard-0.mail>


** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
/drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()


________________________________________________________________________________________________________
*** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
/drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
590     
591     		/* Get last memory resource entry */
592     		resource_list_for_each_entry(tmp, &pp->bridge->windows)
593     			if (resource_type(tmp->res) == IORESOURCE_MEM)
594     				entry = tmp;
595     
>>>     CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "entry".
596     		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
597     					  PCIE_ATU_TYPE_MEM, entry->res->start,
598     					  entry->res->start - entry->offset,
599     					  resource_size(entry->res));
600     		if (pci->num_viewport > 2)
601     			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,

RE: New Defects reported by Coverity Scan for Linux

[Gustavo Pimentel]

On Tue, Nov 10, 2020 at 17:16:41, Bjorn Helgaas <helgaas@kernel.org> 
wrote:

> New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
> ("PCI: dwc: Restore ATU memory resource setup to use last entry").
> 
> I didn't try to figure out if this is real or a false positive, so
> just FYI.
> 
> ----- Forwarded message from scan-admin@coverity.com -----
> 
> Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
> From: scan-admin@coverity.com
> To: bjorn@helgaas.com
> Subject: New Defects reported by Coverity Scan for Linux
> Message-ID: <5fa924618fb3b_a62932acac7322f5033088@prd-scan-dashboard-0.mail>
> 
> 
> ** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> 590     
> 591     		/* Get last memory resource entry */
> 592     		resource_list_for_each_entry(tmp, &pp->bridge->windows)
> 593     			if (resource_type(tmp->res) == IORESOURCE_MEM)

Can the pp->bridge->windows list be empty in a typical use case?

> 594     				entry = tmp;
> 595     
> >>>     CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Dereferencing null pointer "entry".
> 596     		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
> 597     					  PCIE_ATU_TYPE_MEM, entry->res->start,
> 598     					  entry->res->start - entry->offset,
> 599     					  resource_size(entry->res));
> 600     		if (pci->num_viewport > 2)
> 601     			dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,

Re: New Defects reported by Coverity Scan for Linux

[Rob Herring]

On Tue, Nov 10, 2020 at 5:36 PM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> On Tue, Nov 10, 2020 at 17:16:41, Bjorn Helgaas <helgaas@kernel.org>
> wrote:
>
> > New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
> > ("PCI: dwc: Restore ATU memory resource setup to use last entry").
> >
> > I didn't try to figure out if this is real or a false positive, so
> > just FYI.
> >
> > ----- Forwarded message from scan-admin@coverity.com -----
> >
> > Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
> > From: scan-admin@coverity.com
> > To: bjorn@helgaas.com
> > Subject: New Defects reported by Coverity Scan for Linux
> > Message-ID: <5fa924618fb3b_a62932acac7322f5033088@prd-scan-dashboard-0.mail>
> >
> >
> > ** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> >
> >
> > ________________________________________________________________________________________________________
> > *** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > 590
> > 591                   /* Get last memory resource entry */
> > 592                   resource_list_for_each_entry(tmp, &pp->bridge->windows)
> > 593                           if (resource_type(tmp->res) == IORESOURCE_MEM)
>
> Can the pp->bridge->windows list be empty in a typical use case?

Only if the DT has missing/malformed 'ranges'. 'ranges' is required to
have any memory or i/o space, so we would error out before this point.

Rob

Re: New Defects reported by Coverity Scan for Linux

[Bjorn Helgaas]

On Wed, Nov 11, 2020 at 09:34:10AM -0600, Rob Herring wrote:
> On Tue, Nov 10, 2020 at 5:36 PM Gustavo Pimentel
> <Gustavo.Pimentel@synopsys.com> wrote:
> > On Tue, Nov 10, 2020 at 17:16:41, Bjorn Helgaas <helgaas@kernel.org>
> > wrote:
> >
> > > New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
> > > ("PCI: dwc: Restore ATU memory resource setup to use last entry").
> > >
> > > I didn't try to figure out if this is real or a false positive, so
> > > just FYI.
> > >
> > > ----- Forwarded message from scan-admin@coverity.com -----
> > >
> > > Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
> > > From: scan-admin@coverity.com
> > > To: bjorn@helgaas.com
> > > Subject: New Defects reported by Coverity Scan for Linux
> > > Message-ID: <5fa924618fb3b_a62932acac7322f5033088@prd-scan-dashboard-0.mail>
> > >
> > > ** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > >
> > > ________________________________________________________________________________________________________
> > > *** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > > 590
> > > 591                   /* Get last memory resource entry */
> > > 592                   resource_list_for_each_entry(tmp, &pp->bridge->windows)
> > > 593                           if (resource_type(tmp->res) == IORESOURCE_MEM)
> >
> > Can the pp->bridge->windows list be empty in a typical use case?
> 
> Only if the DT has missing/malformed 'ranges'. 'ranges' is required to
> have any memory or i/o space, so we would error out before this point.

There are a lot of paths that lead here, and it's an awful lot of work
to verify that they all correctly error out if 'ranges' is invalid.

It would really be nice if we could structure this in such a way that
local analysis could show that we never dereference a null pointer
here.

I wouldn't want to uglify the code unnecessarily, but if a small code
change could avoid this false positive, I think it might be worth
doing.

Bjorn

Re: New Defects reported by Coverity Scan for Linux

[Rob Herring]

On Wed, Nov 11, 2020 at 3:06 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Nov 11, 2020 at 09:34:10AM -0600, Rob Herring wrote:
> > On Tue, Nov 10, 2020 at 5:36 PM Gustavo Pimentel
> > <Gustavo.Pimentel@synopsys.com> wrote:
> > > On Tue, Nov 10, 2020 at 17:16:41, Bjorn Helgaas <helgaas@kernel.org>
> > > wrote:
> > >
> > > > New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
> > > > ("PCI: dwc: Restore ATU memory resource setup to use last entry").
> > > >
> > > > I didn't try to figure out if this is real or a false positive, so
> > > > just FYI.
> > > >
> > > > ----- Forwarded message from scan-admin@coverity.com -----
> > > >
> > > > Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
> > > > From: scan-admin@coverity.com
> > > > To: bjorn@helgaas.com
> > > > Subject: New Defects reported by Coverity Scan for Linux
> > > > Message-ID: <5fa924618fb3b_a62932acac7322f5033088@prd-scan-dashboard-0.mail>
> > > >
> > > > ** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > > >
> > > > ________________________________________________________________________________________________________
> > > > *** CID 1469110:  Null pointer dereferences  (FORWARD_NULL)
> > > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > > > 590
> > > > 591                   /* Get last memory resource entry */
> > > > 592                   resource_list_for_each_entry(tmp, &pp->bridge->windows)
> > > > 593                           if (resource_type(tmp->res) == IORESOURCE_MEM)
> > >
> > > Can the pp->bridge->windows list be empty in a typical use case?
> >
> > Only if the DT has missing/malformed 'ranges'. 'ranges' is required to
> > have any memory or i/o space, so we would error out before this point.
>
> There are a lot of paths that lead here, and it's an awful lot of work
> to verify that they all correctly error out if 'ranges' is invalid.

There used to be, but I've gotten rid of most. There's also only one
caller of dw_pcie_setup_rc() in the probe path with my latest series
of DWC cleanups that's not applied yet. The only other callers are
from a couple of resume hooks (which I plan to define common functions
for).

The pci_host_bridge allocation/init fails if bridge->windows is not
populated. But actually, 'windows' can never be NULL as it is a
list_head. So it's pp or pp->bridge being NULL that's the complaint,
but that doesn't really change things. The flow is like this:

dw_pcie_host_init()
    -> devm_pci_alloc_host_bridge()
    pp->bridge = bridge;
    ...
    -> dw_pcie_setup_rc()

> It would really be nice if we could structure this in such a way that
> local analysis could show that we never dereference a null pointer
> here.

I've considered making struct pcie_port and struct pci_host_bridge a
single allocation. I'm not sure that's really worth doing though.

> I wouldn't want to uglify the code unnecessarily, but if a small code
> change could avoid this false positive, I think it might be worth
> doing.

Other than also passing in bridge ptr, not sure. Maybe if it is static
which will happen with a common resume routine. That would just shift
the problem though it would be a bit clearer that we really couldn't
ever get to a resume callback with a NULL.

Rob

Fwd: New Defects reported by Coverity Scan for Linux

[Richard Weinberger]

Andrea,

please check. The same pattern seems to be used more than once in this driver.

Thanks,
//richard

----------  Weitergeleitete Nachricht  ----------

Betreff: New Defects reported by Coverity Scan for Linux
Datum: Montag, 27. November 2017, 08:49:21 CET
Von: scan-admin@coverity.com
An: richard@nod.at



Hi,

Please find the latest report on new defect(s) introduced to Linux, under 
component 'Drivers-MTD',  found with Coverity Scan.

1 new defect(s) introduced to Linux, under component 'Drivers-MTD',  found 
with Coverity Scan.
344 defect(s), reported by Coverity Scan earlier, were marked fixed in the 
recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/drivers/mtd/parsers/sharpslpart.c: 195 in sharpsl_nand_init_ftl()


________________________________________________________________________________________________________
*** CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/drivers/mtd/parsers/sharpslpart.c: 195 in sharpsl_nand_init_ftl()
189     	/* initialize ftl->log2phy */
190     	for (i = 0; i < ftl->logmax; i++)
191     		ftl->log2phy[i] = UINT_MAX;
192     
193     	/* create physical-logical table */
194     	for (block_num = 0; block_num < phymax; block_num++) {
>>>     CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>     Potentially overflowing expression "block_num * mtd->erasesize" with 
type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, 
and then used in a context that expects an expression of type "loff_t" (64 
bits, signed).
195     		block_adr = block_num * mtd->erasesize;
196     
197     		if (mtd_block_isbad(mtd, block_adr))
198     			continue;
199     
200     		if (sharpsl_nand_read_oob(mtd, block_adr, oob))


________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/
wf/click?
upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRZILrCyVW9WQLwr7wR5iGubla-2BOU6Se2Euumsa1bPpOJg-3D-3D_p9c2Pq5BRWXelYclnUuZY8l3SYc-2FPJtx2STX-2BpF5A6td-2FPvSZFHfZnIH7Pkuotsj40dgSYlEHnY3fa8hwUfXcZx2zIMi9ygf9fvrncbz9LZ035WDZixEDGKJm-2BkA-2FlNNw4vjG8qChO-2Fsn3KGwsVc2B46OlL-2BawwYWOieMWWWlm44I8px-2BoB-2BuIPoEApB2C1WKE6DZ0qI9OL3nk2Lf6cdMA-3D-3D

To manage Coverity Scan email notifications for "richard@nod.at", click 
https://u2389337.ct.sendgrid.net/wf/click?
upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4PW1q5HE7hT0R7FQopI50uMi-2FXxBV9hqQbIHwUk8i5vtfR-2BsAt7Vmc5VhhK-2BFpW3LXtshV-2BNRMG6fEVAWxE0JubBijVgPqNmrGAZeWjyxZKc-3D_p9c2Pq5BRWXelYclnUuZY8l3SYc-2FPJtx2STX-2BpF5A6td-2FPvSZFHfZnIH7PkuotsjtjhWtoA3IVRL5bV1R9qiwG4cqA-2FKu4HHy-2FY11gvyLcPSbxHkMq75abGzuiZVVK-2BoMEddCyVJkKCXyuwR33GIGAu-2F-2BxMohkiXZLfEkTEF4Cq4rSxCKccF8nqIWYFGjwP7kzyOHAH5cWC2z4YWNyegGg-3D-3D


-------------------------------------------------------------

Re: New Defects reported by Coverity Scan for Linux

[Andrea Adami]

On Mon, Nov 27, 2017 at 2:22 PM, Richard Weinberger <richard@nod.at> wrote:
> Andrea,
>
> please check. The same pattern seems to be used more than once in this driver.
>
> Thanks,
> //richard

Hello,
I acked the patch v2, considering 2 occurrences:

https://lkml.org/lkml/2017/11/8/567

I thought that was enough, what should I do now?
Thanks
Andrea


P.S.
Neverthless, imho it is impposible to overflow with the numbers we are using:

+ for (block_num = 0; block_num < phymax; block_num++) {
+ block_adr = block_num * mtd->erasesize;

#define SHARPSL_FTL_PART_SIZE (7 * SZ_1M)

for pxa25x : nand: 128 MiB, SLC, erase size: 16 KiB, page size: 512,
OOB size: 16
FTL blocks: 448 physical = 24 reserved + 424 logical
max block_adr = 447 * 16384

for pxa27x
nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
FTL blocks: 56 physical = 4 reserved + 52 logical
max block_adr = 55 * 131072


>
> ----------  Weitergeleitete Nachricht  ----------
>
> Betreff: New Defects reported by Coverity Scan for Linux
> Datum: Montag, 27. November 2017, 08:49:21 CET
> Von: scan-admin@coverity.com
> An: richard@nod.at
>
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Linux, under
> component 'Drivers-MTD',  found with Coverity Scan.
>
> 1 new defect(s) introduced to Linux, under component 'Drivers-MTD',  found
> with Coverity Scan.
> 344 defect(s), reported by Coverity Scan earlier, were marked fixed in the
> recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
>
>
> ** CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /drivers/mtd/parsers/sharpslpart.c: 195 in sharpsl_nand_init_ftl()
>
>
> ________________________________________________________________________________________________________
> *** CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /drivers/mtd/parsers/sharpslpart.c: 195 in sharpsl_nand_init_ftl()
> 189             /* initialize ftl->log2phy */
> 190             for (i = 0; i < ftl->logmax; i++)
> 191                     ftl->log2phy[i] = UINT_MAX;
> 192
> 193             /* create physical-logical table */
> 194             for (block_num = 0; block_num < phymax; block_num++) {
>>>>     CID 1424016:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>>     Potentially overflowing expression "block_num * mtd->erasesize" with
> type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic,
> and then used in a context that expects an expression of type "loff_t" (64
> bits, signed).
> 195                     block_adr = block_num * mtd->erasesize;
> 196
> 197                     if (mtd_block_isbad(mtd, block_adr))
> 198                             continue;
> 199
> 200                     if (sharpsl_nand_read_oob(mtd, block_adr, oob))
>
>
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/
> wf/click?
> upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRZILrCyVW9WQLwr7wR5iGubla-2BOU6Se2Euumsa1bPpOJg-3D-3D_p9c2Pq5BRWXelYclnUuZY8l3SYc-2FPJtx2STX-2BpF5A6td-2FPvSZFHfZnIH7Pkuotsj40dgSYlEHnY3fa8hwUfXcZx2zIMi9ygf9fvrncbz9LZ035WDZixEDGKJm-2BkA-2FlNNw4vjG8qChO-2Fsn3KGwsVc2B46OlL-2BawwYWOieMWWWlm44I8px-2BoB-2BuIPoEApB2C1WKE6DZ0qI9OL3nk2Lf6cdMA-3D-3D
>
> To manage Coverity Scan email notifications for "richard@nod.at", click
> https://u2389337.ct.sendgrid.net/wf/click?
> upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4PW1q5HE7hT0R7FQopI50uMi-2FXxBV9hqQbIHwUk8i5vtfR-2BsAt7Vmc5VhhK-2BFpW3LXtshV-2BNRMG6fEVAWxE0JubBijVgPqNmrGAZeWjyxZKc-3D_p9c2Pq5BRWXelYclnUuZY8l3SYc-2FPJtx2STX-2BpF5A6td-2FPvSZFHfZnIH7PkuotsjtjhWtoA3IVRL5bV1R9qiwG4cqA-2FKu4HHy-2FY11gvyLcPSbxHkMq75abGzuiZVVK-2BoMEddCyVJkKCXyuwR33GIGAu-2F-2BxMohkiXZLfEkTEF4Cq4rSxCKccF8nqIWYFGjwP7kzyOHAH5cWC2z4YWNyegGg-3D-3D
>
>
> -------------------------------------------------------------

Re: New Defects reported by Coverity Scan for Linux

[Richard Weinberger]

Am Montag, 27. November 2017, 15:19:20 CET schrieb Andrea Adami:
> On Mon, Nov 27, 2017 at 2:22 PM, Richard Weinberger <richard@nod.at> wrote:
> > Andrea,
> > 
> > please check. The same pattern seems to be used more than once in this
> > driver.
> > 
> > Thanks,
> > //richard
> 
> Hello,
> I acked the patch v2, considering 2 occurrences:
> 
> https://lkml.org/lkml/2017/11/8/567
> 
> I thought that was enough, what should I do now?

Okay, I thought this is a new issue. Then let's keep
this patch. I'll send it to Linus.

Thanks,
//richard

Fw: New Defects reported by Coverity Scan for Linux

[Stephen Hemminger]

The TIPC case is a missing check for memory allocation failure.


Begin forwarded message:

Date: Mon, 02 Nov 2015 23:45:55 -0800
From: scan-admin@coverity.com
To: stephen@networkplumber.org
Subject: New Defects reported by Coverity Scan for Linux



Hi,

Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.

4 new defect(s) introduced to Linux found with Coverity Scan.
9 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)


** CID 1338065:  Error handling issues  (CHECKED_RETURN)
/net/tipc/udp_media.c: 162 in tipc_udp_send_msg()


________________________________________________________________________________________________________
*** CID 1338065:  Error handling issues  (CHECKED_RETURN)
/net/tipc/udp_media.c: 162 in tipc_udp_send_msg()
156     	struct udp_media_addr *dst = (struct udp_media_addr *)&dest->value;
157     	struct udp_media_addr *src = (struct udp_media_addr *)&b->addr.value;
158     	struct sk_buff *clone;
159     	struct rtable *rt;
160     
161     	if (skb_headroom(skb) < UDP_MIN_HEADROOM)
>>>     CID 1338065:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "pskb_expand_head" without checking return value (as is done elsewhere 51 out of 56 times).
162     		pskb_expand_head(skb, UDP_MIN_HEADROOM, 0, GFP_ATOMIC);
163     
164     	clone = skb_clone(skb, GFP_ATOMIC);
165     	skb_set_inner_protocol(clone, htons(ETH_P_TIPC));
166     	ub = rcu_dereference_rtnl(b->media_ptr);
167     	if (!ub) {

** CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
/net/openvswitch/flow_netlink.c: 1379 in __ovs_nla_put_key()


________________________________________________________________________________________________________
*** CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
/net/openvswitch/flow_netlink.c: 1379 in __ovs_nla_put_key()
1373     		goto nla_put_failure;
1374     
1375     	if (nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output->phy.priority))
1376     		goto nla_put_failure;
1377     
1378     	if ((swkey->tun_key.u.ipv4.dst || is_mask)) {
>>>     CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
>>>     Assigning: "opts" = "NULL".
1379     		const void *opts = NULL;
1380     
1381     		if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
1382     			opts = TUN_METADATA_OPTS(output, swkey->tun_opts_len);
1383     
1384     		if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,

** CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/drivers/net/ethernet/cavium/thunder/nic_main.c: 407 in nic_config_cpi()


________________________________________________________________________________________________________
*** CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/drivers/net/ethernet/cavium/thunder/nic_main.c: 407 in nic_config_cpi()
401     			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi << 3),
402     				      (vnic << 24) | (padd << 16) |
403     				      (rssi_base + rssi));
404     		} else {
405     			/* Set MPI_ALG to '0' to disable MCAM parsing */
406     			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi << 3),
>>>     CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>     Potentially overflowing expression "padd << 16" with type "u32" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "u64" (64 bits, unsigned).
407     				      (padd << 16));
408     			/* MPI index is same as CPI if MPI_ALG is not enabled */
409     			nic_reg_write(nic, NIC_PF_MPI_0_2047_CFG | (cpi << 3),
410     				      (vnic << 24) | (rssi_base + rssi));
411     		}
412     

** CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
/include/net/dst_metadata.h: 69 in tun_dst_unclone()


________________________________________________________________________________________________________
*** CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
/include/net/dst_metadata.h: 69 in tun_dst_unclone()
63     static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
64     {
65     	struct metadata_dst *md_dst = skb_metadata_dst(skb);
66     	int md_size = md_dst->u.tun_info.options_len;
67     	struct metadata_dst *new_md;
68     
>>>     CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "md_dst" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
69     	if (!md_dst)
70     		return ERR_PTR(-EINVAL);
71     
72     	new_md = metadata_dst_alloc(md_size, GFP_ATOMIC);
73     	if (!new_md)
74     		return ERR_PTR(-ENOMEM);


________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://scan.coverity.com/projects/linux?tab=overview

To manage Coverity Scan email notifications for "stephen@networkplumber.org", click https://scan.coverity.com/subscriptions/edit?email=stephen%40networkplumber.org&token=41b352b884ef3fc73426635eebc294c3

RE: New Defects reported by Coverity Scan for Linux

[Jon Maloy]

This patch went to 'stable', so I am a little uncertain about how to handle it.
Do I need to post a trailing commit that also goes to stable, or is net-next sufficient?
Personally, I would prefer the latter.

///jon

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 03 November, 2015 11:18
> To: Jon Maloy; Ying Xue
> Cc: netdev@vger.kernel.org
> Subject: Fw: New Defects reported by Coverity Scan for Linux
> 
> The TIPC case is a missing check for memory allocation failure.
> 
> 
> Begin forwarded message:
> 
> Date: Mon, 02 Nov 2015 23:45:55 -0800
> From: scan-admin@coverity.com
> To: stephen@networkplumber.org
> Subject: New Defects reported by Coverity Scan for Linux
> 
> 
> 
> Hi,
> 
> Please find the latest report on new defect(s) introduced to Linux found with
> Coverity Scan.
> 
> 4 new defect(s) introduced to Linux found with Coverity Scan.
> 9 defect(s), reported by Coverity Scan earlier, were marked fixed in the
> recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan Showing 4 of 4 defect(s)
> 
> 
> ** CID 1338065:  Error handling issues  (CHECKED_RETURN)
> /net/tipc/udp_media.c: 162 in tipc_udp_send_msg()
> 
> 
> __________________________________________________________
> ______________________________________________
> *** CID 1338065:  Error handling issues  (CHECKED_RETURN)
> /net/tipc/udp_media.c: 162 in tipc_udp_send_msg()
> 156     	struct udp_media_addr *dst = (struct udp_media_addr *)&dest-
> >value;
> 157     	struct udp_media_addr *src = (struct udp_media_addr *)&b-
> >addr.value;
> 158     	struct sk_buff *clone;
> 159     	struct rtable *rt;
> 160
> 161     	if (skb_headroom(skb) < UDP_MIN_HEADROOM)
> >>>     CID 1338065:  Error handling issues  (CHECKED_RETURN)
> >>>     Calling "pskb_expand_head" without checking return value (as is done
> elsewhere 51 out of 56 times).
> 162     		pskb_expand_head(skb, UDP_MIN_HEADROOM, 0,
> GFP_ATOMIC);
> 163
> 164     	clone = skb_clone(skb, GFP_ATOMIC);
> 165     	skb_set_inner_protocol(clone, htons(ETH_P_TIPC));
> 166     	ub = rcu_dereference_rtnl(b->media_ptr);
> 167     	if (!ub) {
> 
> ** CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
> /net/openvswitch/flow_netlink.c: 1379 in __ovs_nla_put_key()
> 
> 
> __________________________________________________________
> ______________________________________________
> *** CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
> /net/openvswitch/flow_netlink.c: 1379 in __ovs_nla_put_key()
> 1373     		goto nla_put_failure;
> 1374
> 1375     	if (nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output-
> >phy.priority))
> 1376     		goto nla_put_failure;
> 1377
> 1378     	if ((swkey->tun_key.u.ipv4.dst || is_mask)) {
> >>>     CID 1338066:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Assigning: "opts" = "NULL".
> 1379     		const void *opts = NULL;
> 1380
> 1381     		if (output->tun_key.tun_flags &
> TUNNEL_OPTIONS_PRESENT)
> 1382     			opts = TUN_METADATA_OPTS(output, swkey-
> >tun_opts_len);
> 1383
> 1384     		if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,
> 
> ** CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /drivers/net/ethernet/cavium/thunder/nic_main.c: 407 in nic_config_cpi()
> 
> 
> __________________________________________________________
> ______________________________________________
> *** CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /drivers/net/ethernet/cavium/thunder/nic_main.c: 407 in nic_config_cpi()
> 401     			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi <<
> 3),
> 402     				      (vnic << 24) | (padd << 16) |
> 403     				      (rssi_base + rssi));
> 404     		} else {
> 405     			/* Set MPI_ALG to '0' to disable MCAM parsing */
> 406     			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi <<
> 3),
> >>>     CID 1338067:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "padd << 16" with type "u32" (32
> bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a
> context that expects an expression of type "u64" (64 bits, unsigned).
> 407     				      (padd << 16));
> 408     			/* MPI index is same as CPI if MPI_ALG is not
> enabled */
> 409     			nic_reg_write(nic, NIC_PF_MPI_0_2047_CFG | (cpi
> << 3),
> 410     				      (vnic << 24) | (rssi_base + rssi));
> 411     		}
> 412
> 
> ** CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
> /include/net/dst_metadata.h: 69 in tun_dst_unclone()
> 
> 
> __________________________________________________________
> ______________________________________________
> *** CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
> /include/net/dst_metadata.h: 69 in tun_dst_unclone()
> 63     static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
> 64     {
> 65     	struct metadata_dst *md_dst = skb_metadata_dst(skb);
> 66     	int md_size = md_dst->u.tun_info.options_len;
> 67     	struct metadata_dst *new_md;
> 68
> >>>     CID 1338068:  Null pointer dereferences  (REVERSE_INULL)
> >>>     Null-checking "md_dst" suggests that it may be null, but it has already
> been dereferenced on all paths leading to the check.
> 69     	if (!md_dst)
> 70     		return ERR_PTR(-EINVAL);
> 71
> 72     	new_md = metadata_dst_alloc(md_size, GFP_ATOMIC);
> 73     	if (!new_md)
> 74     		return ERR_PTR(-ENOMEM);
> 
> 
> __________________________________________________________
> ______________________________________________
> To view the defects in Coverity Scan visit,
> https://scan.coverity.com/projects/linux?tab=overview
> 
> To manage Coverity Scan email notifications for
> "stephen@networkplumber.org", click
> https://scan.coverity.com/subscriptions/edit?email=stephen%40networkplu
> mber.org&token=41b352b884ef3fc73426635eebc294c3