linux-kernel - Re: request_firmware API exhaust memory

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]

Message-ID: <1272372187.5484.3981.camel@macbook.infradead.org>
Date:	Tue, 27 Apr 2010 13:43:07 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Kay Sievers <kay.sievers@...y.org>, dhowells@...hat.com
Cc:	Tomas Winkler <tomasw@...il.com>, Greg KH <greg@...ah.com>,
	Johannes Berg <johannes@...solutions.net>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Emmanuel Grumbach <egrumbach@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: request_firmware API exhaust memory

On Tue, 2010-04-27 at 14:05 +0200, Kay Sievers wrote:
> 
> The patch I posted makes the issue go away. It's still not the right
> fix, because the pages are only get freed when the device id cleaned
> up, not on calling release_firmware. But it should illustrate the
> underlying issue, and that there is no leaked memory anymore.
> 
> >  I think this needs some more review.
> 
> If David does not fix it, it probably just needs to be reverted. And
> instead of implementing our own "memory management", we should rather
> add a vrealloc(), and the firmware loader should use that. 

The whole point was to avoid the vrealloc(). We really don't want to be
screwing with page tables, globally, for each page written from
userspace.

This untested patch attempts to put the page array into the 'struct
firmware' so that we can free it from release_firmware().

It would actually be nice if we could make that the _primary_ method of
returning data to drivers, and we could ditch the vmap() requirement
altogether... drivers which really need it to be virtually contiguous
can depend on CONFIG_MMU and do the vmap() for themselves.

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..cc9a79b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -162,8 +162,13 @@ static ssize_t firmware_loading_store(struct device *dev,
 			mutex_unlock(&fw_lock);
 			break;
 		}
-		vfree(fw_priv->fw->data);
+		vunmap(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
+		if (fw_priv->fw->pages) {
+			for (i = 0; i < PFN_UP(fw_priv->fw->size); i++)
+				__free_page(fw_priv->fw->pages[i]);
+			kfree(fw_priv->fw->pages);
+		}
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
 		kfree(fw_priv->pages);
@@ -176,7 +181,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			vfree(fw_priv->fw->data);
+			vunmap(fw_priv->fw->data);
 			fw_priv->fw->data = vmap(fw_priv->pages,
 						 fw_priv->nr_pages,
 						 0, PAGE_KERNEL_RO);
@@ -184,7 +189,9 @@ static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: vmap() failed\n", __func__);
 				goto err;
 			}
-			/* Pages will be freed by vfree() */
+			/* Pages are now owned by 'struct firmware' */
+			fw_priv->fw->pages = fw_priv->pages;
+			fw_priv->pages = NULL;
 			fw_priv->page_array_size = 0;
 			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
@@ -571,6 +578,7 @@ void
 release_firmware(const struct firmware *fw)
 {
 	struct builtin_fw *builtin;
+	int i;
 
 	if (fw) {
 		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
@@ -578,7 +586,12 @@ release_firmware(const struct firmware *fw)
 			if (fw->data == builtin->data)
 				goto free_fw;
 		}
-		vfree(fw->data);
+		vunmap(fw->data);
+		if (fw->pages) {
+			for (i = 0; i < PFN_UP(fw->size); i++)
+				__free_page(fw->pages[i]);
+			kfree(fw->pages);
+		}
 	free_fw:
 		kfree(fw);
 	}


-- 
dwmw2

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