Skip to contentSkip to conversation

PCI: dwc: uninitialized variable in dw_handle_msi_irq()

PATCH Applied Dan Carpenter 4 participants 8 messages Feb 17, 2017 – Mar 17, 2017

Subsystems (2)
Kernel janitors PCI
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 PCI: dwc: uninitialized variable in dw_handle_msi_irq()

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

drivers/pci/dwc/pcie-designware.c +4 -3
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,
#1 lore raw

Am 18.02.2017 00:26, schrieb Dan Carpenter:

Quoted 17 lines

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

drivers/pci/dwc/pcie-designware.c +0 -0
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {

drivers/pci/dwc/pcie-designware.c static struct irq_chip dw_msi_irq_chip = { · line 267

Quoted 10 lines
drivers/pci/dwc/pcie-designware.c +2 -2
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);
+				    &val);
 		if (val) {

why not

	if (!val) continue;

it would save an entire indent level and make things a bit more easy to read.

drivers/pci/dwc/pcie-designware.c static struct irq_chip dw_msi_irq_chip = { · line 273

Quoted 7 lines
drivers/pci/dwc/pcie-designware.c +2 -1
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);

irq seems to be 0 when nothing is found. This can never happen ?

find_next_bit() feels a bit overpowered perhaps a simple loop would be more effective and more easy to understand ? something like:

	while ( val) {
	 if (val & 1 )
		found ...
	val>>=1;
	pos++;
	}

just my 2 cents,

re,
wh

Quoted 1 line
drivers/pci/dwc/pcie-designware.c +0 -0
 				dw_pcie_wr_own_conf(pp,
#2 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

Jingoo, Joao?

Quoted 29 lines

---
Static analysis. Not tested.

drivers/pci/dwc/pcie-designware.c +4 -3
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,
#3 lore raw
In reply to Dan Carpenter

Hi Dan,

Às 3:26 PM de 2/17/2017, Dan Carpenter escreveu:

Quoted 8 lines

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

What you are statiting makes perfect sense, since the register is indeed 32 bits and can have undesirable behavior in 64-bit systems for example.
We have more examples like this for MSI related operations in pcie-designware. Could you please change them as well just?

For example, the irq variable declaration is also not consistent as you can see in these examples:

static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)

static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
irq_hw_number_t hwirq)

static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)

static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)

etc.

Thanks
Joao

Quoted 18 lines
drivers/pci/dwc/pcie-designware.c +1 -2
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);

drivers/pci/dwc/pcie-designware.c static struct irq_chip dw_msi_irq_chip = { · line 274

Quoted 10 lines
drivers/pci/dwc/pcie-designware.c +3 -1
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 20 lines · Joao Pinto

Hi Dan,

Às 3:26 PM de 2/17/2017, Dan Carpenter escreveu:

Quoted 8 lines

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

What you are statiting makes perfect sense, since the register is indeed 32 bits and can have undesirable behavior in 64-bit systems for example.
We have more examples like this for MSI related operations in pcie-designware. Could you please change them as well just?

For example, the irq variable declaration is also not consistent as you can see in these examples:

Quoted 8 lines

static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)

static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
irq_hw_number_t hwirq)

static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)

static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)

Where are we with this? It sounds like there's a real problem here, and Dan's original patch fixes one case of it. But if there are other similar cases, we should fix them all at once.

Since this doesn't sound like an urgent bug fix (I don't see user problem reports), I guess I'll wait for an updated patch?

Quoted 13 lines
Quoted 13 lines
drivers/pci/dwc/pcie-designware.c +1 -1
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
Quoted 15 lines
Quoted 4 lines
drivers/pci/dwc/pcie-designware.c +0 -1
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);

drivers/pci/dwc/pcie-designware.c static struct irq_chip dw_msi_irq_chip = { · line 274

Quoted 10 lines
drivers/pci/dwc/pcie-designware.c +3 -1
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw
Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Joao Pinto

Hi Dan,

Às 3:26 PM de 2/17/2017, Dan Carpenter escreveu:

Quoted 8 lines

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

What you are statiting makes perfect sense, since the register is indeed 32 bits and can have undesirable behavior in 64-bit systems for example.
We have more examples like this for MSI related operations in pcie-designware. Could you please change them as well just?

For example, the irq variable declaration is also not consistent as you can see in these examples:

static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)

static int dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
irq_hw_number_t hwirq)

static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)

Quoted 8 lines
Quoted 1 line

static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)

Where are we with this? It sounds like there's a real problem here, and Dan's original patch fixes one case of it. But if there are other similar cases, we should fix them all at once.

Since this doesn't sound like an urgent bug fix (I don't see user problem reports), I guess I'll wait for an updated patch?

Oh... Hm. I misread. I thought that Joao was going to send a patch.

Looking at it more closely now, I think my patch is sufficient. Perhaps I have misunderstood something but I don't see any other bugs here beyond the one I fixed.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Applied
#6 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

I applied this to pci/host-designware for v4.12.

I also applied the patch below based on walter's suggestion:

commit b67d3c69df8d6721f87bbc22a587914e0d4944a7

Author: Bjorn Helgaas <[email protected]>
Date:   Thu Mar 16 14:34:59 2017 -0500

PCI: dwc: Unindent dw_handle_msi_irq() loop

Use "continue" to skip rest of the loop when possible to save an indent level. No functional change intended.

Suggested-by walter harms <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pci/dwc/pcie-designware-host.c +11 -11
index 5ba334938b52..6cdd41f06dea 100644
@@ -63,17 +63,17 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
 				    (u32 *)&val);
-		if (val) {
-			ret = IRQ_HANDLED;
-			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
-				irq = irq_find_mapping(pp->irq_domain,
-						       i * 32 + pos);
-				dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS +
-						    i * 12, 4, 1 << pos);
-				generic_handle_irq(irq);
-				pos++;
-			}
+		if (!val)
+			continue;
+
+		ret = IRQ_HANDLED;
+		pos = 0;
+		while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			irq = irq_find_mapping(pp->irq_domain, i * 32 + pos);
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12,
+					    4, 1 << pos);
+			generic_handle_irq(irq);
+			pos++;
 		}
 	}
Quoted 29 lines

---
Static analysis. Not tested.

drivers/pci/dwc/pcie-designware.c +4 -3
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,
Acked-by
#7 lore raw

It is my feeling that this realy improves the readability.

Acked-by [email protected]

Am 16.03.2017 20:44, schrieb Bjorn Helgaas:

Quoted 93 lines · Dan Carpenter
Quoted 6 lines

The bug is that "val" is unsigned long but we only initialize 32 bits of it. Then we test "if (val)" and that might be true not because we set the bits but because some were never initialized.

Fixes f342d940ee0e ("PCI: exynos: Add support for MSI")
Signed-off-by Dan Carpenter <[email protected]>

I applied this to pci/host-designware for v4.12.

I also applied the patch below based on walter's suggestion:

commit b67d3c69df8d6721f87bbc22a587914e0d4944a7

Author: Bjorn Helgaas <[email protected]>
Date:   Thu Mar 16 14:34:59 2017 -0500

PCI: dwc: Unindent dw_handle_msi_irq() loop

Use "continue" to skip rest of the loop when possible to save an indent level. No functional change intended.

Suggested-by walter harms <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pci/dwc/pcie-designware-host.c +11 -11
index 5ba334938b52..6cdd41f06dea 100644
@@ -63,17 +63,17 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
 				    (u32 *)&val);
-		if (val) {
-			ret = IRQ_HANDLED;
-			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
-				irq = irq_find_mapping(pp->irq_domain,
-						       i * 32 + pos);
-				dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS +
-						    i * 12, 4, 1 << pos);
-				generic_handle_irq(irq);
-				pos++;
-			}
+		if (!val)
+			continue;
+
+		ret = IRQ_HANDLED;
+		pos = 0;
+		while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			irq = irq_find_mapping(pp->irq_domain, i * 32 + pos);
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12,
+					    4, 1 << pos);
+			generic_handle_irq(irq);
+			pos++;
 		}
 	}
Quoted 20 lines

---
Static analysis. Not tested.

drivers/pci/dwc/pcie-designware.c +1 -2
index af8f6e92e885..5bfc377b83e4 100644
@@ -257,17 +257,18 @@ static struct irq_chip dw_msi_irq_chip = {
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	unsigned long val;
+	u32 val;
 	int i, pos, irq;
 	irqreturn_t ret = IRQ_NONE;
 
 	for (i = 0; i < MAX_MSI_CTRLS; i++) {
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				(u32 *)&val);

drivers/pci/dwc/pcie-designware.c static struct irq_chip dw_msi_irq_chip = { · line 274

Quoted 10 lines
drivers/pci/dwc/pcie-designware.c +3 -1
+				    &val);
 		if (val) {
 			ret = IRQ_HANDLED;
 			pos = 0;
-			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
+			while ((pos = find_next_bit((unsigned long *)&val, 32,
+						    pos)) != 32) {
 				irq = irq_find_mapping(pp->irq_domain,
 						i * 32 + pos);
 				dw_pcie_wr_own_conf(pp,

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html