| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0iXcRFamA+mE837=zHReBT-+8WmMeRDR7L9R+FVpLr25A@mail.gmail.com>
Date: Wed, 1 Feb 2023 19:34:48 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Pedro Falcato <pedro.falcato@...il.com>
Cc: linux-acpi@...r.kernel.org,
"Rafael J . Wysocki" <rafael@...nel.org>,
Len Brown <lenb@...nel.org>, linux-kernel@...r.kernel.org,
rui.zhang@...el.com, Hang Zhang <zh.nvgt@...il.com>,
Swift Geek <swiftgeek@...il.com>
Subject: Re: [PATCH] ACPI: Make custom_method use per-open state
On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <pedro.falcato@...il.com> 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@gmail.com/ # [1]
> Reported-by: Hang Zhang <zh.nvgt@...il.com>
> Cc: Swift Geek <swiftgeek@...il.com>
> Signed-off-by: Pedro Falcato <pedro.falcato@...il.com>
> ---
> 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
>
Powered by blists - more mailing lists