| Message ID | 20240415105328.3651441-1-zengheng4@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() | expand |
On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: > If we fail to allocate propname buffer, we need to drop the reference > count we just took. Because the pinctrl_dt_free_maps() includes the > droping operation, here we call it directly. > > Fixes: 91d5c5060ee2 ("pinctrl: devicetree: fix null pointer dereferencing in pinctrl_dt_to_map") > Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: Zeng Heng <zengheng4@huawei.com> > --- Thanks! Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> regards, dan carpenter
> … Because the pinctrl_dt_free_maps() includes the > droping operation, here we call it directly. I find this change description improvable. * How do you think about to avoid a typo? * Would another imperative wording be more desirable? Regards, Markus
On Mon, Apr 15, 2024 at 12:54 PM Zeng Heng <zengheng4@huawei.com> wrote: > If we fail to allocate propname buffer, we need to drop the reference > count we just took. Because the pinctrl_dt_free_maps() includes the > droping operation, here we call it directly. > > Fixes: 91d5c5060ee2 ("pinctrl: devicetree: fix null pointer dereferencing in pinctrl_dt_to_map") > Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: Zeng Heng <zengheng4@huawei.com> Patch applied for fixes. Yours, Linus Walleij
On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: > If we fail to allocate propname buffer, we need to drop the reference > count we just took. Because the pinctrl_dt_free_maps() includes the > droping operation, here we call it directly. ... > for (state = 0; ; state++) { > /* Retrieve the pinctrl-* property */ > propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); > - if (!propname) > - return -ENOMEM; > + if (!propname) { > + ret = -ENOMEM; > + goto err; > + } > prop = of_find_property(np, propname, &size); > kfree(propname); > if (!prop) { > if (state == 0) { > - of_node_put(np); > - return -ENODEV; > + ret = -ENODEV; > + goto err; Has it been tested? How on earth is this a correct change? We iterate over state numbers until we have properties available. This chunk is _successful_ exit path, we may not free parsed maps! Am I wrong? > } > break; > }
On Wed, Apr 17, 2024 at 06:30:59PM +0300, Andy Shevchenko wrote: > On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: > > If we fail to allocate propname buffer, we need to drop the reference > > count we just took. Because the pinctrl_dt_free_maps() includes the > > droping operation, here we call it directly. > > ... > > > for (state = 0; ; state++) { > > /* Retrieve the pinctrl-* property */ > > propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); > > - if (!propname) > > - return -ENOMEM; > > + if (!propname) { > > + ret = -ENOMEM; > > + goto err; > > + } > > prop = of_find_property(np, propname, &size); > > kfree(propname); > > if (!prop) { > > if (state == 0) { > > - of_node_put(np); > > - return -ENODEV; > > + ret = -ENODEV; > > + goto err; > > Has it been tested? How on earth is this a correct change? > > We iterate over state numbers until we have properties available. This chunk is > _successful_ exit path, we may not free parsed maps! Am I wrong? In this path state == 0 so we haven't had a successful iteration yet. regards, dan carpenter
On Wed, Apr 17, 2024 at 06:38:46PM +0300, Dan Carpenter wrote: > On Wed, Apr 17, 2024 at 06:30:59PM +0300, Andy Shevchenko wrote: > > On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: ... > > > for (state = 0; ; state++) { > > > /* Retrieve the pinctrl-* property */ > > > propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); > > > - if (!propname) > > > - return -ENOMEM; > > > + if (!propname) { > > > + ret = -ENOMEM; > > > + goto err; > > > + } > > > prop = of_find_property(np, propname, &size); > > > kfree(propname); > > > if (!prop) { > > > if (state == 0) { > > > - of_node_put(np); > > > - return -ENODEV; > > > + ret = -ENODEV; > > > + goto err; > > > > Has it been tested? How on earth is this a correct change? > > > > We iterate over state numbers until we have properties available. This chunk is > > _successful_ exit path, we may not free parsed maps! Am I wrong? > > In this path state == 0 so we haven't had a successful iteration yet. Ah, indeed, this is not a status. Okay, makes sense, but calling that free function for the purpose of the putting of_node seems an overkill...
On Wed, Apr 17, 2024 at 08:12:23PM +0300, Andy Shevchenko wrote: > On Wed, Apr 17, 2024 at 06:38:46PM +0300, Dan Carpenter wrote: > > On Wed, Apr 17, 2024 at 06:30:59PM +0300, Andy Shevchenko wrote: > > > On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: > > ... > > > > > for (state = 0; ; state++) { > > > > /* Retrieve the pinctrl-* property */ > > > > propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); > > > > - if (!propname) > > > > - return -ENOMEM; > > > > + if (!propname) { > > > > + ret = -ENOMEM; > > > > + goto err; > > > > + } > > > > prop = of_find_property(np, propname, &size); > > > > kfree(propname); > > > > if (!prop) { > > > > if (state == 0) { > > > > - of_node_put(np); > > > > - return -ENODEV; > > > > + ret = -ENODEV; > > > > + goto err; > > > > > > Has it been tested? How on earth is this a correct change? > > > > > > We iterate over state numbers until we have properties available. This chunk is > > > _successful_ exit path, we may not free parsed maps! Am I wrong? > > > > In this path state == 0 so we haven't had a successful iteration yet. > > Ah, indeed, this is not a status. Okay, makes sense, but calling that free > function for the purpose of the putting of_node seems an overkill... Sure, that's one way to look at it, but it's suspicious looking when there is a direct return which is surrounded by gotos. As I write this, I remember that Smatch has a warning for code like that. Probably we should add a comment to say: /* Return -ENODEV if the property 'pinctrl-0' is not present. */ regards, dan carpenter
On Wed, Apr 17, 2024 at 08:49:42PM +0300, Dan Carpenter wrote: > On Wed, Apr 17, 2024 at 08:12:23PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 17, 2024 at 06:38:46PM +0300, Dan Carpenter wrote: > > > On Wed, Apr 17, 2024 at 06:30:59PM +0300, Andy Shevchenko wrote: > > > > On Mon, Apr 15, 2024 at 06:53:28PM +0800, Zeng Heng wrote: ... > > > > > for (state = 0; ; state++) { > > > > > /* Retrieve the pinctrl-* property */ > > > > > propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); > > > > > - if (!propname) > > > > > - return -ENOMEM; > > > > > + if (!propname) { > > > > > + ret = -ENOMEM; > > > > > + goto err; > > > > > + } > > > > > prop = of_find_property(np, propname, &size); > > > > > kfree(propname); > > > > > if (!prop) { > > > > > if (state == 0) { > > > > > - of_node_put(np); > > > > > - return -ENODEV; > > > > > + ret = -ENODEV; > > > > > + goto err; > > > > > > > > Has it been tested? How on earth is this a correct change? > > > > > > > > We iterate over state numbers until we have properties available. This chunk is > > > > _successful_ exit path, we may not free parsed maps! Am I wrong? > > > > > > In this path state == 0 so we haven't had a successful iteration yet. > > > > Ah, indeed, this is not a status. Okay, makes sense, but calling that free > > function for the purpose of the putting of_node seems an overkill... > > Sure, that's one way to look at it, but it's suspicious looking when > there is a direct return which is surrounded by gotos. As I write this, > I remember that Smatch has a warning for code like that. > > Probably we should add a comment to say: > > /* Return -ENODEV if the property 'pinctrl-0' is not present. */ Good idea, go for it!
On Wed, Apr 17, 2024 at 7:49 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > Probably we should add a comment to say: > > /* Return -ENODEV if the property 'pinctrl-0' is not present. */ Would you mind sending a oneliner on top to fix this? Yours, Linus Walleij
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c index df1efc2e5202..6a94ecd6a8de 100644 --- a/drivers/pinctrl/devicetree.c +++ b/drivers/pinctrl/devicetree.c @@ -220,14 +220,16 @@ int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev) for (state = 0; ; state++) { /* Retrieve the pinctrl-* property */ propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state); - if (!propname) - return -ENOMEM; + if (!propname) { + ret = -ENOMEM; + goto err; + } prop = of_find_property(np, propname, &size); kfree(propname); if (!prop) { if (state == 0) { - of_node_put(np); - return -ENODEV; + ret = -ENODEV; + goto err; } break; }
If we fail to allocate propname buffer, we need to drop the reference count we just took. Because the pinctrl_dt_free_maps() includes the droping operation, here we call it directly. Fixes: 91d5c5060ee2 ("pinctrl: devicetree: fix null pointer dereferencing in pinctrl_dt_to_map") Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by: Zeng Heng <zengheng4@huawei.com> --- drivers/pinctrl/devicetree.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)