PCI: fix a potential uninitentional integer overflow issue

PATCH Superseded Colin King 3 participants 6 messages Oct 7, 2020 – Nov 10, 2020

Subsystems (2)

Kernel janitors PCI

lore

Share & tools

lore

Colin King

author

2020-10-07 11:46 UTC #0 lore raw

PATCH 1/1 PCI: fix a potential uninitentional integer overflow issue

From: Colin Ian King <[email protected]>

The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.

Addresses-Coverity: ("Uninitentional integer overflow")

Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")

Signed-off-by Colin Ian King <[email protected]>

---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/pci.c +1 -1

index 6d4d5a2f923d..1a5844d7af35 100644
@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);
 			break;
 		} else if (ret < 0) {
 			pr_err("PCI: Can't parse resource_alignment parameter: %s\n",

Signature · 1 line

2.27.0

Dan Carpenter

2020-10-07 12:33 UTC #1 lore raw

Quoted 20 lines · Colin King

From: Colin Ian King <[email protected]>
The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.
Addresses-Coverity: ("Uninitentional integer overflow")
Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")
Signed-off-by Colin Ian King <[email protected]>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pci.c +0 -0
index 6d4d5a2f923d..1a5844d7af35 100644

drivers/pci/pci.c static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, · line 6212

Quoted 5 lines

drivers/pci/pci.c +1 -1

@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);

"align_order" comes from sscanf() so Smatch thinks it's not trusted. Anything above 63 is undefined behavior. There should be a bounds check on this but I don't know what the valid values of "align" are.

regards,
dan carpenter

Bjorn Helgaas

2020-11-05 22:24 UTC #2 lore raw

Quoted 10 lines · Dan Carpenter

Quoted 9 lines · Colin King
From: Colin Ian King <[email protected]>
The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.
Addresses-Coverity: ("Uninitentional integer overflow")

s/Uninitentional/Unintentional/
Also in subject (please also capitalize subject)

Doesn't Coverity also assign an ID number for this specific issue? Can you include that as well, e.g.,

Addresses-Coverity-ID: 1226899 ("Unintentional integer overflow")

Quoted 6 lines

Quoted 6 lines
Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")
Signed-off-by Colin Ian King <[email protected]>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines

drivers/pci/pci.c static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, · line 6212
Quoted 10 lines
drivers/pci/pci.c +1 -1
index 6d4d5a2f923d..1a5844d7af35 100644
@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);
"align_order" comes from sscanf() so Smatch thinks it's not trusted. Anything above 63 is undefined behavior. There should be a bounds check on this but I don't know what the valid values of "align" are.

The spec doesn't explicitly say what the size limit for 64-bit BARs is, but it does say 32-bit BARs can support up to 2GB (2^31). So I infer that 2^63 would be the limit for 64-bit BARs.

What about something like the following? To me, BIT_ULL doesn't seem like an advantage over "1ULL << ", but maybe there's a reason to use it.

drivers/pci/pci.c +8 -6

index 8b9bea8ba751..6e17d0a6828a 100644
@@ -6197,19 +6197,21 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	while (*p) {
 		count = 0;
 		if (sscanf(p, "%d%n", &align_order, &count) = 1 &&
-							p[count] = '@') {
+		    p[count] = '@') {
 			p += count + 1;
+			if (align_order > 63) {
+				pr_err("PCI: Invalid requested alignment (order %d)\n",
+				       align_order);
+				align_order = PAGE_SHIFT;
+			}
 		} else {
-			align_order = -1;
+			align_order = PAGE_SHIFT;
 		}
 
 		ret = pci_dev_str_match(dev, p, &p);
 		if (ret = 1) {
 			*resize = true;
-			if (align_order = -1)
-				align = PAGE_SIZE;
-			else
-				align = 1 << align_order;
+			align = 1ULL << align_order;
 			break;
 		} else if (ret < 0) {
 			pr_err("PCI: Can't parse resource_alignment parameter: %s\n",

Dan Carpenter

2020-11-06 08:04 UTC #3 lore raw

Quoted 41 lines · Bjorn Helgaas

Quoted 10 lines · Dan Carpenter
Quoted 9 lines · Colin King
From: Colin Ian King <[email protected]>
The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.
Addresses-Coverity: ("Uninitentional integer overflow")
s/Uninitentional/Unintentional/
Also in subject (please also capitalize subject)
Doesn't Coverity also assign an ID number for this specific issue? Can you include that as well, e.g.,
Addresses-Coverity-ID: 1226899 ("Unintentional integer overflow")
Quoted 6 lines
Quoted 6 lines
Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")
Signed-off-by Colin Ian King <[email protected]>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/pci/pci.c static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, · line 6212
Quoted 10 lines
drivers/pci/pci.c +1 -1
index 6d4d5a2f923d..1a5844d7af35 100644
@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);
"align_order" comes from sscanf() so Smatch thinks it's not trusted. Anything above 63 is undefined behavior. There should be a bounds check on this but I don't know what the valid values of "align" are.

Quoted 7 lines

The spec doesn't explicitly say what the size limit for 64-bit BARs is, but it does say 32-bit BARs can support up to 2GB (2^31). So I infer that 2^63 would be the limit for 64-bit BARs.
What about something like the following? To me, BIT_ULL doesn't seem like an advantage over "1ULL << ", but maybe there's a reason to use it.

The advantage of BIT_ULL() is that checkpatch and I think Coccinelle will suggest using it. It's only recently where a few people have complained (actually you're probably the second person) that BIT() is sort of a weird thing to use for size variables.

regards,
dan carpenter

Bjorn Helgaas

2020-11-10 20:54 UTC #4 lore raw

Quoted 46 lines · Dan Carpenter

Quoted 45 lines · Bjorn Helgaas
Quoted 10 lines · Dan Carpenter
Quoted 9 lines · Colin King
From: Colin Ian King <[email protected]>
The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.
Addresses-Coverity: ("Uninitentional integer overflow")
s/Uninitentional/Unintentional/
Also in subject (please also capitalize subject)
Doesn't Coverity also assign an ID number for this specific issue? Can you include that as well, e.g.,
Addresses-Coverity-ID: 1226899 ("Unintentional integer overflow")
Quoted 6 lines
Quoted 6 lines
Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")
Signed-off-by Colin Ian King <[email protected]>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/pci/pci.c static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, · line 6212
Quoted 10 lines
drivers/pci/pci.c +1 -1
index 6d4d5a2f923d..1a5844d7af35 100644
@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);
"align_order" comes from sscanf() so Smatch thinks it's not trusted. Anything above 63 is undefined behavior. There should be a bounds check on this but I don't know what the valid values of "align" are.
The spec doesn't explicitly say what the size limit for 64-bit BARs is, but it does say 32-bit BARs can support up to 2GB (2^31). So I infer that 2^63 would be the limit for 64-bit BARs.

Quoted 8 lines

Quoted 3 lines
What about something like the following? To me, BIT_ULL doesn't seem like an advantage over "1ULL << ", but maybe there's a reason to use it.
The advantage of BIT_ULL() is that checkpatch and I think Coccinelle will suggest using it. It's only recently where a few people have complained (actually you're probably the second person) that BIT() is sort of a weird thing to use for size variables.

If that's the only reason, I definitely prefer "1ULL << align_order".

BIT_ULL is just a pointless abstraction in this case.

Colin Ian King

author

2020-11-10 22:00 UTC #5 lore raw

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Bjorn Helgaas
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Colin King
From: Colin Ian King <[email protected]>
The shift of 1 by align_order is evaluated using 32 bit arithmetic and the result is assigned to a resource_size_t type variable that is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow before widening issue by using the BIT_ULL macro to perform the shift.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Addresses-Coverity: ("Uninitentional integer overflow")
s/Uninitentional/Unintentional/
Also in subject (please also capitalize subject)

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Doesn't Coverity also assign an ID number for this specific issue? Can you include that as well, e.g.,

I'm running this from an internal coverity scan, so the ID is not public.

Quoted 32 lines

Quoted 32 lines
Quoted 31 lines
Addresses-Coverity-ID: 1226899 ("Unintentional integer overflow")
Quoted 6 lines
Quoted 6 lines
Fixes 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters reusable")
Signed-off-by Colin Ian King <[email protected]>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/pci/pci.c static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev, · line 6212
Quoted 10 lines
drivers/pci/pci.c +1 -1
index 6d4d5a2f923d..1a5844d7af35 100644
@@ -6209,7 +6209,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			if (align_order = -1)
 				align = PAGE_SIZE;
 			else
-				align = 1 << align_order;
+				align = BIT_ULL(align_order);
"align_order" comes from sscanf() so Smatch thinks it's not trusted. Anything above 63 is undefined behavior. There should be a bounds check on this but I don't know what the valid values of "align" are.
The spec doesn't explicitly say what the size limit for 64-bit BARs is, but it does say 32-bit BARs can support up to 2GB (2^31). So I infer that 2^63 would be the limit for 64-bit BARs.
What about something like the following? To me, BIT_ULL doesn't seem like an advantage over "1ULL << ", but maybe there's a reason to use it.

Quoted 9 lines

Quoted 4 lines
The advantage of BIT_ULL() is that checkpatch and I think Coccinelle will suggest using it. It's only recently where a few people have complained (actually you're probably the second person) that BIT() is sort of a weird thing to use for size variables.
If that's the only reason, I definitely prefer "1ULL << align_order".
BIT_ULL is just a pointless abstraction in this case.

OK. V2 Arriving later today

Colin