| Message ID | 20210421184747.62391-1-ameynarkhede03@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Check value of resource alignment before using __ffs | expand |
On Thu, Apr 22, 2021 at 12:17:47AM +0530, Amey Narkhede wrote: > Return value of __ffs is undefined if no set bit exists in > its argument. This indicates that the associated BAR has > invalid alignment. > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > --- > drivers/pci/setup-bus.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 2ce636937c6e..44e8449418ae 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -1044,6 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > * resources. > */ > align = pci_resource_alignment(dev, r); > + if (!align) { > + pci_warn(dev, "BAR %d: %pR has bogus alignment\n", > + i, r); > + continue; > + } I see that you copied it from pdev_sort_resources(), but it is incorrect change, see how negative order is handled and later ARRAY_SIZE() check. Thanks > order = __ffs(align) - 20; > if (order < 0) > order = 0; > -- > 2.31.1
On 21/04/22 09:43AM, Leon Romanovsky wrote: > On Thu, Apr 22, 2021 at 12:17:47AM +0530, Amey Narkhede wrote: > > Return value of __ffs is undefined if no set bit exists in > > its argument. This indicates that the associated BAR has > > invalid alignment. > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > > --- > > drivers/pci/setup-bus.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index 2ce636937c6e..44e8449418ae 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -1044,6 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > > * resources. > > */ > > align = pci_resource_alignment(dev, r); > > + if (!align) { > > + pci_warn(dev, "BAR %d: %pR has bogus alignment\n", > > + i, r); > > + continue; > > + } > > I see that you copied it from pdev_sort_resources(), but it is > incorrect change, see how negative order is handled and later > ARRAY_SIZE() check. > > Thanks > Is it guaranteed that it will return value which will result in negative value or >= ARRAY_SIZE? Comment on __ffs says value is undefined for 0 that means it could be anything or am I missing something? Thanks, Amey
On Thu, Apr 22, 2021 at 03:13:23PM +0530, Amey Narkhede wrote: > On 21/04/22 09:43AM, Leon Romanovsky wrote: > > On Thu, Apr 22, 2021 at 12:17:47AM +0530, Amey Narkhede wrote: > > > Return value of __ffs is undefined if no set bit exists in > > > its argument. This indicates that the associated BAR has > > > invalid alignment. > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > > > --- > > > drivers/pci/setup-bus.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > index 2ce636937c6e..44e8449418ae 100644 > > > --- a/drivers/pci/setup-bus.c > > > +++ b/drivers/pci/setup-bus.c > > > @@ -1044,6 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > > > * resources. > > > */ > > > align = pci_resource_alignment(dev, r); > > > + if (!align) { > > > + pci_warn(dev, "BAR %d: %pR has bogus alignment\n", > > > + i, r); > > > + continue; > > > + } > > > > I see that you copied it from pdev_sort_resources(), but it is > > incorrect change, see how negative order is handled and later > > ARRAY_SIZE() check. > > > > Thanks > > > Is it guaranteed that it will return value which will result > in negative value or >= ARRAY_SIZE? Comment on __ffs says value > is undefined for 0 that means it could be anything or am I missing > something? diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 2ce636937c6e..ce5380bdd2fd 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -1044,10 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, * resources. */ align = pci_resource_alignment(dev, r); - order = __ffs(align) - 20; - if (order < 0) - order = 0; - if (order >= ARRAY_SIZE(aligns)) { + if (align) { + order = __ffs(align) - 20; + order = (order < 0) ? 0 : order; + } + if (!align || order >= ARRAY_SIZE(aligns)) { pci_warn(dev, "disabling BAR %d: %pR (bad alignment %#llx)\n", i, r, (unsigned long long) align); r->flags = 0; > > Thanks, > Amey
On 21/04/22 01:41PM, Leon Romanovsky wrote: > On Thu, Apr 22, 2021 at 03:13:23PM +0530, Amey Narkhede wrote: > > On 21/04/22 09:43AM, Leon Romanovsky wrote: > > > On Thu, Apr 22, 2021 at 12:17:47AM +0530, Amey Narkhede wrote: > > > > Return value of __ffs is undefined if no set bit exists in > > > > its argument. This indicates that the associated BAR has > > > > invalid alignment. > > > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > > > > --- > > > > drivers/pci/setup-bus.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > > index 2ce636937c6e..44e8449418ae 100644 > > > > --- a/drivers/pci/setup-bus.c > > > > +++ b/drivers/pci/setup-bus.c > > > > @@ -1044,6 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > > > > * resources. > > > > */ > > > > align = pci_resource_alignment(dev, r); > > > > + if (!align) { > > > > + pci_warn(dev, "BAR %d: %pR has bogus alignment\n", > > > > + i, r); > > > > + continue; > > > > + } > > > > > > I see that you copied it from pdev_sort_resources(), but it is > > > incorrect change, see how negative order is handled and later > > > ARRAY_SIZE() check. > > > > > > Thanks > > > > > Is it guaranteed that it will return value which will result > > in negative value or >= ARRAY_SIZE? Comment on __ffs says value > > is undefined for 0 that means it could be anything or am I missing > > something? > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 2ce636937c6e..ce5380bdd2fd 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -1044,10 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > * resources. > */ > align = pci_resource_alignment(dev, r); > - order = __ffs(align) - 20; > - if (order < 0) > - order = 0; > - if (order >= ARRAY_SIZE(aligns)) { > + if (align) { > + order = __ffs(align) - 20; > + order = (order < 0) ? 0 : order; > + } > + if (!align || order >= ARRAY_SIZE(aligns)) { > pci_warn(dev, "disabling BAR %d: %pR (bad alignment %#llx)\n", > i, r, (unsigned long long) align); > r->flags = 0; > > Oh I see. Thanks. I'll correct this in v2. Thanks, Amey
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 2ce636937c6e..44e8449418ae 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -1044,6 +1044,11 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, * resources. */ align = pci_resource_alignment(dev, r); + if (!align) { + pci_warn(dev, "BAR %d: %pR has bogus alignment\n", + i, r); + continue; + } order = __ffs(align) - 20; if (order < 0) order = 0;
Return value of __ffs is undefined if no set bit exists in its argument. This indicates that the associated BAR has invalid alignment. Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> --- drivers/pci/setup-bus.c | 5 +++++ 1 file changed, 5 insertions(+) -- 2.31.1