[PATCH] firmware loader: fix use-after-free by double abort

[PATCH] firmware loader: fix use-after-free by double abort

[Ming Lei]

fw_priv->buf is accessed in both request_firmware_load() and
writing to sysfs file of 'loading' context, but not protected
by 'fw_lock' entirely. The patch makes sure that access on
'fw_priv->buf' is protected by the lock.

So fixes the double abort problem reported by nirinA raseliarison:

	http://lkml.org/lkml/2013/6/14/188

Reported-and-tested-by: nirinA raseliarison <nirina.raseliarison@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b1f926..01e2103 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -450,8 +450,18 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 {
 	struct firmware_buf *buf = fw_priv->buf;
 
+	/*
+	 * There is a small window in which user can write to 'loading'
+	 * between loading done and disappearance of 'loading'
+	 */
+	if (test_bit(FW_STATUS_DONE, &buf->status))
+		return;
+
 	set_bit(FW_STATUS_ABORT, &buf->status);
 	complete_all(&buf->completion);
+
+	/* avoid user action after loading abort */
+	fw_priv->buf = NULL;
 }
 
 #define is_fw_load_aborted(buf)	\
@@ -528,7 +538,12 @@ static ssize_t firmware_loading_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-	int loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+	int loading = 0;
+
+	mutex_lock(&fw_lock);
+	if (fw_priv->buf)
+		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -570,12 +585,12 @@ static ssize_t firmware_loading_store(struct device *dev,
 				      const char *buf, size_t count)
 {
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-	struct firmware_buf *fw_buf = fw_priv->buf;
+	struct firmware_buf *fw_buf;
 	int loading = simple_strtol(buf, NULL, 10);
 	int i;
 
 	mutex_lock(&fw_lock);
-
+	fw_buf = fw_priv->buf;
 	if (!fw_buf)
 		goto out;
 
@@ -777,10 +792,6 @@ static void firmware_class_timeout_work(struct work_struct *work)
 			struct firmware_priv, timeout_work.work);
 
 	mutex_lock(&fw_lock);
-	if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) {
-		mutex_unlock(&fw_lock);
-		return;
-	}
 	fw_load_abort(fw_priv);
 	mutex_unlock(&fw_lock);
 }
@@ -861,8 +872,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 
-	fw_priv->buf = NULL;
-
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
 	device_remove_bin_file(f_dev, &firmware_attr_data);
-- 
1.7.9.5

Re: [PATCH] firmware loader: fix use-after-free by double abort

[Greg Kroah-Hartman]

On Sat, Jun 15, 2013 at 04:36:38PM +0800, Ming Lei wrote:
> fw_priv->buf is accessed in both request_firmware_load() and
> writing to sysfs file of 'loading' context, but not protected
> by 'fw_lock' entirely. The patch makes sure that access on
> 'fw_priv->buf' is protected by the lock.
> 
> So fixes the double abort problem reported by nirinA raseliarison:
> 
> 	http://lkml.org/lkml/2013/6/14/188
> 
> Reported-and-tested-by: nirinA raseliarison <nirina.raseliarison@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

So this is a 3.9-stable thing?  Anything newer than that?

thanks,

greg k-h

Re: [PATCH] firmware loader: fix use-after-free by double abort

[Ming Lei]

On Tue, Jun 18, 2013 at 7:59 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Jun 15, 2013 at 04:36:38PM +0800, Ming Lei wrote:
>> fw_priv->buf is accessed in both request_firmware_load() and
>> writing to sysfs file of 'loading' context, but not protected
>> by 'fw_lock' entirely. The patch makes sure that access on
>> 'fw_priv->buf' is protected by the lock.
>>
>> So fixes the double abort problem reported by nirinA raseliarison:
>>
>>       http://lkml.org/lkml/2013/6/14/188
>>
>> Reported-and-tested-by: nirinA raseliarison <nirina.raseliarison@gmail.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: stable <stable@vger.kernel.org>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>
> So this is a 3.9-stable thing?  Anything newer than that?

Yes, only 3.9-stable need this.

Thanks,
--
Ming Lei

Re: [PATCH] firmware loader: fix use-after-free by double abort

[Guenter Roeck]

On Tue, Jun 18, 2013 at 08:33:55AM +0800, Ming Lei wrote:
> On Tue, Jun 18, 2013 at 7:59 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Jun 15, 2013 at 04:36:38PM +0800, Ming Lei wrote:
> >> fw_priv->buf is accessed in both request_firmware_load() and
> >> writing to sysfs file of 'loading' context, but not protected
> >> by 'fw_lock' entirely. The patch makes sure that access on
> >> 'fw_priv->buf' is protected by the lock.
> >>
> >> So fixes the double abort problem reported by nirinA raseliarison:
> >>
> >>       http://lkml.org/lkml/2013/6/14/188
> >>
> >> Reported-and-tested-by: nirinA raseliarison <nirina.raseliarison@gmail.com>
> >> Cc: Guenter Roeck <linux@roeck-us.net>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: stable <stable@vger.kernel.org>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >
> > So this is a 3.9-stable thing?  Anything newer than that?
> 
> Yes, only 3.9-stable need this.
> 
I may be missing something, but why would mainline not need it ?
Or do you mean "mainline plus 3.9" ?

Thanks,
Guenter

Re: [PATCH] firmware loader: fix use-after-free by double abort

[Ming Lei]

On Tue, Jun 18, 2013 at 12:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>
> I may be missing something, but why would mainline not need it ?
> Or do you mean "mainline plus 3.9" ?

Yes, mainline need it of course, sorry for not mentioning that explicitly.

Thanks,
--
Ming Lei