Re: [PATCH] ACPI: Make custom_method use per-open state

[Rafael J. Wysocki]

On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <pedro.falcato@xxxxxxxxx> wrote:
>
> Make custom_method keep its own per-file-open state instead of global
> state in order to avoid race conditions[1] and other possible conflicts
> with other concurrent users.
>
> Link: https://lore.kernel.org/linux-acpi/20221227063335.61474-1-zh.nvgt@xxxxxxxxx/ # [1]
> Reported-by: Hang Zhang <zh.nvgt@xxxxxxxxx>
> Cc: Swift Geek <swiftgeek@xxxxxxxxx>
> Signed-off-by: Pedro Falcato <pedro.falcato@xxxxxxxxx>
> ---
>  This patch addresses Hang's problems plus the ones raised by Rafael in his review (see link above).
>  https://lore.kernel.org/lkml/2667007.mvXUDI8C0e@kreacher/ was submitted but since there were still people
>  that wanted this feature, I took my time to write up a patch that should fix the issues.
>  Hopefully the linux-acpi maintainers have not decided to remove custom_method just yet.

Well, thanks for the patch, but yes, they have.  Sorry.

>  drivers/acpi/custom_method.c | 119 +++++++++++++++++++++++++++--------
>  1 file changed, 92 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> index d39a9b47472..034fb14f118 100644
> --- a/drivers/acpi/custom_method.c
> +++ b/drivers/acpi/custom_method.c
> @@ -17,73 +17,138 @@ MODULE_LICENSE("GPL");
>
>  static struct dentry *cm_dentry;
>
> +struct custom_method_state {
> +       char *buf;
> +       u32 max_size;
> +       u32 uncopied_bytes;
> +       struct mutex lock;
> +};
> +
> +static int cm_open(struct inode *inode, struct file *file)
> +{
> +       struct custom_method_state *state;
> +
> +       state = kzalloc(sizeof(struct custom_method_state), GFP_KERNEL);
> +
> +       if (!state)
> +               return -ENOMEM;
> +
> +       file->private_data = state;
> +       mutex_init(&state->lock);
> +
> +       return 0;
> +}
> +
> +static int cm_release(struct inode *inode, struct file *file)
> +{
> +       struct custom_method_state *state;
> +
> +       state = file->private_data;
> +
> +       mutex_destroy(&state->lock);
> +
> +       /* Make sure the buf gets freed */
> +       kfree(state->buf);
> +
> +       kfree(state);
> +       return 0;
> +}
> +
>  /* /sys/kernel/debug/acpi/custom_method */
>
>  static ssize_t cm_write(struct file *file, const char __user *user_buf,
>                         size_t count, loff_t *ppos)
>  {
> -       static char *buf;
> -       static u32 max_size;
> -       static u32 uncopied_bytes;
> +       struct custom_method_state *state;
> +       char *buf;
>
>         struct acpi_table_header table;
>         acpi_status status;
>         int ret;
>
> +       state = file->private_data;
> +       buf = state->buf;
> +
>         ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
>         if (ret)
>                 return ret;
>
> +       mutex_lock(&state->lock);
> +
>         if (!(*ppos)) {
>                 /* parse the table header to get the table length */
> -               if (count <= sizeof(struct acpi_table_header))
> -                       return -EINVAL;
> +               if (count <= sizeof(struct acpi_table_header)) {
> +                       count = -EINVAL;
> +                       goto out;
> +               }
> +
>                 if (copy_from_user(&table, user_buf,
> -                                  sizeof(struct acpi_table_header)))
> -                       return -EFAULT;
> -               uncopied_bytes = max_size = table.length;
> +                                  sizeof(struct acpi_table_header))) {
> +                       count = -EFAULT;
> +                       goto out;
> +               }
> +
> +               state->uncopied_bytes = state->max_size = table.length;
>                 /* make sure the buf is not allocated */
>                 kfree(buf);
> -               buf = kzalloc(max_size, GFP_KERNEL);
> -               if (!buf)
> -                       return -ENOMEM;
> +               buf = state->buf = kzalloc(state->max_size, GFP_KERNEL);
> +               if (!buf) {
> +                       count = -ENOMEM;
> +                       goto out;
> +               }
>         }
>
> -       if (buf == NULL)
> -               return -EINVAL;
> +       /* Check if someone seeked ahead or if we errored out
> +        * (buf will be NULL)
> +        */
> +       if (buf == NULL) {
> +               count = -EINVAL;
> +               goto out;
> +       }
>
> -       if ((*ppos > max_size) ||
> -           (*ppos + count > max_size) ||
> +       if ((*ppos > state->max_size) ||
> +           (*ppos + count > state->max_size) ||
>             (*ppos + count < count) ||
> -           (count > uncopied_bytes)) {
> -               kfree(buf);
> -               buf = NULL;
> -               return -EINVAL;
> +           (count > state->uncopied_bytes)) {
> +               count = -EINVAL;
> +               goto err_free;
>         }
>
>         if (copy_from_user(buf + (*ppos), user_buf, count)) {
> -               kfree(buf);
> -               buf = NULL;
> -               return -EFAULT;
> +               count = -EFAULT;
> +               goto err_free;
>         }
>
> -       uncopied_bytes -= count;
> +       state->uncopied_bytes -= count;
>         *ppos += count;
>
> -       if (!uncopied_bytes) {
> +       if (!state->uncopied_bytes) {
>                 status = acpi_install_method(buf);
>                 kfree(buf);
> -               buf = NULL;
> -               if (ACPI_FAILURE(status))
> -                       return -EINVAL;
> +               state->buf = NULL;
> +
> +               if (ACPI_FAILURE(status)) {
> +                       count = -EINVAL;
> +                       goto out;
> +               }
> +
>                 add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
>         }
>
> +out:
> +       mutex_unlock(&state->lock);
> +       return count;
> +err_free:
> +       mutex_unlock(&state->lock);
> +       kfree(buf);
> +       state->buf = NULL;
>         return count;
>  }
>
>  static const struct file_operations cm_fops = {
>         .write = cm_write,
> +       .open = cm_open,
> +       .release = cm_release,
>         .llseek = default_llseek,
>  };
>
> --
> 2.39.0
>