Hello Moudy Ho, The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3 driver" from Aug 23, 2022, leads to the following Smatch static checker warning: drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c:460 mdp_cmdq_send() error: we previously assumed 'cmd' could be null (see line 369) The error handling in mdp_cmdq_send() has a number of bugs. Dereferencing uninitialized pointers, a double free, stack traces, and not propagating the negative error code, and returning a positive number instead of a negative error codes. See below for details and potential fixes (not complete). Read my blog for more Error Handling Tips! https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/ diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c index 29f6c1cd3de7..0cdb62c27b58 100644 --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c @@ -252,10 +252,9 @@ static int mdp_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *pkt, dma_addr_t dma_addr; pkt->va_base = kzalloc(size, GFP_KERNEL); - if (!pkt->va_base) { - kfree(pkt); NOTE1: Freeing here lead to a double free + if (!pkt->va_base) return -ENOMEM; - } + pkt->buf_size = size; pkt->cl = (void *)client; @@ -368,25 +367,24 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param) cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); if (!cmd) { ret = -ENOMEM; - goto err_cmdq_data; + goto err_decrement; } - if (mdp_cmdq_pkt_create(mdp->cmdq_clt, &cmd->pkt, SZ_16K)) { - ret = -ENOMEM; + ret = mdp_cmdq_pkt_create(mdp->cmdq_clt, &cmd->pkt, SZ_16K); + if (ret) goto err_cmdq_data; - } comps = kcalloc(param->config->num_components, sizeof(*comps), GFP_KERNEL); if (!comps) { ret = -ENOMEM; - goto err_cmdq_data; + goto err_destroy_pkt; } path = kzalloc(sizeof(*path), GFP_KERNEL); if (!path) { ret = -ENOMEM; - goto err_cmdq_data; + goto err_free_comps; } path->mdp_dev = mdp; @@ -406,7 +404,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param) ret = mdp_path_ctx_init(mdp, path); if (ret) { dev_err(dev, "mdp_path_ctx_init error\n"); - goto err_cmdq_data; + goto err_free_path; } mtk_mutex_prepare(mdp->mdp_mutex[MDP_PIPE_RDMA0]); @@ -414,7 +412,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param) ret = mdp_path_config(mdp, cmd, path); if (ret) { dev_err(dev, "mdp_path_config error\n"); - goto err_cmdq_data; + goto err_free_path; } cmdq_pkt_finalize(&cmd->pkt); @@ -433,7 +431,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param) ret = mdp_comp_clocks_on(&mdp->pdev->dev, cmd->comps, cmd->num_comps); if (ret) { dev_err(dev, "comp %d failed to enable clock!\n", ret); - goto err_clock_off; + goto err_free_path; } dma_sync_single_for_device(mdp->cmdq_clt->chan->mbox->dev, @@ -453,14 +451,20 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param) mtk_mutex_unprepare(mdp->mdp_mutex[MDP_PIPE_RDMA0]); mdp_comp_clocks_off(&mdp->pdev->dev, cmd->comps, cmd->num_comps); -err_cmdq_data: +// FIXME: what to do here? +// err_cmdq_data: +// wake_up(&mdp->callback_wq); NOTE2: not sure how why we call wake_up(&mdp->callback_wq); +err_free_path: kfree(path); - atomic_dec(&mdp->job_count); - wake_up(&mdp->callback_wq); - if (cmd->pkt.buf_size > 0) - mdp_cmdq_pkt_destroy(&cmd->pkt); +err_free_comps: kfree(comps); +err_destroy_pkt: + mdp_cmdq_pkt_destroy(&cmd->pkt); +err_free_cmd: kfree(cmd); +err_decrement: + atomic_dec(&mdp->job_count); + return ret; } EXPORT_SYMBOL_GPL(mdp_cmdq_send); diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c index e62abf3587bf..a18ac10269a6 100644 --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c @@ -699,11 +699,22 @@ int mdp_comp_clock_on(struct device *dev, struct mdp_comp *comp) dev_err(dev, "Failed to enable clk %d. type:%d id:%d\n", i, comp->type, comp->id); - return ret; + goto err_unwind; } } return 0; NOTE3: This function *must* clean up after itself. Calling clk_disable_unprepare() on an uninitialized clk will trigger a stack trace. + +err_unwind: + while (--i >= 0) { + if (IS_ERR_OR_NULL(comp->clks[i])) + continue; + clk_disable_unprepare(comp->clks[i]); + } + if (comp->comp_dev) + pm_runtime_put_sync(comp->comp_dev); + + return ret; } void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp) @@ -722,13 +733,21 @@ void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp) int mdp_comp_clocks_on(struct device *dev, struct mdp_comp *comps, int num) { - int i; + int ret, i; - for (i = 0; i < num; i++) - if (mdp_comp_clock_on(dev, &comps[i]) != 0) - return ++i; NOTE4: This function is supposed to return a negative error code. + for (i = 0; i < num; i++) { + ret = mdp_comp_clock_on(dev, &comps[i]); + if (ret) + goto err_unwind; + } return 0; + NOTE5: Same bug here. +err_unwind: + while (--i >= 0) + mdp_comp_clock_off(dev, &comps[i]); + + return ret; } void mdp_comp_clocks_off(struct device *dev, struct mdp_comp *comps, int num)
