diff mbox series

[v2,2/2] platform/x86/amd/pmf: Fixup error handling for amd_pmf_init_smart_pc()

Message ID 20240217014107.113749-3-mario.limonciello@amd.com
State New
Headers
Series AMD PMF Smart PC error handling cleanups |

Commit Message

Mario Limonciello Feb. 17, 2024, 1:41 a.m. UTC 
  amd_pmf_init_smart_pc() calls out to amd_pmf_get_bios_buffer() but
the error handling flow doesn't clean everything up allocated
memory.

As amd_pmf_get_bios_buffer() is only called by amd_pmf_init_smart_pc(),
fold it into the function and add labels to clean up any step that
can fail along the way. Explicitly set everything allocated to NULL as
there are other features that may access some of the same variables.

Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Use a single label
 * Move all into amd_pmf_deinit_smart_pc()
 * Set to NULL explicitly
---
 drivers/platform/x86/amd/pmf/tee-if.c | 65 ++++++++++++++++-----------
 1 file changed, 40 insertions(+), 25 deletions(-)

Comments

Hans de Goede Feb. 18, 2024, 2:58 p.m. UTC | #1 
Hi Mario,

Thank you for the quick v2.

On 2/17/24 02:41, Mario Limonciello wrote:
> amd_pmf_init_smart_pc() calls out to amd_pmf_get_bios_buffer() but
> the error handling flow doesn't clean everything up allocated
> memory.
> 
> As amd_pmf_get_bios_buffer() is only called by amd_pmf_init_smart_pc(),
> fold it into the function and add labels to clean up any step that
> can fail along the way. Explicitly set everything allocated to NULL as
> there are other features that may access some of the same variables.
> 
> Fixes: 7c45534afa44 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Use a single label
>  * Move all into amd_pmf_deinit_smart_pc()
>  * Set to NULL explicitly
> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 65 ++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1359ab340f7c..4f74de680654 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -338,25 +338,6 @@ static void amd_pmf_remove_pb(struct amd_pmf_dev *dev) {}
>  static void amd_pmf_hex_dump_pb(struct amd_pmf_dev *dev) {}
>  #endif
>  
> -static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> -{
> -	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> -	if (!dev->policy_buf)
> -		return -ENOMEM;
> -
> -	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
> -	if (!dev->policy_base)
> -		return -ENOMEM;
> -
> -	memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
> -
> -	amd_pmf_hex_dump_pb(dev);
> -	if (pb_side_load)
> -		amd_pmf_open_pb(dev, dev->dbgfs_dir);
> -
> -	return amd_pmf_start_policy_engine(dev);
> -}
> -
>  static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
>  {
>  	return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -455,22 +436,56 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  		return ret;
>  
>  	INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> -	amd_pmf_set_dram_addr(dev, true);
> -	amd_pmf_get_bios_buffer(dev);
> +
> +	ret = amd_pmf_set_dram_addr(dev, true);
> +	if (ret)
> +		goto error;
> +
> +	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
> +	if (!dev->policy_base) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> +	if (!dev->policy_buf) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
> +
> +	amd_pmf_hex_dump_pb(dev);
> +	if (pb_side_load)
> +		amd_pmf_open_pb(dev, dev->dbgfs_dir);

There is a small race here where the debugfs file can be used
to load another policy while amd_pmf_start_policy_engine()
is running. Leading to both the policy-buffer getting modified
underneath the first thread running amd_pmf_start_policy_engine()
and to 2 threads possibly running amd_pmf_start_policy_engine()
at once.

This is a pre-existing problem. Can you post a follow-up
patch (on top of this series) to move the:

	if (pb_side_load)
		amd_pmf_open_pb(dev, dev->dbgfs_dir);

To below the amd_pmf_start_policy_engine() call ?
That avoids the possibility for this race.

I don't expect this to be a problem in real life,
but it would be good to close the race alltogether.

Since this is not a problem with this patch:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for this patch as-is.

Regards,

Hans





> +
>  	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
>  	if (!dev->prev_data)
> -		return -ENOMEM;
> +		goto error;
>  
> -	return dev->smart_pc_enabled;
> +	ret = amd_pmf_start_policy_engine(dev);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	amd_pmf_deinit_smart_pc(dev);
> +
> +	return ret;
>  }
>  
>  void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>  {
> -	if (pb_side_load)
> +	if (pb_side_load && dev->esbin)
>  		amd_pmf_remove_pb(dev);
>  
> +	cancel_delayed_work_sync(&dev->pb_work);
>  	kfree(dev->prev_data);
> +	dev->prev_data = NULL;
>  	kfree(dev->policy_buf);
> -	cancel_delayed_work_sync(&dev->pb_work);
> +	dev->policy_buf = NULL;
> +	kfree(dev->buf);
> +	dev->buf = NULL;
>  	amd_pmf_tee_deinit(dev);
>  }
diff mbox series

Patch

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 1359ab340f7c..4f74de680654 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -338,25 +338,6 @@  static void amd_pmf_remove_pb(struct amd_pmf_dev *dev) {}
 static void amd_pmf_hex_dump_pb(struct amd_pmf_dev *dev) {}
 #endif
 
-static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
-{
-	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
-	if (!dev->policy_buf)
-		return -ENOMEM;
-
-	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
-	if (!dev->policy_base)
-		return -ENOMEM;
-
-	memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
-
-	amd_pmf_hex_dump_pb(dev);
-	if (pb_side_load)
-		amd_pmf_open_pb(dev, dev->dbgfs_dir);
-
-	return amd_pmf_start_policy_engine(dev);
-}
-
 static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
 {
 	return ver->impl_id == TEE_IMPL_ID_AMDTEE;
@@ -455,22 +436,56 @@  int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 		return ret;
 
 	INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
-	amd_pmf_set_dram_addr(dev, true);
-	amd_pmf_get_bios_buffer(dev);
+
+	ret = amd_pmf_set_dram_addr(dev, true);
+	if (ret)
+		goto error;
+
+	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
+	if (!dev->policy_base) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
+	if (!dev->policy_buf) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
+
+	amd_pmf_hex_dump_pb(dev);
+	if (pb_side_load)
+		amd_pmf_open_pb(dev, dev->dbgfs_dir);
+
 	dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
 	if (!dev->prev_data)
-		return -ENOMEM;
+		goto error;
 
-	return dev->smart_pc_enabled;
+	ret = amd_pmf_start_policy_engine(dev);
+	if (ret)
+		goto error;
+
+	return 0;
+
+error:
+	amd_pmf_deinit_smart_pc(dev);
+
+	return ret;
 }
 
 void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
 {
-	if (pb_side_load)
+	if (pb_side_load && dev->esbin)
 		amd_pmf_remove_pb(dev);
 
+	cancel_delayed_work_sync(&dev->pb_work);
 	kfree(dev->prev_data);
+	dev->prev_data = NULL;
 	kfree(dev->policy_buf);
-	cancel_delayed_work_sync(&dev->pb_work);
+	dev->policy_buf = NULL;
+	kfree(dev->buf);
+	dev->buf = NULL;
 	amd_pmf_tee_deinit(dev);
 }