Messages in this thread Patch in this message | | | Date | Wed, 23 Sep 2009 09:42:41 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [patch -stable] firware_class oops: fix firmware_loading_store locking |
| |
On Mon, 21 Sep 2009, Frederik Deweerdt wrote: < > 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.
I don't think this is correct.
I think you should protect the FW_STATUS_LOADING bit too, shouldn't you?
As it is, it does this:
if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { mutex_lock(&fw_lock); ... clear_bit(FW_STATUS_LOADING, &fw_priv->status); mutex_unlock(&fw_lock); break; }
and if this code can race (which it obviously can, since your addition of fw_lock mutex matters), then I think it can race on that FW_STATUS_LOADING bit too. No?
So my gut feel is that the whole damn function should be protected by the mutex_lock thing. IOW, the patch would be something like the appended.
UNTESTED. Somebody needs to test this, verify, and send it back to me.
Am I missing something?
Linus
--- drivers/base/firmware_class.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 7376367..1b803df 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -150,13 +150,12 @@ static ssize_t firmware_loading_store(struct device *dev, int loading = simple_strtol(buf, NULL, 10); int i; + mutex_lock(&fw_lock); switch (loading) { case 1: - mutex_lock(&fw_lock); - if (!fw_priv->fw) { - mutex_unlock(&fw_lock); + if (!fw_priv->fw) break; - } + vfree(fw_priv->fw->data); fw_priv->fw->data = NULL; for (i = 0; i < fw_priv->nr_pages; i++) @@ -167,7 +166,6 @@ static ssize_t firmware_loading_store(struct device *dev, fw_priv->nr_pages = 0; fw_priv->fw->size = 0; set_bit(FW_STATUS_LOADING, &fw_priv->status); - mutex_unlock(&fw_lock); break; case 0: if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { @@ -195,6 +193,7 @@ static ssize_t firmware_loading_store(struct device *dev, fw_load_abort(fw_priv); break; } + mutex_unlock(&fw_lock); return count; }
| |