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 >
