* [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() @ 2024-04-15 10:53 Zeng Heng 2024-04-15 11:08 ` Dan Carpenter ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Zeng Heng @ 2024-04-15 10:53 UTC (permalink / raw) To: linus.walleij Cc: linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, dan.carpenter, liwei391 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(-) 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; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-15 10:53 [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() Zeng Heng @ 2024-04-15 11:08 ` Dan Carpenter 2024-04-15 16:36 ` Markus Elfring ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2024-04-15 11:08 UTC (permalink / raw) To: Zeng Heng Cc: linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-15 10:53 [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() Zeng Heng 2024-04-15 11:08 ` Dan Carpenter @ 2024-04-15 16:36 ` Markus Elfring 2024-04-16 13:33 ` Linus Walleij 2024-04-17 15:30 ` Andy Shevchenko 3 siblings, 0 replies; 10+ messages in thread From: Markus Elfring @ 2024-04-15 16:36 UTC (permalink / raw) To: Zeng Heng, linux-gpio, kernel-janitors, Linus Walleij, Dan Carpenter Cc: LKML, Wei Li, Wei Yongjun, Xie XiuQi > … 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-15 10:53 [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() Zeng Heng 2024-04-15 11:08 ` Dan Carpenter 2024-04-15 16:36 ` Markus Elfring @ 2024-04-16 13:33 ` Linus Walleij 2024-04-17 15:30 ` Andy Shevchenko 3 siblings, 0 replies; 10+ messages in thread From: Linus Walleij @ 2024-04-16 13:33 UTC (permalink / raw) To: Zeng Heng Cc: linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, dan.carpenter, liwei391 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-15 10:53 [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() Zeng Heng ` (2 preceding siblings ...) 2024-04-16 13:33 ` Linus Walleij @ 2024-04-17 15:30 ` Andy Shevchenko 2024-04-17 15:38 ` Dan Carpenter 3 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2024-04-17 15:30 UTC (permalink / raw) To: Zeng Heng Cc: linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, dan.carpenter, liwei391 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; > } -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-17 15:30 ` Andy Shevchenko @ 2024-04-17 15:38 ` Dan Carpenter 2024-04-17 17:12 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2024-04-17 15:38 UTC (permalink / raw) To: Andy Shevchenko Cc: Zeng Heng, linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-17 15:38 ` Dan Carpenter @ 2024-04-17 17:12 ` Andy Shevchenko 2024-04-17 17:49 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2024-04-17 17:12 UTC (permalink / raw) To: Dan Carpenter Cc: Zeng Heng, linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-17 17:12 ` Andy Shevchenko @ 2024-04-17 17:49 ` Dan Carpenter 2024-04-18 10:05 ` Andy Shevchenko 2024-04-19 13:24 ` Linus Walleij 0 siblings, 2 replies; 10+ messages in thread From: Dan Carpenter @ 2024-04-17 17:49 UTC (permalink / raw) To: Andy Shevchenko Cc: Zeng Heng, linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-17 17:49 ` Dan Carpenter @ 2024-04-18 10:05 ` Andy Shevchenko 2024-04-19 13:24 ` Linus Walleij 1 sibling, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2024-04-18 10:05 UTC (permalink / raw) To: Dan Carpenter Cc: Zeng Heng, linus.walleij, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() 2024-04-17 17:49 ` Dan Carpenter 2024-04-18 10:05 ` Andy Shevchenko @ 2024-04-19 13:24 ` Linus Walleij 1 sibling, 0 replies; 10+ messages in thread From: Linus Walleij @ 2024-04-19 13:24 UTC (permalink / raw) To: Dan Carpenter Cc: Andy Shevchenko, Zeng Heng, linux-kernel, xiexiuqi, linux-gpio, weiyongjun1, liwei391 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-19 13:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-15 10:53 [PATCH] pinctrl: devicetree: fix refcount leak in pinctrl_dt_to_map() Zeng Heng 2024-04-15 11:08 ` Dan Carpenter 2024-04-15 16:36 ` Markus Elfring 2024-04-16 13:33 ` Linus Walleij 2024-04-17 15:30 ` Andy Shevchenko 2024-04-17 15:38 ` Dan Carpenter 2024-04-17 17:12 ` Andy Shevchenko 2024-04-17 17:49 ` Dan Carpenter 2024-04-18 10:05 ` Andy Shevchenko 2024-04-19 13:24 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).