[PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()

[PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()

[Joe Hattori]

scmi_txrx_setup() calls scmi_chan_setup(), which increments the refcount
of the given OF node. When the Rx channel setup fails with ENOMEM,
scmi_chan_setup() returns the error, but does not release the OF node
obtained in the Tx channel setup. Thus, add an of_node_put() call when
the Rx setup fails with ENOMEM.

This bug was found by an experimental static analysis tool that I am
developing.

Fixes: be9ba1f7f9e0 ("firmware: arm_scmi: Make Rx chan_setup fail on memory errors")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 drivers/firmware/arm_scmi/driver.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 1b5fb2c4ce86..5d8f877d75f3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2700,7 +2700,9 @@ scmi_txrx_setup(struct scmi_info *info, struct device_node *of_node,
 	if (!ret) {
 		/* Rx is optional, report only memory errors */
 		ret = scmi_chan_setup(info, of_node, prot_id, false);
-		if (ret && ret != -ENOMEM)
+		if (ret == -ENOMEM)
+			of_node_put(of_node);
+		else if (ret)
 			ret = 0;
 	}
 
-- 
2.34.1

Re: [PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()

[Dan Carpenter]

On Tue, Dec 17, 2024 at 11:31:22AM +0900, Joe Hattori wrote:
> scmi_txrx_setup() calls scmi_chan_setup(), which increments the refcount
> of the given OF node. When the Rx channel setup fails with ENOMEM,
> scmi_chan_setup() returns the error, but does not release the OF node
> obtained in the Tx channel setup. Thus, add an of_node_put() call when
> the Rx setup fails with ENOMEM.
> 
> This bug was found by an experimental static analysis tool that I am
> developing.
>

No, the patch is wrong.

Your static checker has missed the call to of_node_get(of_node).  Only
the success path takes a reference to the of_node.  To be honest, I
think the success path should drop the reference.  Generally with
firmware nodes, we hold the reference for very short time.

drivers/firmware/arm_scmi/driver.c
  2615  static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
  2616                             int prot_id, bool tx)
  2617  {
  2618          int ret, idx;
  2619          char name[32];
  2620          struct scmi_chan_info *cinfo;
  2621          struct idr *idr;
  2622          struct scmi_device *tdev = NULL;
  2623  
  2624          /* Transmit channel is first entry i.e. index 0 */
  2625          idx = tx ? 0 : 1;
  2626          idr = tx ? &info->tx_idr : &info->rx_idr;
  2627  
  2628          if (!info->desc->ops->chan_available(of_node, idx)) {
  2629                  cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
  2630                  if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
  2631                          return -EINVAL;

Why does your checker not complain about any of these error paths?

  2632                  goto idr_alloc;

This goto is succes, but doesn't boost the refcount.  Which is correct.

  2633          }
  2634  
  2635          cinfo = devm_kzalloc(info->dev, sizeof(*cinfo), GFP_KERNEL);
  2636          if (!cinfo)
  2637                  return -ENOMEM;

I guess this is what your checker sees?

  2638  
  2639          cinfo->is_p2a = !tx;
  2640          cinfo->rx_timeout_ms = info->desc->max_rx_timeout_ms;
  2641          cinfo->max_msg_size = info->desc->max_msg_size;
  2642  
  2643          /* Create a unique name for this transport device */
  2644          snprintf(name, 32, "__scmi_transport_device_%s_%02X",
  2645                   idx ? "rx" : "tx", prot_id);
  2646          /* Create a uniquely named, dedicated transport device for this chan */
  2647          tdev = scmi_device_create(of_node, info->dev, prot_id, name);
  2648          if (!tdev) {
  2649                  dev_err(info->dev,
  2650                          "failed to create transport device (%s)\n", name);
  2651                  devm_kfree(info->dev, cinfo);
  2652                  return -EINVAL;
  2653          }
  2654          of_node_get(of_node);

Bump the refcount.

  2655  
  2656          cinfo->id = prot_id;
  2657          cinfo->dev = &tdev->dev;
  2658          ret = info->desc->ops->chan_setup(cinfo, info->dev, tx);
  2659          if (ret) {
  2660                  of_node_put(of_node);

Drop the refcount on failure

  2661                  scmi_device_destroy(info->dev, prot_id, name);
  2662                  devm_kfree(info->dev, cinfo);
  2663                  return ret;
  2664          }
  2665  
  2666          if (tx && is_polling_required(cinfo, info->desc)) {
  2667                  if (is_transport_polling_capable(info->desc))
  2668                          dev_info(&tdev->dev,
  2669                                   "Enabled polling mode TX channel - prot_id:%d\n",
  2670                                   prot_id);
  2671                  else
  2672                          dev_warn(&tdev->dev,
  2673                                   "Polling mode NOT supported by transport.\n");
  2674          }
  2675  
  2676  idr_alloc:
  2677          ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
  2678          if (ret != prot_id) {
  2679                  dev_err(info->dev,
  2680                          "unable to allocate SCMI idr slot err %d\n", ret);
  2681                  /* Destroy channel and device only if created by this call. */
  2682                  if (tdev) {
  2683                          of_node_put(of_node);

Drop the refcount on failure

  2684                          scmi_device_destroy(info->dev, prot_id, name);
  2685                          devm_kfree(info->dev, cinfo);
  2686                  }
  2687                  return ret;
  2688          }
  2689  
  2690          cinfo->handle = &info->handle;
  2691          return 0;
  2692  }

Btw, have you looked at the refcount tracking in Smatch?  It's does
cross function analysis.  But scmi_chan_setup() the success path only
increments the refcount sometimes so it doesn't get recorded.  If I
comment out goto idr_alloc and the label then it works.

$ smdb return_states scmi_chan_setup | grep REFCOUNT
drivers/firmware/arm_scmi/driver.c | scmi_chan_setup | 730 |             0|    REFCOUNT_INC |  1 | $->kobj.kref.refcount.refs.counter |                      |
drivers/firmware/arm_scmi/driver.c | scmi_chan_setup | 731 |             0|    REFCOUNT_INC |  1 | $->kobj.kref.refcount.refs.counter |                      |

The last success path is split into multiple returns based on some
heuristics and two of them are success...  Need to investigate.  That
seems like a waste.

regards,
dan carpenter

Re: [PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()

[Krzysztof Kozlowski]

On 17/12/2024 06:51, Dan Carpenter wrote:
> On Tue, Dec 17, 2024 at 11:31:22AM +0900, Joe Hattori wrote:
>> scmi_txrx_setup() calls scmi_chan_setup(), which increments the refcount
>> of the given OF node. When the Rx channel setup fails with ENOMEM,
>> scmi_chan_setup() returns the error, but does not release the OF node
>> obtained in the Tx channel setup. Thus, add an of_node_put() call when
>> the Rx setup fails with ENOMEM.
>>
>> This bug was found by an experimental static analysis tool that I am
>> developing.
>>
> 
> No, the patch is wrong.
> 
There are more patches like this without understanding the OF
drop/retain reference behavior. I suggest careful review of all results
of this "static analysis tool".

Best regards,
Krzysztof

Re: [PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()

[Sudeep Holla]

On Tue, Dec 17, 2024 at 11:50:27AM +0100, Krzysztof Kozlowski wrote:
> On 17/12/2024 06:51, Dan Carpenter wrote:
> > On Tue, Dec 17, 2024 at 11:31:22AM +0900, Joe Hattori wrote:
> >> scmi_txrx_setup() calls scmi_chan_setup(), which increments the refcount
> >> of the given OF node. When the Rx channel setup fails with ENOMEM,
> >> scmi_chan_setup() returns the error, but does not release the OF node
> >> obtained in the Tx channel setup. Thus, add an of_node_put() call when
> >> the Rx setup fails with ENOMEM.
> >>
> >> This bug was found by an experimental static analysis tool that I am
> >> developing.
> >>
> > 
> > No, the patch is wrong.
> > 
> There are more patches like this without understanding the OF
> drop/retain reference behavior. I suggest careful review of all results
> of this "static analysis tool".
> 

Thanks Dan and Krzysztof for the quick heads up and suggestions. Much
appreciated.

--
Regards,
Sudeep