* Re: [PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup()
2024-12-17 2:31 [PATCH] firmware: arm_scmi: fix an OF node reference leak in scmi_txrx_setup() Joe Hattori
@ 2024-12-17 5:51 ` Dan Carpenter
2024-12-17 10:50 ` Krzysztof Kozlowski
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-12-17 5:51 UTC (permalink / raw)
To: Joe Hattori; +Cc: sudeep.holla, cristian.marussi, arm-scmi
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
^ permalink raw reply [flat|nested] 4+ messages in thread