[[email protected]: New Defects reported by Coverity Scan for Linux]

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 [email protected] -----

Date: Thu, 23 Jan 2014 04:23:21 -0800
From: [email protected]
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 -----

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

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.

index 7a0fec6ce571..39dff3fe57af 100644
@@ -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;

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:

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

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

greg k-h

From: Greg Kroah-Hartman <[email protected]>

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

---
 drivers/pci/msi.c | 1 +
 1 file changed, 1 insertion(+)

index 39dff3fe57af..6f0474ebe420 100644
@@ -595,6 +595,7 @@ error_attrs:
 		++count;
 		msi_attr = msi_attrs[count];
 	}
+	kfree(msi_attrs);
 	return ret;
 }
 

1.8.5.1.163.gd7aced9

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

Applied to my pci/msi branch, thanks!

Applied to my pci/msi branch, thanks!