Hi,
On 12/12/23 02:47, Shyam Sundar S K wrote:
> A policy binary is OS agnostic, and the same policies are expected to work
> across the OSes. At times it becomes difficult to debug when the policies
> inside the policy binaries starts to misbehave. Add a way to sideload such
> policies independently to debug them via a debugfs entry.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 1 +
> drivers/platform/x86/amd/pmf/tee-if.c | 60 +++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 55cd2b301bbb..16999c5b334f 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -219,6 +219,7 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + struct dentry *esbin;
> unsigned char *policy_buf;
> u32 policy_sz;
> struct tee_context *tee_ctx;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 38b75198cc3f..cf95251741c7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -8,6 +8,7 @@
> * Author: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> */
>
> +#include <linux/debugfs.h>
> #include <linux/tee_drv.h>
> #include <linux/uuid.h>
> #include "pmf.h"
> @@ -16,9 +17,14 @@
>
> /* Policy binary actions sampling frequency (in ms) */
> static int pb_actions_ms = MSEC_PER_SEC;
> +/* Sideload policy binaries to debug policy failures */
> +static bool pb_side_load;
> +
> #ifdef CONFIG_AMD_PMF_DEBUG
> module_param(pb_actions_ms, int, 0644);
> MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
> +module_param(pb_side_load, bool, 0444);
> +MODULE_PARM_DESC(pb_side_load, "Sideload policy binaries debug policy failures");
> #endif
>
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> @@ -269,6 +275,54 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
> return 0;
> }
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
> + size_t length, loff_t *pos)
> +{
> + struct amd_pmf_dev *dev = filp->private_data;
> + int ret;
> +
> + /* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> + if (length > POLICY_BUF_MAX_SZ || length == 0)
> + return -EINVAL;
> +
> + dev->policy_sz = length;
> +
> + /* re-alloc to the new buffer length of the policy binary */
You are leaking the original policy_buf here. Also by
storing length + the kzalloc result in policy_sz + policy_buf
before a successful kzalloc() + copy_from_usr() you are
leaving things in a state where there is no valid policy_buf
on error exits, where as other code assumes there always
is a valid policy_buf.
I have squashed in the following fix to fix both these issues:
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 9a4757f4f521..502ce93d5cdd 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -286,22 +286,25 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
size_t length, loff_t *pos)
{
struct amd_pmf_dev *dev = filp->private_data;
+ unsigned char *new_policy_buf;
int ret;
/* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
if (length > POLICY_BUF_MAX_SZ || length == 0)
return -EINVAL;
- dev->policy_sz = length;
-
/* re-alloc to the new buffer length of the policy binary */
- dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
- if (!dev->policy_buf)
+ new_policy_buf = kzalloc(length, GFP_KERNEL);
+ if (!new_policy_buf)
return -ENOMEM;
- if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
+ if (copy_from_user(new_policy_buf, buf, length))
return -EFAULT;
+ kfree(dev->policy_buf);
+ dev->policy_buf = new_policy_buf;
+ dev->policy_sz = length;
+
amd_pmf_hex_dump_pb(dev);
ret = amd_pmf_start_policy_engine(dev);
if (ret)
Regards,
Hans
> + dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> + if (!dev->policy_buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
> + return -EFAULT;
> +
> + ret = amd_pmf_start_policy_engine(dev);
> + if (ret)
> + return -EINVAL;
> +
> + return length;
> +}
> +
> +static const struct file_operations pb_fops = {
> + .write = amd_pmf_get_pb_data,
> + .open = simple_open,
> +};
> +
> +static void amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
> +{
> + dev->esbin = debugfs_create_dir("pb", debugfs_root);
> + debugfs_create_file("update_policy", 0644, dev->esbin, dev, &pb_fops);
> +}
> +
> +static void amd_pmf_remove_pb(struct amd_pmf_dev *dev)
> +{
> + debugfs_remove_recursive(dev->esbin);
> +}
> +#else
> +static void amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root) {}
> +static void amd_pmf_remove_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);
> @@ -281,6 +335,9 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
>
> memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
>
> + if (pb_side_load)
> + amd_pmf_open_pb(dev, dev->dbgfs_dir);
> +
> return amd_pmf_start_policy_engine(dev);
> }
>
> @@ -393,6 +450,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> + if (pb_side_load)
> + amd_pmf_remove_pb(dev);
> +
> kfree(dev->prev_data);
> kfree(dev->policy_buf);
> cancel_delayed_work_sync(&dev->pb_work);
