| Message ID | ceb5eb396e417f9e45d39fd5ef565ba77aae6a63.1612389406.git.gustavo.pimentel@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw-edma: HDMA support | expand |
[+cc Krzysztof] From reading the subject, I thought you were adding a function to check the return values, i.e., a "checker." But you're really adding "checks" :) On Wed, Feb 03, 2021 at 10:58:06PM +0100, Gustavo Pimentel wrote: > Detected by CoverityScan CID 16555 ("Dereference null return") > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > --- > drivers/dma/dw-edma/dw-edma-pcie.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c > index 686b4ff..7445033 100644 > --- a/drivers/dma/dw-edma/dw-edma-pcie.c > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c > @@ -238,6 +238,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > dw->rd_ch_cnt = vsec_data.rd_ch_cnt; > > dw->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar]; > + if (!dw->rg_region.vaddr) > + return -ENOMEM; This doesn't seem quite right. If pcim_iomap_table() fails, it returns NULL. But then we assign "vaddr = NULL[vsec_data.rg.bar]" which dereferences the NULL pointer even before your test. This "pcim_iomap_table(dev)[n]" pattern is extremely common. There are over 100 calls of pcim_iomap_table(), and $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l says about 75 of them are of this form, where we dereference the result before testing it. > dw->rg_region.vaddr += vsec_data.rg.off; > dw->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start; > dw->rg_region.paddr += vsec_data.rg.off; > @@ -250,12 +253,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > struct dw_edma_block *dt_block = &vsec_data.dt_wr[i]; > > ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; > + if (!ll_region->vaddr) > + return -ENOMEM; > + > ll_region->vaddr += ll_block->off; > ll_region->paddr = pdev->resource[ll_block->bar].start; > ll_region->paddr += ll_block->off; > ll_region->sz = ll_block->sz; > > dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; > + if (!dt_region->vaddr) > + return -ENOMEM; > + > dt_region->vaddr += dt_block->off; > dt_region->paddr = pdev->resource[dt_block->bar].start; > dt_region->paddr += dt_block->off; > @@ -269,12 +278,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > struct dw_edma_block *dt_block = &vsec_data.dt_rd[i]; > > ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; > + if (!ll_region->vaddr) > + return -ENOMEM; > + > ll_region->vaddr += ll_block->off; > ll_region->paddr = pdev->resource[ll_block->bar].start; > ll_region->paddr += ll_block->off; > ll_region->sz = ll_block->sz; > > dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; > + if (!dt_region->vaddr) > + return -ENOMEM; > + > dt_region->vaddr += dt_block->off; > dt_region->paddr = pdev->resource[dt_block->bar].start; > dt_region->paddr += dt_block->off; > -- > 2.7.4 >
On Mon, Feb 8, 2021 at 19:35:16, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Krzysztof] > > From reading the subject, I thought you were adding a function to > check the return values, i.e., a "checker." But you're really adding > "checks" :) That's true, I will rework the subject. > > On Wed, Feb 03, 2021 at 10:58:06PM +0100, Gustavo Pimentel wrote: > > Detected by CoverityScan CID 16555 ("Dereference null return") > > > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > --- > > drivers/dma/dw-edma/dw-edma-pcie.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c > > index 686b4ff..7445033 100644 > > --- a/drivers/dma/dw-edma/dw-edma-pcie.c > > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c > > @@ -238,6 +238,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > > dw->rd_ch_cnt = vsec_data.rd_ch_cnt; > > > > dw->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar]; > > + if (!dw->rg_region.vaddr) > > + return -ENOMEM; > > This doesn't seem quite right. If pcim_iomap_table() fails, it > returns NULL. But then we assign "vaddr = NULL[vsec_data.rg.bar]" > which dereferences the NULL pointer even before your test. I'll add verification of the pointer given by pcim_iomap_table(pdev) first. > > This "pcim_iomap_table(dev)[n]" pattern is extremely common. There > are over 100 calls of pcim_iomap_table(), and > > $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l > > says about 75 of them are of this form, where we dereference the > result before testing it. That's true, there are a lot of drivers that don't verify that pointer. What do you suggest? 1) To remove the verification so that is aligned with the other drivers 2) Leave it as is. Or even to add this verification to the other drivers? Either way, I will add the pcim_iomap_table(pdev) before this instruction. > > > dw->rg_region.vaddr += vsec_data.rg.off; > > dw->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start; > > dw->rg_region.paddr += vsec_data.rg.off; > > @@ -250,12 +253,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > > struct dw_edma_block *dt_block = &vsec_data.dt_wr[i]; > > > > ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; > > + if (!ll_region->vaddr) > > + return -ENOMEM; > > + > > ll_region->vaddr += ll_block->off; > > ll_region->paddr = pdev->resource[ll_block->bar].start; > > ll_region->paddr += ll_block->off; > > ll_region->sz = ll_block->sz; > > > > dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; > > + if (!dt_region->vaddr) > > + return -ENOMEM; > > + > > dt_region->vaddr += dt_block->off; > > dt_region->paddr = pdev->resource[dt_block->bar].start; > > dt_region->paddr += dt_block->off; > > @@ -269,12 +278,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, > > struct dw_edma_block *dt_block = &vsec_data.dt_rd[i]; > > > > ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; > > + if (!ll_region->vaddr) > > + return -ENOMEM; > > + > > ll_region->vaddr += ll_block->off; > > ll_region->paddr = pdev->resource[ll_block->bar].start; > > ll_region->paddr += ll_block->off; > > ll_region->sz = ll_block->sz; > > > > dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; > > + if (!dt_region->vaddr) > > + return -ENOMEM; > > + > > dt_region->vaddr += dt_block->off; > > dt_region->paddr = pdev->resource[dt_block->bar].start; > > dt_region->paddr += dt_block->off; > > -- > > 2.7.4 > >
Hi Gustavo, [...] > > This "pcim_iomap_table(dev)[n]" pattern is extremely common. There > > are over 100 calls of pcim_iomap_table(), and > > > > $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l > > > > says about 75 of them are of this form, where we dereference the > > result before testing it. > > That's true, there are a lot of drivers that don't verify that pointer. > What do you suggest? > 1) To remove the verification so that is aligned with the other drivers > 2) Leave it as is. Or even to add this verification to the other drivers? > > Either way, I will add the pcim_iomap_table(pdev) before this > instruction. [...] A lot of the drivers consume the value from pcim_iomap_table() at a given BAR index directly as-is, some check if the pointer they got back is not NULL, a very few also check if the address at a given index is not NULL. Given that the memory allocation for the table can fail, we ought to check for a NULL pointer. It's a bit worrying that people decided to consume the value it returns directly without any verification. I only found two drivers that perform this additional verification of checking whether the address at a given index is valid, as per: https://lore.kernel.org/linux-pci/YCLFTjZQ2bCfGC+J@rocinante/ Personally, I would opt for (2), and then like you suggested send a separate series to update other drivers so that they also include the this NULL pointer check. But let's wait for Bjorn's take on this, though. Krzysztof
Hi Gustavo, [...] > > That's true, there are a lot of drivers that don't verify that pointer. > > What do you suggest? > > 1) To remove the verification so that is aligned with the other drivers > > 2) Leave it as is. Or even to add this verification to the other drivers? > > > > Either way, I will add the pcim_iomap_table(pdev) before this > > instruction. > [...] > > A lot of the drivers consume the value from pcim_iomap_table() at > a given BAR index directly as-is, some check if the pointer they got > back is not NULL, a very few also check if the address at a given index > is not NULL. > > Given that the memory allocation for the table can fail, we ought to > check for a NULL pointer. It's a bit worrying that people decided to > consume the value it returns directly without any verification. > > I only found two drivers that perform this additional verification of > checking whether the address at a given index is valid, as per: > > https://lore.kernel.org/linux-pci/YCLFTjZQ2bCfGC+J@rocinante/ > > Personally, I would opt for (2), and then like you suggested send > a separate series to update other drivers so that they also include the > this NULL pointer check. > > But let's wait for Bjorn's take on this, though. As per Bjorn's reply: https://lore.kernel.org/linux-pci/20210209185246.GA494880@bjorn-Precision-5520/ These extra checks I proposed would be definitely too much, especially since almost everyone who uses pcim_iomap_table() also calls either pcim_iomap_regions() or pcim_iomap_regions_request_all() before accessing the table. There probably is also an opportunity to simplify some of the other drivers in the future, especially if do some API changes as per what Bjorn suggested. Sorry for taking your time, and thank you again! Krzysztof
diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c index 686b4ff..7445033 100644 --- a/drivers/dma/dw-edma/dw-edma-pcie.c +++ b/drivers/dma/dw-edma/dw-edma-pcie.c @@ -238,6 +238,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, dw->rd_ch_cnt = vsec_data.rd_ch_cnt; dw->rg_region.vaddr = pcim_iomap_table(pdev)[vsec_data.rg.bar]; + if (!dw->rg_region.vaddr) + return -ENOMEM; + dw->rg_region.vaddr += vsec_data.rg.off; dw->rg_region.paddr = pdev->resource[vsec_data.rg.bar].start; dw->rg_region.paddr += vsec_data.rg.off; @@ -250,12 +253,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, struct dw_edma_block *dt_block = &vsec_data.dt_wr[i]; ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; + if (!ll_region->vaddr) + return -ENOMEM; + ll_region->vaddr += ll_block->off; ll_region->paddr = pdev->resource[ll_block->bar].start; ll_region->paddr += ll_block->off; ll_region->sz = ll_block->sz; dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; + if (!dt_region->vaddr) + return -ENOMEM; + dt_region->vaddr += dt_block->off; dt_region->paddr = pdev->resource[dt_block->bar].start; dt_region->paddr += dt_block->off; @@ -269,12 +278,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev, struct dw_edma_block *dt_block = &vsec_data.dt_rd[i]; ll_region->vaddr = pcim_iomap_table(pdev)[ll_block->bar]; + if (!ll_region->vaddr) + return -ENOMEM; + ll_region->vaddr += ll_block->off; ll_region->paddr = pdev->resource[ll_block->bar].start; ll_region->paddr += ll_block->off; ll_region->sz = ll_block->sz; dt_region->vaddr = pcim_iomap_table(pdev)[dt_block->bar]; + if (!dt_region->vaddr) + return -ENOMEM; + dt_region->vaddr += dt_block->off; dt_region->paddr = pdev->resource[dt_block->bar].start; dt_region->paddr += dt_block->off;
Detected by CoverityScan CID 16555 ("Dereference null return") Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> --- drivers/dma/dw-edma/dw-edma-pcie.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)