[patch -stable] firware_class oops: fix firmware_loading_store locking

[Frederik Deweerdt <frederik.deweerdt@xprog.eu>]

Hi all,
On Fri, Sep 18, 2009 at 07:53:40PM +0200, Lars Ericsson wrote:
> > On Wed, Sep 16, 2009 at 08:44:43PM +0200, Lars Ericsson wrote:
> > > Hi,
> > > 
> > > I have discovered a Oops in the firmware_loading_store function. 
> > > At first it looks like a timing issue but after adding a 
> > BUG_ON test,
> > > it fails every time. 
> > > 
> > > drivers\base\firmware_class:
> > > ------------------------------
> > >  541 01c0 F6463401 	testb $1,52(%esi)
> > >  542 01c4 0F843FFF 	je .L38
> > >  542      FFFF
> > >  543              	.loc 1 174 0
> > >  544 01ca 8B4630   	movl 48(%esi),%eax
> > >  545 01cd 8B4004   	movl 4(%eax),%eax	<---- Oops
> > >  546 01d0 E8FCFFFF 	call vfree
> > >  546      FF
> > > 
> > > The code that fails was introduced in commit
> > > 6e03a201bbe8137487f340d26aa662110e324b20 
> > Could you try the following patch?
> 
> No, problem.
> I have tried the patch and it works for me. I will continue my 
> tests and report any problems. 
> 
> Should I close the http://bugzilla.kernel.org/show_bug.cgi?id=14185 ,
> which I have created before I noticed you reply ?
> 
I'd rather wait someone picks it up for mainline inclusion. I've added
your {Reported,Tested}-by tags.

The bug its vanilla 2.6.31, and should be considered for -stable inclusion.

Regards,
Frederik

----

The code introduced by commit 6e03a201bbe8137487f340d26aa662110e324b20 leads
to a potential null deref. The following patch adds the proper locking
around the accesses to fw_priv->fw.
See http://bugzilla.kernel.org/show_bug.cgi?id=14185 for a full bug report.


Reported-by: Lars Ericsson <Lars_Ericsson@telia.com>
Signed-off-by: Frederik Deweerdt <frederik.deweerdt@xprog.eu>
Tested-by: Lars Ericsson <Lars_Ericsson@telia.com>


diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8a267c4..49105ed 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -171,12 +171,18 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
+			mutex_lock(&fw_lock);
+			if (!fw_priv->fw) {
+				mutex_unlock(&fw_lock);
+				break;
+			}
 			vfree(fw_priv->fw->data);
 			fw_priv->fw->data = vmap(fw_priv->pages,
 						 fw_priv->nr_pages,
 						 0, PAGE_KERNEL_RO);
 			if (!fw_priv->fw->data) {
 				dev_err(dev, "%s: vmap() failed\n", __func__);
+				mutex_unlock(&fw_lock);
 				goto err;
 			}
 			/* Pages will be freed by vfree() */
@@ -185,6 +191,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
 			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
+			mutex_unlock(&fw_lock);
 			break;
 		}
 		/* fallthrough */