LinuxLists.cc - [patch] vfio: make an array larger

2015-11-04 13:26:40

Subject: [patch] 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]>

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index ff75ca3..001d48a 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -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,

2015-11-04 16:40:24

by Joe Perches

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

2015-11-04 16:54:42

by Alex Williamson

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

On Wed, 2015-11-04 at 16:26 +0300, Dan Carpenter wrote:
> 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]>
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index ff75ca3..001d48a 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -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

2015-11-04 18:20:36

by Dan Carpenter

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

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

> > -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.

> };
>
> 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

>
> 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

2015-11-04 18:23:57

by Dan Carpenter

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

On Wed, Nov 04, 2015 at 08:40:19AM -0800, Joe Perches wrote:
> 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.

> Both array declarations might be better as const.

Sure. I will do this as well.

regards,
dan carpenter

2015-11-04 18:29:08

by Alex Williamson

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

On Wed, 2015-11-04 at 21:20 +0300, Dan Carpenter wrote:
> Sorry, I should have said that I am on linux-next at the start.
>
> > > -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.
>
> > };
> >
> > 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
>
> >
> > 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

2015-11-04 21:49:14

by walter harms

[permalink] [raw]

Subject: Re: [patch] vfio: make an array larger

Am 04.11.2015 14:26, schrieb 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]>
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index ff75ca3..001d48a 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -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

>

2015-11-09 12:25:13

by Dan Carpenter

[permalink] [raw]

Subject: [patch v2] 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

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index a8657ef..fe2b470 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -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,

2015-11-10 19:03:55

by Alex Williamson

[permalink] [raw]

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

On Mon, 2015-11-09 at 15:24 +0300, Dan Carpenter wrote:
> 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]>
> ---

Applied to next for v4.4. Thanks!

Alex