Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231624 > unrolled thread

Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

Started byWill Deacon <will.deacon@arm.com>
First post2015-09-23 20:10 +0200
Last post2015-09-23 21:40 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in  bus_max calculation. Will Deacon <will.deacon@arm.com> - 2015-09-23 20:10 +0200
    Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation. Arnd Bergmann <arnd@arndb.de> - 2015-09-23 21:30 +0200
      Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation. Arnd Bergmann <arnd@arndb.de> - 2015-09-23 21:40 +0200
        Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in  bus_max calculation. Will Deacon <will.deacon@arm.com> - 2015-09-23 21:50 +0200
        Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation. Arnd Bergmann <arnd@arndb.de> - 2015-09-23 22:50 +0200
      Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in  bus_max calculation. Will Deacon <will.deacon@arm.com> - 2015-09-23 21:40 +0200
    Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in  bus_max calculation. Will Deacon <will.deacon@arm.com> - 2015-09-23 21:40 +0200

#1231624 — Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

FromWill Deacon <will.deacon@arm.com>
Date2015-09-23 20:10 +0200
SubjectRe: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Message-ID<qbWEq-3q5-33@gated-at.bofh.it>
On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> There are two problems with the bus_max calculation:
> 
> 1) The u8 data type can overflow for large config space windows.
> 
> 2) The calculation is incorrect for a bus range that doesn't start at
>    zero.
> 
> Since the configuration space is relative to bus zero, make bus_max
> just be the size of the config window scaled by bus_shift.  Then clamp
> it to a maximum of 255, per PCI.  Use a data type of int to avoid
> overflow problems.
> 
> Update host-generic-pci.txt to clarify the semantics of the "reg"
> property with respect to non-zero starting bus numbers.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
> Change from v1: Added text to host-generic-pci.txt
> 
>  Documentation/devicetree/bindings/pci/host-generic-pci.txt | 4 +++-
>  drivers/pci/host/pci-host-generic.c                        | 7 ++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> index cf3e205..105a968 100644
> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> @@ -34,7 +34,9 @@ Properties of the host controller node:
>  - #size-cells    : Must be 2.
>  
>  - reg            : The Configuration Space base address and size, as accessed
> -                   from the parent bus.
> +                   from the parent bus.  The base address corresponds to
> +                   bus zero, even though the "bus-range" property may specify
> +                   a different starting bus number.

That's a useful clarification, thanks.

>  Properties of the /chosen node:
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 77cf4bd..0a9c453 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -164,7 +164,7 @@ out_release_res:
>  static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  {
>  	int err;
> -	u8 bus_max;
> +	int bus_max;
>  	resource_size_t busn;
>  	struct resource *bus_range;
>  	struct device *dev = pci->host.dev.parent;
> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  	}
>  
>  	/* Limit the bus-range to fit within reg */
> -	bus_max = pci->cfg.bus_range->start +
> -		  (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> +	bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> +	if (bus_max > 255)
> +		bus_max = 255;

I still don't understand the need for this part. If the cfg space is bigger
than bus_max, isn't that simply an invalid resource? Given that the resource
could be broken in other ways too, this check feels more like a specific
workaround rather than generally useful code.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1231668 — Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

FromArnd Bergmann <arnd@arndb.de>
Date2015-09-23 21:30 +0200
SubjectRe: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Message-ID<qbXTP-58R-3@gated-at.bofh.it>
In reply to#1231624
On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> >>
> >>      /* Limit the bus-range to fit within reg */
> >> -    bus_max = pci->cfg.bus_range->start +
> >> -              (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +    bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +    if (bus_max > 255)
> >> +            bus_max = 255;
> >
> > I still don't understand the need for this part. If the cfg space is bigger
> > than bus_max, isn't that simply an invalid resource? Given that the resource
> > could be broken in other ways too, this check feels more like a specific
> > workaround rather than generally useful code.
> 
> Imagine...
> 
>    bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> entire range of 0..0xff.
> 
>    according to the calculations above, (resource_size(&pci->cfg.res) >> 
> pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...

Extending the computation to 32 bit seems fine, but I'd rather warn loudly
if the bus range does not fit within the registers.

Also note that the computation is already correct with my interpretation
of the reg property.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231672 — Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

FromArnd Bergmann <arnd@arndb.de>
Date2015-09-23 21:40 +0200
SubjectRe: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Message-ID<qbY3v-5k0-1@gated-at.bofh.it>
In reply to#1231668
On Wednesday 23 September 2015 20:35:45 Will Deacon wrote:
> On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote:
> > On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> > > >>
> > > >>      /* Limit the bus-range to fit within reg */
> > > >> -    bus_max = pci->cfg.bus_range->start +
> > > >> -              (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > > >> +    bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > > >> +    if (bus_max > 255)
> > > >> +            bus_max = 255;
> > > >
> > > > I still don't understand the need for this part. If the cfg space is bigger
> > > > than bus_max, isn't that simply an invalid resource? Given that the resource
> > > > could be broken in other ways too, this check feels more like a specific
> > > > workaround rather than generally useful code.
> > > 
> > > Imagine...
> > > 
> > >    bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> > > entire range of 0..0xff.
> > > 
> > >    according to the calculations above, (resource_size(&pci->cfg.res) >> 
> > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> > 
> > Extending the computation to 32 bit seems fine, but I'd rather warn loudly
> > if the bus range does not fit within the registers.
> > 
> > Also note that the computation is already correct with my interpretation
> > of the reg property.
> 
> From what Lorenzo was saying, ACPI shares the interpretation that David is
> implementing here and, given that the DT version seems to be subjective,
> aligning this reg property with MMCFG seems to make sense.

We should then make it very clear in the binding that the driver
is not allowed to actually map the registers for the buses outside
of the bus-range, as that is highly unusual.

We would also need a special exception for this if we ever get to
implement the DT source checker that we have been talking about for
years, as the reg property might then overlap with a property from
another device.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231682

FromWill Deacon <will.deacon@arm.com>
Date2015-09-23 21:50 +0200
Message-ID<qbYdd-5vh-23@gated-at.bofh.it>
In reply to#1231672
On Wed, Sep 23, 2015 at 08:39:27PM +0100, Arnd Bergmann wrote:
> On Wednesday 23 September 2015 20:35:45 Will Deacon wrote:
> > On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote:
> > > On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> > > > >>
> > > > >>      /* Limit the bus-range to fit within reg */
> > > > >> -    bus_max = pci->cfg.bus_range->start +
> > > > >> -              (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > > > >> +    bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > > > >> +    if (bus_max > 255)
> > > > >> +            bus_max = 255;
> > > > >
> > > > > I still don't understand the need for this part. If the cfg space is bigger
> > > > > than bus_max, isn't that simply an invalid resource? Given that the resource
> > > > > could be broken in other ways too, this check feels more like a specific
> > > > > workaround rather than generally useful code.
> > > > 
> > > > Imagine...
> > > > 
> > > >    bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> > > > entire range of 0..0xff.
> > > > 
> > > >    according to the calculations above, (resource_size(&pci->cfg.res) >> 
> > > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> > > 
> > > Extending the computation to 32 bit seems fine, but I'd rather warn loudly
> > > if the bus range does not fit within the registers.
> > > 
> > > Also note that the computation is already correct with my interpretation
> > > of the reg property.
> > 
> > From what Lorenzo was saying, ACPI shares the interpretation that David is
> > implementing here and, given that the DT version seems to be subjective,
> > aligning this reg property with MMCFG seems to make sense.
> 
> We should then make it very clear in the binding that the driver
> is not allowed to actually map the registers for the buses outside
> of the bus-range, as that is highly unusual.
> 
> We would also need a special exception for this if we ever get to
> implement the DT source checker that we have been talking about for
> years, as the reg property might then overlap with a property from
> another device.

Completely agreed. Having a base that isn't actually safe to map is horrible
and should be called out.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231716 — Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

FromArnd Bergmann <arnd@arndb.de>
Date2015-09-23 22:50 +0200
SubjectRe: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Message-ID<qbZ9g-6R6-13@gated-at.bofh.it>
In reply to#1231672
On Wednesday 23 September 2015 21:39:27 Arnd Bergmann wrote:
> On Wednesday 23 September 2015 20:35:45 Will Deacon wrote:
> > 
> > From what Lorenzo was saying, ACPI shares the interpretation that David is
> > implementing here and, given that the DT version seems to be subjective,
> > aligning this reg property with MMCFG seems to make sense.
> 
> We should then make it very clear in the binding that the driver
> is not allowed to actually map the registers for the buses outside
> of the bus-range, as that is highly unusual.

One more point here: I think ACPI does something different here,
it lists the base address of mmconfig space of the the PCI domain
that the host bridge belongs to. In contrast, in DT the 'reg'
property is defined as the registers belonging to the host bridge
itself, and the DT does not say anything about domains at all
(aside from the linux,pci-domain property that is not mandatory
and a Linux-specific extension).

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231674

FromWill Deacon <will.deacon@arm.com>
Date2015-09-23 21:40 +0200
Message-ID<qbY3v-5k0-3@gated-at.bofh.it>
In reply to#1231668
On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote:
> On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> > >>
> > >>      /* Limit the bus-range to fit within reg */
> > >> -    bus_max = pci->cfg.bus_range->start +
> > >> -              (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > >> +    bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> > >> +    if (bus_max > 255)
> > >> +            bus_max = 255;
> > >
> > > I still don't understand the need for this part. If the cfg space is bigger
> > > than bus_max, isn't that simply an invalid resource? Given that the resource
> > > could be broken in other ways too, this check feels more like a specific
> > > workaround rather than generally useful code.
> > 
> > Imagine...
> > 
> >    bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> > entire range of 0..0xff.
> > 
> >    according to the calculations above, (resource_size(&pci->cfg.res) >> 
> > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> 
> Extending the computation to 32 bit seems fine, but I'd rather warn loudly
> if the bus range does not fit within the registers.
> 
> Also note that the computation is already correct with my interpretation
> of the reg property.

From what Lorenzo was saying, ACPI shares the interpretation that David is
implementing here and, given that the DT version seems to be subjective,
aligning this reg property with MMCFG seems to make sense.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231676

FromWill Deacon <will.deacon@arm.com>
Date2015-09-23 21:40 +0200
Message-ID<qbY3B-5k0-21@gated-at.bofh.it>
In reply to#1231624
On Wed, Sep 23, 2015 at 07:21:56PM +0100, David Daney wrote:
> On 09/23/2015 11:01 AM, Will Deacon wrote:
> > On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote:
> [...]
> >
> >>   Properties of the /chosen node:
> >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> >> index 77cf4bd..0a9c453 100644
> >> --- a/drivers/pci/host/pci-host-generic.c
> >> +++ b/drivers/pci/host/pci-host-generic.c
> >> @@ -164,7 +164,7 @@ out_release_res:
> >>   static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> >>   {
> >>   	int err;
> >> -	u8 bus_max;
> >> +	int bus_max;
> >>   	resource_size_t busn;
> >>   	struct resource *bus_range;
> >>   	struct device *dev = pci->host.dev.parent;
> >> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> >>   	}
> >>
> >>   	/* Limit the bus-range to fit within reg */
> >> -	bus_max = pci->cfg.bus_range->start +
> >> -		  (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +	bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +	if (bus_max > 255)
> >> +		bus_max = 255;
> >
> > I still don't understand the need for this part. If the cfg space is bigger
> > than bus_max, isn't that simply an invalid resource? Given that the resource
> > could be broken in other ways too, this check feels more like a specific
> > workaround rather than generally useful code.
> 
> Imagine...
> 
>    bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> entire range of 0..0xff.
> 
>    according to the calculations above, (resource_size(&pci->cfg.res) >> 
> pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> 
>    bus_max = 0x80 + 0xff -> OVERFLOW of u8!
> 
> That is not useful.  bus_max should represent the largest bus number 
> that can be covered by cfg.res.  That is what my patch is attempting to 
> accomplish.  Calculate the largest bus number that can be accommodated 
> by cfg.res, and then clamp it to 0xff.

Sorry, I should've been more specific. The only part I don't like is the
'if (bus_max > 255)' check.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web