diff mbox

vfio: make an array larger

Message ID 20151104132624.GC20966@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Nov. 4, 2015, 1:26 p.m. UTC 
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>

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

Comments

Joe Perches Nov. 4, 2015, 4:40 p.m. UTC | #1 
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.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson Nov. 4, 2015, 4:54 p.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Nov. 4, 2015, 6:20 p.m. UTC | #3 
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Nov. 4, 2015, 6:23 p.m. UTC | #4 
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson Nov. 4, 2015, 6:28 p.m. UTC | #5 
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Walter Harms Nov. 4, 2015, 9:39 p.m. UTC | #6 
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



> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,