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

[Will Deacon]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/