Skip to contentSkip to conversation

vfio: make an array larger

PATCH Superseded Dan Carpenter 4 participants 9 messages Nov 4, 2015 – Nov 10, 2015

Subsystems (3)
KVM Kernel janitors VFIO driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 vfio: make an array larger

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

Fix this by making the array larger.

Signed-off-by Dan Carpenter <[email protected]>
drivers/vfio/pci/vfio_pci_config.c +1 -1
index ff75ca3..001d48a 100644
@@ -46,7 +46,7 @@
  *   0: Removed from the user visible capability list
  *   FF: Variable length
  */
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,
#1 lore raw

drivers/vfio/pci/vfio_pci_config.c

Quoted 10 lines · Dan Carpenter

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

Fix this by making the array larger.

Signed-off-by Dan Carpenter <[email protected]>
drivers/vfio/pci/vfio_pci_config.c +0 -0

[]

drivers/vfio/pci/vfio_pci_config.c line 52

Quoted 9 lines
drivers/vfio/pci/vfio_pci_config.c +1 -1
@@ -46,7 +46,7 @@
  *   0: Removed from the user visible capability list
  *   FF: Variable length
  */
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

Doesn't the same thing happen with pci_ext_cap_length? Both array declarations might be better as const.

author
#2 lore raw
Quoted 1 line · Joe Perches

Doesn't the same thing happen with pci_ext_cap_length?

pci_ext_cap_length is fine as-is but you're right that we probably should make the size explicit as well. I will fix and resend.

Quoted 1 line

Both array declarations might be better as const.

Sure. I will do this as well.

regards,
dan carpenter

#3 lore raw
In reply to Dan Carpenter
Quoted 14 lines · Dan Carpenter

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

Fix this by making the array larger.

Signed-off-by Dan Carpenter <[email protected]>
drivers/vfio/pci/vfio_pci_config.c +0 -0
index ff75ca3..001d48a 100644

drivers/vfio/pci/vfio_pci_config.c line 52

Quoted 8 lines
drivers/vfio/pci/vfio_pci_config.c +1 -1
@@ -46,7 +46,7 @@
  *   0: Removed from the user visible capability list
  *   FF: Variable length
  */
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

This doesn't make a whole lot of sense to me. The last entry we define is:

[PCI_CAP_ID_AF] = PCI_CAP_AF_SIZEOF,
};

and PCI_CAP_ID_MAX is defined as:

#define PCI_CAP_ID_MAX PCI_CAP_ID_AF

So the array is implicitly sized to PCI_CAP_ID_MAX + 1 already, this doesn't make it any larger. I imagine this silences smatch because it's hitting this:

                if (cap <= PCI_CAP_ID_MAX) {
                        len = pci_cap_length[cap];

And it doesn't like that we're indexing an array that has entries up to PCI_CAP_ID_AF and we're testing against PCI_CAP_ID_MAX. They happen to be the same now, but that could change and then we'd index off the end of the array. That's unlikely, but valid. Is that the real justification for this patch? Thanks,

Alex

author
#4 lore raw

Sorry, I should have said that I am on linux-next at the start.

Quoted 10 lines

drivers/vfio/pci/vfio_pci_config.c line 52

Quoted 5 lines
drivers/vfio/pci/vfio_pci_config.c +1 -1
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

This doesn't make a whole lot of sense to me. The last entry we define is:

[PCI_CAP_ID_AF] = PCI_CAP_AF_SIZEOF,

Yes.

Quoted 5 lines

};

and PCI_CAP_ID_MAX is defined as:

#define PCI_CAP_ID_MAX PCI_CAP_ID_AF

No. I am on linux-next and we appear to have added a new element beyond PCI_CAP_ID_AF.

#define  PCI_CAP_ID_AF          0x13    /* PCI Advanced Features */
#define  PCI_CAP_ID_EA          0x14    /* PCI Enhanced Allocation */
#define  PCI_CAP_ID_MAX         PCI_CAP_ID_EA
Quoted 3 lines

So the array is implicitly sized to PCI_CAP_ID_MAX + 1 already, this doesn't make it any larger.

In linux-next it makes it larger. But also explicitly using PCI_CAP_ID_MAX + 1 is cleaner as well as fixing the bug in case we add more elements later again.

regards,
dan carpenter

#5 lore raw
Quoted 29 lines · Dan Carpenter

Sorry, I should have said that I am on linux-next at the start.

Quoted 10 lines

drivers/vfio/pci/vfio_pci_config.c line 52

Quoted 5 lines
drivers/vfio/pci/vfio_pci_config.c +1 -1
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

This doesn't make a whole lot of sense to me. The last entry we define is:

[PCI_CAP_ID_AF] = PCI_CAP_AF_SIZEOF,

Yes.

Quoted 5 lines

};

and PCI_CAP_ID_MAX is defined as:

#define PCI_CAP_ID_MAX PCI_CAP_ID_AF

No. I am on linux-next and we appear to have added a new element beyond PCI_CAP_ID_AF.

#define  PCI_CAP_ID_AF          0x13    /* PCI Advanced Features */
#define  PCI_CAP_ID_EA          0x14    /* PCI Enhanced Allocation */
#define  PCI_CAP_ID_MAX         PCI_CAP_ID_EA
Quoted 1 line
Quoted 6 lines
Quoted 2 lines

So the array is implicitly sized to PCI_CAP_ID_MAX + 1 already, this doesn't make it any larger.

In linux-next it makes it larger. But also explicitly using PCI_CAP_ID_MAX + 1 is cleaner as well as fixing the bug in case we add more elements later again.

Ok, all the pieces line up now. Please add mention of that to the commit log and I'll look for the respin including the same for pci_ext_cap_length. Thanks for spotting this!

Alex

author
#6 lore raw

PATCH v2 1/1 vfio/pci: make an array larger

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

The problem is that pci_cap_length[] was defined as large enough to hold "PCI_CAP_ID_AF + 1" elements. The code in vfio_cap_init() assumes it has PCI_CAP_ID_MAX + 1 elements. Originally, PCI_CAP_ID_AF and PCI_CAP_ID_MAX were the same but then we introduced PCI_CAP_ID_EA in f80b0ba95964 ('PCI: Add Enhanced Allocation register entries') so now the array is too small.

Let's fix this by making the array size PCI_CAP_ID_MAX + 1. And let's make a similar change to pci_ext_cap_length[] for consistency. Also both these arrays can be made const.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: more cleanups

drivers/vfio/pci/vfio_pci_config.c +2 -2
index a8657ef..fe2b470 100644
@@ -46,7 +46,7 @@
  *   0: Removed from the user visible capability list
  *   FF: Variable length
  */
-static u8 pci_cap_length[] = {
+static const u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

@@ -74,7 +74,7 @@ static u8 pci_cap_length[] = {
  *   0: Removed or masked from the user visible capabilty list
  *   FF: Variable length
  */
-static u16 pci_ext_cap_length[] = {
+static const u16 pci_ext_cap_length[PCI_EXT_CAP_ID_MAX + 1] = {
 	[PCI_EXT_CAP_ID_ERR]	=	PCI_ERR_ROOT_COMMAND,
 	[PCI_EXT_CAP_ID_VC]	=	0xFF,
 	[PCI_EXT_CAP_ID_DSN]	=	PCI_EXT_CAP_DSN_SIZEOF,
Applied
#7 lore raw

Re: [patch v2] vfio/pci: make an array larger

Quoted 12 lines · Dan Carpenter

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

The problem is that pci_cap_length[] was defined as large enough to hold "PCI_CAP_ID_AF + 1" elements. The code in vfio_cap_init() assumes it has PCI_CAP_ID_MAX + 1 elements. Originally, PCI_CAP_ID_AF and PCI_CAP_ID_MAX were the same but then we introduced PCI_CAP_ID_EA in f80b0ba95964 ('PCI: Add Enhanced Allocation register entries') so now the array is too small.

Quoted 6 lines

Let's fix this by making the array size PCI_CAP_ID_MAX + 1. And let's make a similar change to pci_ext_cap_length[] for consistency. Also both these arrays can be made const.

Signed-off-by Dan Carpenter <[email protected]>

---

Applied to next for v4.4. Thanks!

Alex

#8 lore raw
In reply to Dan Carpenter

Am 04.11.2015 14:26, schrieb Dan Carpenter:

Quoted 14 lines

Smatch complains about a possible out of bounds error:

	drivers/vfio/pci/vfio_pci_config.c:1241 vfio_cap_init()
	error: buffer overflow 'pci_cap_length' 20 <= 20

Fix this by making the array larger.

Signed-off-by Dan Carpenter <[email protected]>
drivers/vfio/pci/vfio_pci_config.c +0 -0
index ff75ca3..001d48a 100644

drivers/vfio/pci/vfio_pci_config.c line 52

Quoted 8 lines
drivers/vfio/pci/vfio_pci_config.c +1 -1
@@ -46,7 +46,7 @@
  *   0: Removed from the user visible capability list
  *   FF: Variable length
  */
-static u8 pci_cap_length[] = {
+static u8 pci_cap_length[PCI_CAP_ID_MAX + 1] = {
 	[PCI_CAP_ID_BASIC]	= PCI_STD_HEADER_SIZEOF, /* pci config header */
 	[PCI_CAP_ID_PM]		= PCI_PM_SIZEOF,
 	[PCI_CAP_ID_AGP]	= PCI_AGP_SIZEOF,

(i am sorry Dave)

I am not sure if that is the way to go.
this define make me feel uneasy,
#define PCI_CAP_ID_MAX PCI_CAP_ID_AF

Would it be possible to ARRAY_SIZE(pci_cap_length) instead of PCI_CAP_ID_MAX ? Then that would grow automatically with the array. And its more clear what is actually happening.

re,
wh

Quoted 1 line