| Message ID | 43ad5358-f5b2-4cfc-85b4-e7ab8c7cf329@stanley.mountain (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: fix cleanup in amd_pmf_init_smart_pc() | expand |
On Mon, 10 Mar 2025, Dan Carpenter wrote: > There are a couple problems in this code: > > First, if amd_pmf_tee_init() fails then the function returns directly > instead of cleaning up. We cannot simply do a "goto error;" because > that would lead to a double free. I have re-written this code to > use an unwind ladder to free the allocations. Thanks Dan, Could you please amend this with the information of what is getting double freed, it took considerable amount of time for me to figure out. I assume it's ->fw_shm_pool ? > Second, if amd_pmf_start_policy_engine() fails on every iteration though > the loop then the code calls amd_pmf_tee_deinit() twice which is also a > double free. Call amd_pmf_tee_deinit() inside the loop for each failed > iteration. Also on that path the error codes are not necessarily > negative kernel error codes. Set the error code to -EINVAL. Maybe I should start to consistently reject any attempt to use cleanup/deinit helper functions instead of a proper rollback. It seems a pattern that is very prone to errors like this. > Fixes: 376a8c2a1443 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/platform/x86/amd/pmf/tee-if.c | 36 +++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c > index ceaff1ebb7b9..a1e43873a07b 100644 > --- a/drivers/platform/x86/amd/pmf/tee-if.c > +++ b/drivers/platform/x86/amd/pmf/tee-if.c > @@ -510,18 +510,18 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) > > ret = amd_pmf_set_dram_addr(dev, true); > if (ret) > - goto error; > + goto err_cancel_work; > > dev->policy_base = devm_ioremap_resource(dev->dev, dev->res); > if (IS_ERR(dev->policy_base)) { > ret = PTR_ERR(dev->policy_base); > - goto error; > + goto err_free_dram_buf; > } > > dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); > if (!dev->policy_buf) { > ret = -ENOMEM; > - goto error; > + goto err_free_dram_buf; > } > > memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz); > @@ -531,13 +531,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) > dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); > if (!dev->prev_data) { > ret = -ENOMEM; > - goto error; > + goto err_free_policy; > } > > for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) { > ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]); > if (ret) > - return ret; > + goto err_free_prev_data; > > ret = amd_pmf_start_policy_engine(dev); > switch (ret) { > @@ -550,27 +550,41 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) > status = false; > break; > default: > - goto error; > + ret = -EINVAL; > + amd_pmf_tee_deinit(dev); > + goto err_free_prev_data; > } > > if (status) > break; > } > > - if (!status && !pb_side_load) > - goto error; > + if (!status && !pb_side_load) { > + ret = -EINVAL; > + goto err_free_prev_data; > + } > > if (pb_side_load) > amd_pmf_open_pb(dev, dev->dbgfs_dir); > > ret = amd_pmf_register_input_device(dev); > if (ret) > - goto error; > + goto err_pmf_remove_pb; > > return 0; > > -error: > - amd_pmf_deinit_smart_pc(dev); > +err_pmf_remove_pb: > + if (pb_side_load && dev->esbin) > + amd_pmf_remove_pb(dev); > + amd_pmf_tee_deinit(dev); > +err_free_prev_data: > + kfree(dev->prev_data); > +err_free_policy: > + kfree(dev->policy_buf); > +err_free_dram_buf: > + kfree(dev->buf); > +err_cancel_work: > + cancel_delayed_work_sync(&dev->pb_work); > > return ret; > } >
On Mon, Mar 10, 2025 at 02:43:51PM +0200, Ilpo Järvinen wrote: > On Mon, 10 Mar 2025, Dan Carpenter wrote: > > > There are a couple problems in this code: > > > > First, if amd_pmf_tee_init() fails then the function returns directly > > instead of cleaning up. We cannot simply do a "goto error;" because > > that would lead to a double free. I have re-written this code to > > use an unwind ladder to free the allocations. > > Thanks Dan, > > Could you please amend this with the information of what is getting > double freed, it took considerable amount of time for me to figure out. > I assume it's ->fw_shm_pool ? > Yes, that's it. Sure, I can re-write that. > > Second, if amd_pmf_start_policy_engine() fails on every iteration though > > the loop then the code calls amd_pmf_tee_deinit() twice which is also a > > double free. Call amd_pmf_tee_deinit() inside the loop for each failed > > iteration. Also on that path the error codes are not necessarily > > negative kernel error codes. Set the error code to -EINVAL. > > Maybe I should start to consistently reject any attempt to use > cleanup/deinit helper functions instead of a proper rollback. It > seems a pattern that is very prone to errors like this. I do not like deinit functions. They are so hard to review. But I detected this bug because of a Smatch warning: drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc() warn: missing unwind goto? regards, dan carpenter
Hi Dan, On 3/10/2025 20:22, Dan Carpenter wrote: > On Mon, Mar 10, 2025 at 02:43:51PM +0200, Ilpo Järvinen wrote: >> On Mon, 10 Mar 2025, Dan Carpenter wrote: >> >>> There are a couple problems in this code: >>> >>> First, if amd_pmf_tee_init() fails then the function returns directly >>> instead of cleaning up. We cannot simply do a "goto error;" because >>> that would lead to a double free. I have re-written this code to >>> use an unwind ladder to free the allocations. >> >> Thanks Dan, >> >> Could you please amend this with the information of what is getting >> double freed, it took considerable amount of time for me to figure out. >> I assume it's ->fw_shm_pool ? >> > > Yes, that's it. Sure, I can re-write that. > >>> Second, if amd_pmf_start_policy_engine() fails on every iteration though >>> the loop then the code calls amd_pmf_tee_deinit() twice which is also a >>> double free. Call amd_pmf_tee_deinit() inside the loop for each failed >>> iteration. Also on that path the error codes are not necessarily >>> negative kernel error codes. Set the error code to -EINVAL. >> >> Maybe I should start to consistently reject any attempt to use >> cleanup/deinit helper functions instead of a proper rollback. It >> seems a pattern that is very prone to errors like this. > > I do not like deinit functions. They are so hard to review. But I > detected this bug because of a Smatch warning: > > drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc() warn: missing unwind goto? Thank you for the fix. We have a CI that runs sparse/smatch/coverity to catch all these issues and unfortunately this was not caught by the CI system. But, I can confirm that Smatch is triggering this. drivers/platform/x86/amd/pmf/tee-if.c CHECK scripts/mod/empty.c CALL scripts/checksyscalls.sh DESCEND objtool INSTALL libsubcmd_headers CC [M] drivers/platform/x86/amd/pmf/tee-if.o CHECK drivers/platform/x86/amd/pmf/tee-if.c drivers/platform/x86/amd/pmf/tee-if.c:540 amd_pmf_init_smart_pc() warn: missing unwind goto? Thanks, Shyam
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c index ceaff1ebb7b9..a1e43873a07b 100644 --- a/drivers/platform/x86/amd/pmf/tee-if.c +++ b/drivers/platform/x86/amd/pmf/tee-if.c @@ -510,18 +510,18 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) ret = amd_pmf_set_dram_addr(dev, true); if (ret) - goto error; + goto err_cancel_work; dev->policy_base = devm_ioremap_resource(dev->dev, dev->res); if (IS_ERR(dev->policy_base)) { ret = PTR_ERR(dev->policy_base); - goto error; + goto err_free_dram_buf; } dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL); if (!dev->policy_buf) { ret = -ENOMEM; - goto error; + goto err_free_dram_buf; } memcpy_fromio(dev->policy_buf, dev->policy_base, dev->policy_sz); @@ -531,13 +531,13 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL); if (!dev->prev_data) { ret = -ENOMEM; - goto error; + goto err_free_policy; } for (i = 0; i < ARRAY_SIZE(amd_pmf_ta_uuid); i++) { ret = amd_pmf_tee_init(dev, &amd_pmf_ta_uuid[i]); if (ret) - return ret; + goto err_free_prev_data; ret = amd_pmf_start_policy_engine(dev); switch (ret) { @@ -550,27 +550,41 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev) status = false; break; default: - goto error; + ret = -EINVAL; + amd_pmf_tee_deinit(dev); + goto err_free_prev_data; } if (status) break; } - if (!status && !pb_side_load) - goto error; + if (!status && !pb_side_load) { + ret = -EINVAL; + goto err_free_prev_data; + } if (pb_side_load) amd_pmf_open_pb(dev, dev->dbgfs_dir); ret = amd_pmf_register_input_device(dev); if (ret) - goto error; + goto err_pmf_remove_pb; return 0; -error: - amd_pmf_deinit_smart_pc(dev); +err_pmf_remove_pb: + if (pb_side_load && dev->esbin) + amd_pmf_remove_pb(dev); + amd_pmf_tee_deinit(dev); +err_free_prev_data: + kfree(dev->prev_data); +err_free_policy: + kfree(dev->policy_buf); +err_free_dram_buf: + kfree(dev->buf); +err_cancel_work: + cancel_delayed_work_sync(&dev->pb_work); return ret; }
There are a couple problems in this code: First, if amd_pmf_tee_init() fails then the function returns directly instead of cleaning up. We cannot simply do a "goto error;" because that would lead to a double free. I have re-written this code to use an unwind ladder to free the allocations. Second, if amd_pmf_start_policy_engine() fails on every iteration though the loop then the code calls amd_pmf_tee_deinit() twice which is also a double free. Call amd_pmf_tee_deinit() inside the loop for each failed iteration. Also on that path the error codes are not necessarily negative kernel error codes. Set the error code to -EINVAL. Fixes: 376a8c2a1443 ("platform/x86/amd/pmf: Update PMF Driver for Compatibility with new PMF-TA") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/platform/x86/amd/pmf/tee-if.c | 36 +++++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-)