linux-kernel - memory leak and other oddness in pinctrl-mvebu.c

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-next>] [day] [month] [year] [list]

Date:	Fri, 08 Mar 2013 15:58:51 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Jason Cooper <jason@...edaemon.net>,
	linux-kernel@...r.kernel.org
Subject: memory leak and other oddness in pinctrl-mvebu.c

I'm just looking through the kernel for krealloc() abuse, and the
'interesting' code in mvebu_pinctrl_build_functions() came to my
attention.

First it allocates an array 'funcs' as follows:
	/* we allocate functions for number of pins and hope
	 * there are less unique functions than pins available */
	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);

(note: s/less/fewer/ in the comment).

Then it populates the array, seemly trusting to its "hope" and just
going off the end of the array if there are more unique functions than
pins?

And then finally it reallocates the array to the correct size:

	/* with the number of unique functions and it's groups known,
	   reallocate functions and assign group names */
	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
			 GFP_KERNEL);

(note: s/it's/its/)

So... if krealloc fails, we *leak* the originally-allocated 'funcs'.
Except actually we don't because it was allocated with devm_kzalloc() so
that's OK.

If krealloc *succeeds* then I think we should get a double-free because
it doesn't free the old 'funcs' via devm_kfree() so when the device is
eventually released, won't devres_release_all() free it again?

I'm not entirely sure what that krealloc is *for*, anyway. Apart from
retrospectively reallocating the array to the size that we've already
*scribbled* over? Some kind of request for forgiveness, perhaps?

We should probably make the standard kfree() (and hence krealloc())
actually fail when handed pointers which were allocated with
devm_kzalloc()?

This completely untested patch attempts to fix it by counting the
maximum number of functions in advance, then allocating the array at
that size. In practice it may overallocate if there are name collisions
and _add_function() returns -EEXIST. Is that something we really need to
lose sleep over?

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..5f9ece6 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -500,13 +500,25 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int num = 0;
 	int n, s;

-	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
-	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+	/* Count the number of functions prior to allocating 'funcs' array */
+	for (n = 0; n < pctl->num_groups; n++) {
+		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
+		for (s = 0; s < grp->num_settings; s++) {
+			/* skip unsupported settings on this variant */
+			if (pctl->variant &&
+			    !(pctl->variant & grp->settings[s].variant))
+				continue;
+
+			num++;
+		}
+	}
+
+	funcs = devm_kzalloc(&pdev->dev, num *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
 		return -ENOMEM;

+	num = 0;
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
@@ -523,13 +535,6 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 		}
 	}

-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;

-- 
dwmw2

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (6171 bytes)