[scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[Bjorn Helgaas]

FYI.  I think the first two (related to "name") are valid.  I haven't
figured out the "msi_attrs" one yet.

I think these are related to 1c51b50c2995 ("PCI/MSI: Export MSI mode using
attributes, not kobjects").

Bjorn

----- Forwarded message from scan-admin@coverity.com -----

Date: Thu, 23 Jan 2014 04:23:21 -0800
From: scan-admin@coverity.com
Subject: New Defects reported by Coverity Scan for Linux

Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.

...

** CID 1163315:  Dereference null return value  (NULL_RETURNS)
/drivers/pci/msi.c: 551 in populate_msi_sysfs()

** CID 1163316:  Resource leak  (RESOURCE_LEAK)
/drivers/pci/msi.c: 550 in populate_msi_sysfs()

** CID 1163317:  Resource leak  (RESOURCE_LEAK)
/drivers/pci/msi.c: 592 in populate_msi_sysfs()

...
________________________________________________________________________________________________________
*** CID 1163315:  Dereference null return value  (NULL_RETURNS)
/drivers/pci/msi.c: 551 in populate_msi_sysfs()
545     		return -ENOMEM;
546     	list_for_each_entry(entry, &pdev->msi_list, list) {
547     		char *name = kmalloc(20, GFP_KERNEL);
548     		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
549     		if (!msi_dev_attr)
550     			goto error_attrs;
>>>     CID 1163315:  Dereference null return value  (NULL_RETURNS)
>>>     Dereferencing a pointer that might be null "name" when calling "sprintf(char *, char const *, ...)".
551     		sprintf(name, "%d", entry->irq);
552     		sysfs_attr_init(&msi_dev_attr->attr);
553     		msi_dev_attr->attr.name = name;
554     		msi_dev_attr->attr.mode = S_IRUGO;
555     		msi_dev_attr->show = msi_mode_show;
556     		msi_attrs[count] = &msi_dev_attr->attr;

________________________________________________________________________________________________________
*** CID 1163316:  Resource leak  (RESOURCE_LEAK)
/drivers/pci/msi.c: 550 in populate_msi_sysfs()
544     	if (!msi_attrs)
545     		return -ENOMEM;
546     	list_for_each_entry(entry, &pdev->msi_list, list) {
547     		char *name = kmalloc(20, GFP_KERNEL);
548     		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
549     		if (!msi_dev_attr)
>>>     CID 1163316:  Resource leak  (RESOURCE_LEAK)
>>>     Variable "name" going out of scope leaks the storage it points to.
550     			goto error_attrs;
551     		sprintf(name, "%d", entry->irq);
552     		sysfs_attr_init(&msi_dev_attr->attr);
553     		msi_dev_attr->attr.name = name;
554     		msi_dev_attr->attr.mode = S_IRUGO;
555     		msi_dev_attr->show = msi_mode_show;

________________________________________________________________________________________________________
*** CID 1163317:  Resource leak  (RESOURCE_LEAK)
/drivers/pci/msi.c: 592 in populate_msi_sysfs()
586     		msi_dev_attr = container_of(msi_attr, struct device_attribute, attr);
587     		kfree(msi_attr->name);
588     		kfree(msi_dev_attr);
589     		++count;
590     		msi_attr = msi_attrs[count];
591     	}
>>>     CID 1163317:  Resource leak  (RESOURCE_LEAK)
>>>     Variable "msi_attrs" going out of scope leaks the storage it points to.
592     	return ret;
593     }
594     
595     /**
596      * msi_capability_init - configure device's MSI capability structure
597      * @dev: pointer to the pci_dev data structure of MSI device function

...

To view the defects in Coverity Scan visit, http://scan.coverity.com/projects/128?tab=Overview

----- End forwarded message -----

Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[Greg Kroah-Hartman]

On Thu, Jan 23, 2014 at 11:42:46AM -0700, Bjorn Helgaas wrote:
> FYI.  I think the first two (related to "name") are valid.  I haven't
> figured out the "msi_attrs" one yet.

Ah, missed the name kmalloc() call, I'll send you a patch now to fix
that up, and try to figure out the msi_attrs one as well.

thanks for the note, this blew by me in the results, I should have seen
it as I do get these emails as well.

greg k-h

[PATCH] PCI: msi: properly check return value of kmalloc

[Greg Kroah-Hartman]

Coverity reported that I forgot to check the return value of kmalloc
when creating the MSI attribute name, so fix that up and properly free
it if there is an error when allocating the msi_dev_attr variable.

Fixes: 1c51b50c2995 ("PCI/MSI: Export MSI mode using attributes, not kobjects")
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 7a0fec6ce571..39dff3fe57af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -545,9 +545,15 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
 		return -ENOMEM;
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		char *name = kmalloc(20, GFP_KERNEL);
+		if (!name)
+			goto error_attrs;
+
 		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
-		if (!msi_dev_attr)
+		if (!msi_dev_attr) {
+			kfree(name);
 			goto error_attrs;
+		}
+
 		sprintf(name, "%d", entry->irq);
 		sysfs_attr_init(&msi_dev_attr->attr);
 		msi_dev_attr->attr.name = name;

[PATCH] PCI: msi: properly free memory on an error path

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Coverity reported that I forgot to clean up some allocated memory on the
error path in populate_msi_sysfs(), so this patch fixes that.

Thanks to Dave Jones for pointing out where the error was, I obviously
can't read code this morning...

Fixes: 1c51b50c2995 ("PCI/MSI: Export MSI mode using attributes, not kobjects")
Cc: Dave Jones <davej@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/pci/msi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 39dff3fe57af..6f0474ebe420 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -595,6 +595,7 @@ error_attrs:
 		++count;
 		msi_attr = msi_attrs[count];
 	}
+	kfree(msi_attrs);
 	return ret;
 }
 
-- 
1.8.5.1.163.gd7aced9

Re: [PATCH] PCI: msi: properly free memory on an error path

[Dave Jones]

On Thu, Jan 23, 2014 at 12:01:12PM -0800, Greg Kroah-Hartman wrote:
 > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 > 
 > Coverity reported that I forgot to clean up some allocated memory on the
 > error path in populate_msi_sysfs(), so this patch fixes that.
 > 
 > Thanks to Dave Jones for pointing out where the error was, I obviously
 > can't read code this morning...
 
I think it doesn't help that you have 'msi_attr' and 'msi_attrs'.
My eyes kinda glazed over a few times reading those.

	Dave

Re: [PATCH] PCI: msi: properly free memory on an error path

[Bjorn Helgaas]

On Thu, Jan 23, 2014 at 12:01:12PM -0800, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Coverity reported that I forgot to clean up some allocated memory on the
> error path in populate_msi_sysfs(), so this patch fixes that.
> 
> Thanks to Dave Jones for pointing out where the error was, I obviously
> can't read code this morning...
> 
> Fixes: 1c51b50c2995 ("PCI/MSI: Export MSI mode using attributes, not kobjects")
> Cc: Dave Jones <davej@redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied to my pci/msi branch, thanks!

> ---
>  drivers/pci/msi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 39dff3fe57af..6f0474ebe420 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -595,6 +595,7 @@ error_attrs:
>  		++count;
>  		msi_attr = msi_attrs[count];
>  	}
> +	kfree(msi_attrs);
>  	return ret;
>  }
>  
> -- 
> 1.8.5.1.163.gd7aced9
> 

Re: [PATCH] PCI: msi: properly check return value of kmalloc

[Bjorn Helgaas]

On Thu, Jan 23, 2014 at 11:30:37AM -0800, Greg Kroah-Hartman wrote:
> Coverity reported that I forgot to check the return value of kmalloc
> when creating the MSI attribute name, so fix that up and properly free
> it if there is an error when allocating the msi_dev_attr variable.
> 
> Fixes: 1c51b50c2995 ("PCI/MSI: Export MSI mode using attributes, not kobjects")
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied to my pci/msi branch, thanks!

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 7a0fec6ce571..39dff3fe57af 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -545,9 +545,15 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
>  		return -ENOMEM;
>  	list_for_each_entry(entry, &pdev->msi_list, list) {
>  		char *name = kmalloc(20, GFP_KERNEL);
> +		if (!name)
> +			goto error_attrs;
> +
>  		msi_dev_attr = kzalloc(sizeof(*msi_dev_attr), GFP_KERNEL);
> -		if (!msi_dev_attr)
> +		if (!msi_dev_attr) {
> +			kfree(name);
>  			goto error_attrs;
> +		}
> +
>  		sprintf(name, "%d", entry->irq);
>  		sysfs_attr_init(&msi_dev_attr->attr);
>  		msi_dev_attr->attr.name = name;

Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[Greg Kroah-Hartman]

On Thu, Jan 23, 2014 at 11:42:46AM -0700, Bjorn Helgaas wrote:
> FYI.  I think the first two (related to "name") are valid.  I haven't
> figured out the "msi_attrs" one yet.

I've send a fix for the first two to you now, that should resolve this
issue.

But the last one, I can't figure out either.  I think Coverity doesn't
realize that we saved off the pointer and can get back to it later on,
as it's a non-trivial pointer chain involved here.

Dave, you stare at Coverity bug reports all the time, can you make any
sense out of the following report:

> Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan.
> 
> ...
> 
> 
> ** CID 1163317:  Resource leak  (RESOURCE_LEAK)
> /drivers/pci/msi.c: 592 in populate_msi_sysfs()
> 
> ...
> ________________________________________________________________________________________________________
> *** CID 1163317:  Resource leak  (RESOURCE_LEAK)
> /drivers/pci/msi.c: 592 in populate_msi_sysfs()
> 586     		msi_dev_attr = container_of(msi_attr, struct device_attribute, attr);
> 587     		kfree(msi_attr->name);
> 588     		kfree(msi_dev_attr);
> 589     		++count;
> 590     		msi_attr = msi_attrs[count];
> 591     	}
> >>>     CID 1163317:  Resource leak  (RESOURCE_LEAK)
> >>>     Variable "msi_attrs" going out of scope leaks the storage it points to.
> 592     	return ret;
> 593     }
> 594     
> 595     /**
> 596      * msi_capability_init - configure device's MSI capability structure
> 597      * @dev: pointer to the pci_dev data structure of MSI device function
> 
> ...
> 
> To view the defects in Coverity Scan visit, http://scan.coverity.com/projects/128?tab=Overview
> 
> ----- End forwarded message -----

Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[Dave Jones]

On Thu, Jan 23, 2014 at 11:34:05AM -0800, Greg Kroah-Hartman wrote:
 > On Thu, Jan 23, 2014 at 11:42:46AM -0700, Bjorn Helgaas wrote:
 > > FYI.  I think the first two (related to "name") are valid.  I haven't
 > > figured out the "msi_attrs" one yet.
 > 
 > I've send a fix for the first two to you now, that should resolve this
 > issue.
 > 
 > But the last one, I can't figure out either.  I think Coverity doesn't
 > realize that we saved off the pointer and can get back to it later on,
 > as it's a non-trivial pointer chain involved here.

This is in the error path though, where we're tearing down all that stuff
we just built up, so there is no 'later on' afaict.  If we took one of the
error_* branches, we're not storing this for anything to free later.

I've only looked at this quickly, but it looks valid to me.

it looks like we're freeing the contents of the array, but not the array itself.

	Dave

Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Linux]

[Greg Kroah-Hartman]

On Thu, Jan 23, 2014 at 02:46:31PM -0500, Dave Jones wrote:
> On Thu, Jan 23, 2014 at 11:34:05AM -0800, Greg Kroah-Hartman wrote:
>  > On Thu, Jan 23, 2014 at 11:42:46AM -0700, Bjorn Helgaas wrote:
>  > > FYI.  I think the first two (related to "name") are valid.  I haven't
>  > > figured out the "msi_attrs" one yet.
>  > 
>  > I've send a fix for the first two to you now, that should resolve this
>  > issue.
>  > 
>  > But the last one, I can't figure out either.  I think Coverity doesn't
>  > realize that we saved off the pointer and can get back to it later on,
>  > as it's a non-trivial pointer chain involved here.
> 
> This is in the error path though, where we're tearing down all that stuff
> we just built up, so there is no 'later on' afaict.  If we took one of the
> error_* branches, we're not storing this for anything to free later.
> 
> I've only looked at this quickly, but it looks valid to me.
> 
> it looks like we're freeing the contents of the array, but not the array itself.

Doh, you are right, I missed that, thanks, I'll go send a fix for this.

greg k-h