[patch] vfio: make an array larger

[patch] vfio: make an array larger

[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 <dan.carpenter@oracle.com>

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,

Re: [patch] vfio: make an array larger

[Joe Perches]

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 <dan.carpenter@oracle.com>
> 
> diff --git 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,

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

Re: [patch] vfio: make an array larger

[Dan Carpenter]

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

Re: [patch] vfio: make an array larger

[Alex Williamson]

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 <dan.carpenter@oracle.com>
> 
> 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

Re: [patch] vfio: make an array larger

[Dan Carpenter]

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

Re: [patch] vfio: make an array larger

[Alex Williamson]

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

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

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

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 <dan.carpenter@oracle.com>
---
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,

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

[Alex Williamson]

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 <dan.carpenter@oracle.com>
> ---

Applied to next for v4.4.  Thanks!

Alex

Re: [patch] vfio: make an array larger

[walter harms]

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 <dan.carpenter@oracle.com>
> 
> 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



>