[PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Geert Uytterhoeven]

drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (slot == PCI_SLOT(devfn) && type == rtype) {
                                       ^

If there's ever gonna be a configuration space or 64-bit memory space
entry in DT, rtype will be uninitialized, and the wrong entry may be
returned.

Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.

Introduced in commit 11be65472a427dcf7a11ab6e3e3628f1c6768b5b ("PCI:
mvebu: Adapt to the new device tree layout").

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
other entries than for I/O or 32-bit memory space?
---
 drivers/pci/host/pci-mvebu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ce23e0f076b6..9515f0d13fd4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 		u32 flags = of_read_number(range, 1);
 		u32 slot = of_read_number(range + 1, 1);
 		u64 cpuaddr = of_read_number(range + na, pna);
-		unsigned long rtype;
+		unsigned long rtype = 0;
 
 		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
 			rtype = IORESOURCE_IO;
-- 
1.9.1

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Bjorn Helgaas]

On Fri, Aug 08, 2014 at 05:34:05PM +0200, Geert Uytterhoeven wrote:
> drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
> drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (slot == PCI_SLOT(devfn) && type == rtype) {
>                                        ^
> 
> If there's ever gonna be a configuration space or 64-bit memory space
> entry in DT, rtype will be uninitialized, and the wrong entry may be
> returned.
> 
> Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.
> 
> Introduced in commit 11be65472a427dcf7a11ab6e3e3628f1c6768b5b ("PCI:
> mvebu: Adapt to the new device tree layout").
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
> DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
> other entries than for I/O or 32-bit memory space?
> ---
>  drivers/pci/host/pci-mvebu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index ce23e0f076b6..9515f0d13fd4 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  		u32 flags = of_read_number(range, 1);
>  		u32 slot = of_read_number(range + 1, 1);
>  		u64 cpuaddr = of_read_number(range + na, pna);
> -		unsigned long rtype;
> +		unsigned long rtype = 0;
>  
>  		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
>  			rtype = IORESOURCE_IO;
> -- 
> 1.9.1
> 

This fix looks right to me.  I added a stable tag as follows.  Thomas
and/or Jason, and you ack this?


commit f96f4040d0d01b6eeacda212cf7db105d06a55ba
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Date:   Fri Aug 8 17:34:05 2014 +0200

    PCI: mvebu: Fix uninitialized "rtype" in mvebu_get_tgt_attr()
    
    drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
    drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
       if (slot == PCI_SLOT(devfn) && type == rtype) {
                                           ^
    
    If there's ever a DT entry other than DT_TYPE_IO or DT_TYPE_MEM32,
    e.g., a configuration space or 64-bit memory space entry, rtype will
    be uninitialized, and the wrong entry may be returned.
    
    Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.
    
    Fixes: 11be65472a42 ("PCI: mvebu: Adapt to the new device tree layout")
    Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: stable@vger.kernel.org	# v3.12+

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a92e0f..081579c0971e 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 		u32 flags = of_read_number(range, 1);
 		u32 slot = of_read_number(range + 1, 1);
 		u64 cpuaddr = of_read_number(range + na, pna);
-		unsigned long rtype;
+		unsigned long rtype = 0;
 
 		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
 			rtype = IORESOURCE_IO;

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Arnd Bergmann]

On Friday 05 September 2014 11:41:22 Bjorn Helgaas wrote:
> On Fri, Aug 08, 2014 at 05:34:05PM +0200, Geert Uytterhoeven wrote:
> > drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
> > drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    if (slot == PCI_SLOT(devfn) && type == rtype) {
> >                                        ^
> > 
> > If there's ever gonna be a configuration space or 64-bit memory space
> > entry in DT, rtype will be uninitialized, and the wrong entry may be
> > returned.
> > 
> > Initialize rtype to 0 (which is an unused IORESOURCE_* type) to fix this.
> > 
> > Introduced in commit 11be65472a427dcf7a11ab6e3e3628f1c6768b5b ("PCI:
> > mvebu: Adapt to the new device tree layout").
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Alternatively, should the "else if (DT_FLAGS_TO_TYPE(flags) ==
> > DT_TYPE_MEM32)" just be changed to "else", assuming there can never be
> > other entries than for I/O or 32-bit memory space?
> > ---
> >  drivers/pci/host/pci-mvebu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index ce23e0f076b6..9515f0d13fd4 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -877,7 +877,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
> >  		u32 flags = of_read_number(range, 1);
> >  		u32 slot = of_read_number(range + 1, 1);
> >  		u64 cpuaddr = of_read_number(range + na, pna);
> > -		unsigned long rtype;
> > +		unsigned long rtype = 0;
> >  
> >  		if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
> >  			rtype = IORESOURCE_IO;
> 
> This fix looks right to me.  I added a stable tag as follows.  Thomas
> and/or Jason, and you ack this?


I had a local fix for this, which I haven't gotten around to
send a proper changelog for, but it seems like a more appropriate
fix, avoiding the spurious initialization.

The other fix looks technically correct as well though.

	Arnd

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a92e0f..678849836649 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -883,6 +883,8 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 			rtype = IORESOURCE_IO;
 		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
 			rtype = IORESOURCE_MEM;
+		else
+			rtype = -1;
 
 		if (slot == PCI_SLOT(devfn) && type == rtype) {
 			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Thomas Petazzoni]

Dear Bjorn Helgaas,

On Fri, 5 Sep 2014 11:41:22 -0600, Bjorn Helgaas wrote:

> This fix looks right to me.  I added a stable tag as follows.  Thomas
> and/or Jason, and you ack this?

Hum, I think I would actually prefer something like:

                if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
                        rtype = IORESOURCE_IO;
                else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
                        rtype = IORESOURCE_MEM;
+		else
+			continue;

So that we're explicit with the fact that we only care about I/O and
MEM32 resource types.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Arnd Bergmann]

On Friday 05 September 2014 20:20:44 Thomas Petazzoni wrote:
> Hum, I think I would actually prefer something like:
> 
>                 if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
>                         rtype = IORESOURCE_IO;
>                 else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
>                         rtype = IORESOURCE_MEM;
> +               else
> +                       continue;
> 
> So that we're explicit with the fact that we only care about I/O and
> MEM32 resource types.

Agreed, that looks better than my patch as well.

	Arnd

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Bjorn Helgaas]

On Fri, Sep 5, 2014 at 12:34 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 05 September 2014 20:20:44 Thomas Petazzoni wrote:
>> Hum, I think I would actually prefer something like:
>>
>>                 if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
>>                         rtype = IORESOURCE_IO;
>>                 else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
>>                         rtype = IORESOURCE_MEM;
>> +               else
>> +                       continue;
>>
>> So that we're explicit with the fact that we only care about I/O and
>> MEM32 resource types.
>
> Agreed, that looks better than my patch as well.

I like it better, too, but we still need the "range += rangesz" part,
so I don't think it will work.  I suppose that could be moved to the
update expression of the "for" loop.  Or, since we don't use "i" in
the loop at all, maybe we could do something like this:

    for (; range < rend; range += rangesz)

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Bjorn Helgaas]

On Fri, Sep 05, 2014 at 01:00:29PM -0600, Bjorn Helgaas wrote:
> On Fri, Sep 5, 2014 at 12:34 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 05 September 2014 20:20:44 Thomas Petazzoni wrote:
> >> Hum, I think I would actually prefer something like:
> >>
> >>                 if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_IO)
> >>                         rtype = IORESOURCE_IO;
> >>                 else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
> >>                         rtype = IORESOURCE_MEM;
> >> +               else
> >> +                       continue;
> >>
> >> So that we're explicit with the fact that we only care about I/O and
> >> MEM32 resource types.
> >
> > Agreed, that looks better than my patch as well.
> 
> I like it better, too, but we still need the "range += rangesz" part,
> so I don't think it will work.  I suppose that could be moved to the
> update expression of the "for" loop.  Or, since we don't use "i" in
> the loop at all, maybe we could do something like this:
> 
>     for (; range < rend; range += rangesz)

Any more input on this?  I don't think I've seen anything actually acked by
Thomas or Jason.

[PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Thomas Petazzoni]

Geert Uytterhoeven reported a warning when building pci-mvebu:

drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (slot == PCI_SLOT(devfn) && type == rtype) {
                                       ^

And indeed, the code of mvebu_get_tgt_attr() may lead to the usage of
rtype when being uninitialized, even though it would only happen if we
had entries other than I/O space and 32 bits memory space.

This commit fixes that by simply skipping the current DT range being
considered, if it doesn't match the resource type we're looking for.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This patch is a different solution than the one proposed by Geert. It
(hopefully) matches the discussion we had with Bjorn and Arnd.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index a8c6f1a..b1315e1 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -873,7 +873,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 	rangesz = pna + na + ns;
 	nranges = rlen / sizeof(__be32) / rangesz;
 
-	for (i = 0; i < nranges; i++) {
+	for (i = 0; i < nranges; i++, range += rangesz) {
 		u32 flags = of_read_number(range, 1);
 		u32 slot = of_read_number(range + 1, 1);
 		u64 cpuaddr = of_read_number(range + na, pna);
@@ -883,14 +883,14 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
 			rtype = IORESOURCE_IO;
 		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
 			rtype = IORESOURCE_MEM;
+		else
+			continue;
 
 		if (slot == PCI_SLOT(devfn) && type == rtype) {
 			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
 			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
 			return 0;
 		}
-
-		range += rangesz;
 	}
 
 	return -ENOENT;
-- 
2.0.0

Re: [PATCH] PCI: mvebu: Fix uninitialized variable in mvebu_get_tgt_attr()

[Bjorn Helgaas]

On Wed, Sep 17, 2014 at 05:58:27PM +0200, Thomas Petazzoni wrote:
> Geert Uytterhoeven reported a warning when building pci-mvebu:
> 
> drivers/pci/host/pci-mvebu.c: In function 'mvebu_get_tgt_attr':
> drivers/pci/host/pci-mvebu.c:887:39: warning: 'rtype' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    if (slot == PCI_SLOT(devfn) && type == rtype) {
>                                        ^
> 
> And indeed, the code of mvebu_get_tgt_attr() may lead to the usage of
> rtype when being uninitialized, even though it would only happen if we
> had entries other than I/O space and 32 bits memory space.
> 
> This commit fixes that by simply skipping the current DT range being
> considered, if it doesn't match the resource type we're looking for.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch is a different solution than the one proposed by Geert. It
> (hopefully) matches the discussion we had with Bjorn and Arnd.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Applied to pci/host-mvebu for v3.18, thanks!  Oh, and I added a stable tag
for v3.12+, since mvebu_get_tgt_attr() appeared in v3.12.

> ---
>  drivers/pci/host/pci-mvebu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index a8c6f1a..b1315e1 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -873,7 +873,7 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  	rangesz = pna + na + ns;
>  	nranges = rlen / sizeof(__be32) / rangesz;
>  
> -	for (i = 0; i < nranges; i++) {
> +	for (i = 0; i < nranges; i++, range += rangesz) {
>  		u32 flags = of_read_number(range, 1);
>  		u32 slot = of_read_number(range + 1, 1);
>  		u64 cpuaddr = of_read_number(range + na, pna);
> @@ -883,14 +883,14 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  			rtype = IORESOURCE_IO;
>  		else if (DT_FLAGS_TO_TYPE(flags) == DT_TYPE_MEM32)
>  			rtype = IORESOURCE_MEM;
> +		else
> +			continue;
>  
>  		if (slot == PCI_SLOT(devfn) && type == rtype) {
>  			*tgt = DT_CPUADDR_TO_TARGET(cpuaddr);
>  			*attr = DT_CPUADDR_TO_ATTR(cpuaddr);
>  			return 0;
>  		}
> -
> -		range += rangesz;
>  	}
>  
>  	return -ENOENT;
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html