linux-kernel - [patch -stable] firware_class oops: fix firmware_loading

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 21 Sep 2009 13:32:23 +0000
From:	Frederik Deweerdt <frederik.deweerdt@...og.eu>
To:	greg@...ah.org, torvalds@...ux-foundation.org,
	Lars Ericsson <Lars_Ericsson@...ia.com>
Cc:	'Frederik Deweerdt' <frederik.deweerdt@...og.eu>,
	David.Woodhouse@...el.com, sachinp@...ibm.com,
	linux-kernel@...r.kernel.org, 'Ivo van Doorn' <ivdoorn@...il.com>
Subject: [patch -stable] firware_class oops: fix firmware_loading_store
	locking

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@...ia.com>
Signed-off-by: Frederik Deweerdt <frederik.deweerdt@...og.eu>
Tested-by: Lars Ericsson <Lars_Ericsson@...ia.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 */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/