| Message ID | 20190809175741.7066-1-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [V3,1/3] PCI: rcar: Move the inbound index check | expand |
On Fri, Aug 09, 2019 at 07:57:39PM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > Since the $idx variable value is stored across multiple calls to > rcar_pcie_inbound_ranges() function, and the $idx value is used to > index registers which are written, subsequent calls might cause > the $idx value to be high enough to trigger writes into nonexistent > registers. Can this really happen ? 'index' is initialized to 0 in rcar_pci_parse_map_dma_ranges() and, through rcar_pcie_inbound_ranges() return value, it bails out on idx overrun, we can argue this patch improves robustness but I do not think it is fixing anything. Lorenzo > Fix this by moving the $idx value check to the beginning of the loop. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > --- > V2: New patch > V3: Adjust the check to idx >= MAX_NR_INBOUND_MAPS - 1 > --- > drivers/pci/controller/pcie-rcar.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index f6a669a9af41..56a6433eb70b 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -1048,6 +1048,10 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, > mask &= ~0xf; > > while (cpu_addr < cpu_end) { > + if (idx >= MAX_NR_INBOUND_MAPS - 1) { > + dev_err(pcie->dev, "Failed to map inbound regions!\n"); > + return -EINVAL; > + } > /* > * Set up 64-bit inbound regions as the range parser doesn't > * distinguish between 32 and 64-bit types. > @@ -1067,11 +1071,6 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, > pci_addr += size; > cpu_addr += size; > idx += 2; > - > - if (idx > MAX_NR_INBOUND_MAPS) { > - dev_err(pcie->dev, "Failed to map inbound regions!\n"); > - return -EINVAL; > - } > } > *index = idx; > > -- > 2.20.1 >
On 8/16/19 12:52 PM, Lorenzo Pieralisi wrote: > On Fri, Aug 09, 2019 at 07:57:39PM +0200, marek.vasut@gmail.com wrote: >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> Since the $idx variable value is stored across multiple calls to >> rcar_pcie_inbound_ranges() function, and the $idx value is used to >> index registers which are written, subsequent calls might cause >> the $idx value to be high enough to trigger writes into nonexistent >> registers. > > Can this really happen ? 'index' is initialized to 0 in > rcar_pci_parse_map_dma_ranges() and, through rcar_pcie_inbound_ranges() > return value, it bails out on idx overrun, we can argue this patch > improves robustness but I do not think it is fixing anything. We probably don't want to write into non-existent registers ? I think it can happen when there are too many ranges in DT.
On Fri, Aug 16, 2019 at 12:59:51PM +0200, Marek Vasut wrote: > On 8/16/19 12:52 PM, Lorenzo Pieralisi wrote: > > On Fri, Aug 09, 2019 at 07:57:39PM +0200, marek.vasut@gmail.com wrote: > >> From: Marek Vasut <marek.vasut+renesas@gmail.com> > >> > >> Since the $idx variable value is stored across multiple calls to > >> rcar_pcie_inbound_ranges() function, and the $idx value is used to > >> index registers which are written, subsequent calls might cause > >> the $idx value to be high enough to trigger writes into nonexistent > >> registers. > > > > Can this really happen ? 'index' is initialized to 0 in > > rcar_pci_parse_map_dma_ranges() and, through rcar_pcie_inbound_ranges() > > return value, it bails out on idx overrun, we can argue this patch > > improves robustness but I do not think it is fixing anything. > > We probably don't want to write into non-existent registers ? I have not questioned that. > I think it can happen when there are too many ranges in DT. Yes that's true when idx == MAX_NR_INBOUND_MAPS, forgive me the question. Lorenzo
On 8/9/19 7:57 PM, Marek Vasut wrote: > Since the $idx variable value is stored across multiple calls to > rcar_pcie_inbound_ranges() function, and the $idx value is used to > index registers which are written, subsequent calls might cause > the $idx value to be high enough to trigger writes into nonexistent > registers. > > Fix this by moving the $idx value check to the beginning of the loop. Is there any reason why none of these patches got applied yet ? [...]
On Fri, Aug 09, 2019 at 07:57:39PM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > Since the $idx variable value is stored across multiple calls to > rcar_pcie_inbound_ranges() function, and the $idx value is used to > index registers which are written, subsequent calls might cause > the $idx value to be high enough to trigger writes into nonexistent > registers. > > Fix this by moving the $idx value check to the beginning of the loop. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > --- > V2: New patch > V3: Adjust the check to idx >= MAX_NR_INBOUND_MAPS - 1 > --- > drivers/pci/controller/pcie-rcar.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > Reviewed-by: Andrew Murray <andrew.murray@arm.com> > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index f6a669a9af41..56a6433eb70b 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -1048,6 +1048,10 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, > mask &= ~0xf; > > while (cpu_addr < cpu_end) { > + if (idx >= MAX_NR_INBOUND_MAPS - 1) { > + dev_err(pcie->dev, "Failed to map inbound regions!\n"); > + return -EINVAL; > + } > /* > * Set up 64-bit inbound regions as the range parser doesn't > * distinguish between 32 and 64-bit types. > @@ -1067,11 +1071,6 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, > pci_addr += size; > cpu_addr += size; > idx += 2; > - > - if (idx > MAX_NR_INBOUND_MAPS) { > - dev_err(pcie->dev, "Failed to map inbound regions!\n"); > - return -EINVAL; > - } > } > *index = idx; > > -- > 2.20.1 >
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index f6a669a9af41..56a6433eb70b 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -1048,6 +1048,10 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, mask &= ~0xf; while (cpu_addr < cpu_end) { + if (idx >= MAX_NR_INBOUND_MAPS - 1) { + dev_err(pcie->dev, "Failed to map inbound regions!\n"); + return -EINVAL; + } /* * Set up 64-bit inbound regions as the range parser doesn't * distinguish between 32 and 64-bit types. @@ -1067,11 +1071,6 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, pci_addr += size; cpu_addr += size; idx += 2; - - if (idx > MAX_NR_INBOUND_MAPS) { - dev_err(pcie->dev, "Failed to map inbound regions!\n"); - return -EINVAL; - } } *index = idx;