[PATCH] ACPI: custom_method: fix potential use-after-free issues

[PATCH] ACPI: custom_method: fix potential use-after-free issues

[Hang Zhang]

cm_write() is the .write callback of the custom_method debugfs
interface, it operates on a global pointer "buf" (e.g., dereference,
allocate, free, and nullification), the problem is that cm_write()
is not protected by any locks, so concurrent invocations of it
may cause use-after-free issues for "buf", e.g., one invocation
may have just freed "buf" while being preempted before nullifying
the pointer, then another invocation can dereference the now dangling
"buf" pointer.

Fix the issue by protecting the "buf" operations in cm_write() with
the inode write lock. Note that the .llseek callback of the debugfs
interface has been protected by the same lock, this patch basically
introduces it to the .write callback as well.

Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
---
 drivers/acpi/custom_method.c | 43 +++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index d39a9b474727..e3de5a06d903 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -29,28 +29,38 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
 	struct acpi_table_header table;
 	acpi_status status;
 	int ret;
+	struct inode *inode = file_inode(file);
 
 	ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
 	if (ret)
 		return ret;
 
+	inode_lock(inode);
 	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)) {
+			ret = -EINVAL;
+			goto err;
+		}
 		if (copy_from_user(&table, user_buf,
-				   sizeof(struct acpi_table_header)))
-			return -EFAULT;
+				   sizeof(struct acpi_table_header))) {
+			ret = -EFAULT;
+			goto err;
+		}
 		uncopied_bytes = max_size = table.length;
 		/* make sure the buf is not allocated */
 		kfree(buf);
 		buf = kzalloc(max_size, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
+		if (!buf) {
+			ret = -ENOMEM;
+			goto err;
+		}
 	}
 
-	if (buf == NULL)
-		return -EINVAL;
+	if (buf == NULL) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	if ((*ppos > max_size) ||
 	    (*ppos + count > max_size) ||
@@ -58,13 +68,15 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
 	    (count > uncopied_bytes)) {
 		kfree(buf);
 		buf = NULL;
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	if (copy_from_user(buf + (*ppos), user_buf, count)) {
 		kfree(buf);
 		buf = NULL;
-		return -EFAULT;
+		ret = -EFAULT;
+		goto err;
 	}
 
 	uncopied_bytes -= count;
@@ -74,12 +86,17 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
 		status = acpi_install_method(buf);
 		kfree(buf);
 		buf = NULL;
-		if (ACPI_FAILURE(status))
-			return -EINVAL;
+		if (ACPI_FAILURE(status)) {
+			ret = -EINVAL;
+			goto err;
+		}
 		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
 	}
 
-	return count;
+	ret = count;
+err:
+	inode_unlock(inode);
+	return ret;
 }
 
 static const struct file_operations cm_fops = {
-- 
2.39.0

Re: [PATCH] ACPI: custom_method: fix potential use-after-free issues

[Rafael J. Wysocki]

On Tue, Dec 27, 2022 at 7:34 AM Hang Zhang <zh.nvgt@gmail.com> wrote:
>
> cm_write() is the .write callback of the custom_method debugfs
> interface, it operates on a global pointer "buf" (e.g., dereference,
> allocate, free, and nullification), the problem is that cm_write()
> is not protected by any locks, so concurrent invocations of it
> may cause use-after-free issues for "buf", e.g., one invocation
> may have just freed "buf" while being preempted before nullifying
> the pointer, then another invocation can dereference the now dangling
> "buf" pointer.
>
> Fix the issue by protecting the "buf" operations in cm_write() with
> the inode write lock. Note that the .llseek callback of the debugfs
> interface has been protected by the same lock, this patch basically
> introduces it to the .write callback as well.

The problem is there, but the whole state is not protected from
concurrent use and the fix doesn't look sufficient to me (for example,
a different writer may start writing into the file before the previous
one has finished and the result will still be broken AFAICS).

It looks like the file should be prevented from being opened by more
than one writer at a time.

Or maybe it's time to drop this interface from the kernel altogether.

> Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> ---
>  drivers/acpi/custom_method.c | 43 +++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> index d39a9b474727..e3de5a06d903 100644
> --- a/drivers/acpi/custom_method.c
> +++ b/drivers/acpi/custom_method.c
> @@ -29,28 +29,38 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
>         struct acpi_table_header table;
>         acpi_status status;
>         int ret;
> +       struct inode *inode = file_inode(file);
>
>         ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
>         if (ret)
>                 return ret;
>
> +       inode_lock(inode);
>         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)) {
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
>                 if (copy_from_user(&table, user_buf,
> -                                  sizeof(struct acpi_table_header)))
> -                       return -EFAULT;
> +                                  sizeof(struct acpi_table_header))) {
> +                       ret = -EFAULT;
> +                       goto err;
> +               }
>                 uncopied_bytes = max_size = table.length;
>                 /* make sure the buf is not allocated */
>                 kfree(buf);
>                 buf = kzalloc(max_size, GFP_KERNEL);
> -               if (!buf)
> -                       return -ENOMEM;
> +               if (!buf) {
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
>         }
>
> -       if (buf == NULL)
> -               return -EINVAL;
> +       if (buf == NULL) {
> +               ret = -EINVAL;
> +               goto err;
> +       }
>
>         if ((*ppos > max_size) ||
>             (*ppos + count > max_size) ||
> @@ -58,13 +68,15 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
>             (count > uncopied_bytes)) {
>                 kfree(buf);
>                 buf = NULL;
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err;
>         }
>
>         if (copy_from_user(buf + (*ppos), user_buf, count)) {
>                 kfree(buf);
>                 buf = NULL;
> -               return -EFAULT;
> +               ret = -EFAULT;
> +               goto err;
>         }
>
>         uncopied_bytes -= count;
> @@ -74,12 +86,17 @@ static ssize_t cm_write(struct file *file, const char __user *user_buf,
>                 status = acpi_install_method(buf);
>                 kfree(buf);
>                 buf = NULL;
> -               if (ACPI_FAILURE(status))
> -                       return -EINVAL;
> +               if (ACPI_FAILURE(status)) {
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
>                 add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
>         }
>
> -       return count;
> +       ret = count;
> +err:
> +       inode_unlock(inode);
> +       return ret;
>  }
>
>  static const struct file_operations cm_fops = {
> --
> 2.39.0
>

Re: [PATCH] ACPI: custom_method: fix potential use-after-free issues

[Hang Zhang]

On Fri, Dec 30, 2022 at 1:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Dec 27, 2022 at 7:34 AM Hang Zhang <zh.nvgt@gmail.com> wrote:
> >
> > cm_write() is the .write callback of the custom_method debugfs
> > interface, it operates on a global pointer "buf" (e.g., dereference,
> > allocate, free, and nullification), the problem is that cm_write()
> > is not protected by any locks, so concurrent invocations of it
> > may cause use-after-free issues for "buf", e.g., one invocation
> > may have just freed "buf" while being preempted before nullifying
> > the pointer, then another invocation can dereference the now dangling
> > "buf" pointer.
> >
> > Fix the issue by protecting the "buf" operations in cm_write() with
> > the inode write lock. Note that the .llseek callback of the debugfs
> > interface has been protected by the same lock, this patch basically
> > introduces it to the .write callback as well.
>
> The problem is there, but the whole state is not protected from
> concurrent use and the fix doesn't look sufficient to me (for example,
> a different writer may start writing into the file before the previous
> one has finished and the result will still be broken AFAICS).
>
> It looks like the file should be prevented from being opened by more
> than one writer at a time.
>
> Or maybe it's time to drop this interface from the kernel altogether.

Hi, Rafael,

Thank you very much for your feedback! We initially intended to bring
up this potential concurrent UAF issue to the community with this
tentative patch, but we do not have deep domain knowledge for the
ACPI subsystem and the bigger picture, so your comment is highly
valuable to us!

As far as I can understand, inode_lock is uniquely associated with
the opened file, e.g., if two writers open the same debugfs file
and write to it, then inode_lock as used in this patch should be
able to synchronize their concurrent write because their inode_lock
are on the same semaphore. Do you mean that this "custom_method"
driver will handle the .open/.write of multiple different debugfs
file instances, so that writers accessing different file instances
will not be properly synchronized with inode_lock? Sorry if I missed
anything here, and thank you in advance for your explanation!

I also think it is a good solution to totally drop this interface
if the maintainers consider it appropriate (I do not have
the knowledge to assess the role this interface plays, though).
Thank you for your help!

Re: [PATCH] ACPI: custom_method: fix potential use-after-free issues

[Zhang, Rui]

On Fri, 2022-12-30 at 19:31 +0100, Rafael J. Wysocki wrote:
> On Tue, Dec 27, 2022 at 7:34 AM Hang Zhang <zh.nvgt@gmail.com> wrote:
> > cm_write() is the .write callback of the custom_method debugfs
> > interface, it operates on a global pointer "buf" (e.g.,
> > dereference,
> > allocate, free, and nullification), the problem is that cm_write()
> > is not protected by any locks, so concurrent invocations of it
> > may cause use-after-free issues for "buf", e.g., one invocation
> > may have just freed "buf" while being preempted before nullifying
> > the pointer, then another invocation can dereference the now
> > dangling
> > "buf" pointer.
> > 
> > Fix the issue by protecting the "buf" operations in cm_write() with
> > the inode write lock. Note that the .llseek callback of the debugfs
> > interface has been protected by the same lock, this patch basically
> > introduces it to the .write callback as well.
> 
> The problem is there, but the whole state is not protected from
> concurrent use and the fix doesn't look sufficient to me (for
> example,
> a different writer may start writing into the file before the
> previous
> one has finished and the result will still be broken AFAICS).
> 
> It looks like the file should be prevented from being opened by more
> than one writer at a time.
> 
> Or maybe it's time to drop this interface from the kernel altogether.
> 
I still use this interface for debugging AML issues occasionally. Say,
dumping the value of some key objects to see the AML code path.

I'm not sure if there is any alternative way to do this, especially in
remote debug case. (This can be done via DSDT override, but not all
users have the knowledge of building a customized kernel)

If this is not a problem, then I think it is safe to remove this
interface because I suspect I am the only user of this interface.
Because there are some special tricks I got from Erik, to make it fully
work after some certain ACPICA release. And this is not documented in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/firmware-guide/acpi/method-customizing.rst#n58
Say, to generate the AML code of the method, we need to
1. compile the table with external declarations.
2. If step 1 compiles successfully, remove the external declarations
from the table and compile with -f.

thanks,
rui

> > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> > ---
> >  drivers/acpi/custom_method.c | 43 +++++++++++++++++++++++++-------
> > ----
> >  1 file changed, 30 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/acpi/custom_method.c
> > b/drivers/acpi/custom_method.c
> > index d39a9b474727..e3de5a06d903 100644
> > --- a/drivers/acpi/custom_method.c
> > +++ b/drivers/acpi/custom_method.c
> > @@ -29,28 +29,38 @@ static ssize_t cm_write(struct file *file,
> > const char __user *user_buf,
> >         struct acpi_table_header table;
> >         acpi_status status;
> >         int ret;
> > +       struct inode *inode = file_inode(file);
> > 
> >         ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
> >         if (ret)
> >                 return ret;
> > 
> > +       inode_lock(inode);
> >         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)) {
> > +                       ret = -EINVAL;
> > +                       goto err;
> > +               }
> >                 if (copy_from_user(&table, user_buf,
> > -                                  sizeof(struct
> > acpi_table_header)))
> > -                       return -EFAULT;
> > +                                  sizeof(struct
> > acpi_table_header))) {
> > +                       ret = -EFAULT;
> > +                       goto err;
> > +               }
> >                 uncopied_bytes = max_size = table.length;
> >                 /* make sure the buf is not allocated */
> >                 kfree(buf);
> >                 buf = kzalloc(max_size, GFP_KERNEL);
> > -               if (!buf)
> > -                       return -ENOMEM;
> > +               if (!buf) {
> > +                       ret = -ENOMEM;
> > +                       goto err;
> > +               }
> >         }
> > 
> > -       if (buf == NULL)
> > -               return -EINVAL;
> > +       if (buf == NULL) {
> > +               ret = -EINVAL;
> > +               goto err;
> > +       }
> > 
> >         if ((*ppos > max_size) ||
> >             (*ppos + count > max_size) ||
> > @@ -58,13 +68,15 @@ static ssize_t cm_write(struct file *file,
> > const char __user *user_buf,
> >             (count > uncopied_bytes)) {
> >                 kfree(buf);
> >                 buf = NULL;
> > -               return -EINVAL;
> > +               ret = -EINVAL;
> > +               goto err;
> >         }
> > 
> >         if (copy_from_user(buf + (*ppos), user_buf, count)) {
> >                 kfree(buf);
> >                 buf = NULL;
> > -               return -EFAULT;
> > +               ret = -EFAULT;
> > +               goto err;
> >         }
> > 
> >         uncopied_bytes -= count;
> > @@ -74,12 +86,17 @@ static ssize_t cm_write(struct file *file,
> > const char __user *user_buf,
> >                 status = acpi_install_method(buf);
> >                 kfree(buf);
> >                 buf = NULL;
> > -               if (ACPI_FAILURE(status))
> > -                       return -EINVAL;
> > +               if (ACPI_FAILURE(status)) {
> > +                       ret = -EINVAL;
> > +                       goto err;
> > +               }
> >                 add_taint(TAINT_OVERRIDDEN_ACPI_TABLE,
> > LOCKDEP_NOW_UNRELIABLE);
> >         }
> > 
> > -       return count;
> > +       ret = count;
> > +err:
> > +       inode_unlock(inode);
> > +       return ret;
> >  }
> > 
> >  static const struct file_operations cm_fops = {
> > --
> > 2.39.0
> > 

Re: [PATCH] ACPI: custom_method: fix potential use-after-free issues

[Rafael J. Wysocki]

On Mon, Jan 2, 2023 at 2:42 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Fri, 2022-12-30 at 19:31 +0100, Rafael J. Wysocki wrote:
> > On Tue, Dec 27, 2022 at 7:34 AM Hang Zhang <zh.nvgt@gmail.com> wrote:
> > > cm_write() is the .write callback of the custom_method debugfs
> > > interface, it operates on a global pointer "buf" (e.g.,
> > > dereference,
> > > allocate, free, and nullification), the problem is that cm_write()
> > > is not protected by any locks, so concurrent invocations of it
> > > may cause use-after-free issues for "buf", e.g., one invocation
> > > may have just freed "buf" while being preempted before nullifying
> > > the pointer, then another invocation can dereference the now
> > > dangling
> > > "buf" pointer.
> > >
> > > Fix the issue by protecting the "buf" operations in cm_write() with
> > > the inode write lock. Note that the .llseek callback of the debugfs
> > > interface has been protected by the same lock, this patch basically
> > > introduces it to the .write callback as well.
> >
> > The problem is there, but the whole state is not protected from
> > concurrent use and the fix doesn't look sufficient to me (for
> > example,
> > a different writer may start writing into the file before the
> > previous
> > one has finished and the result will still be broken AFAICS).
> >
> > It looks like the file should be prevented from being opened by more
> > than one writer at a time.
> >
> > Or maybe it's time to drop this interface from the kernel altogether.
> >
> I still use this interface for debugging AML issues occasionally. Say,
> dumping the value of some key objects to see the AML code path.
>
> I'm not sure if there is any alternative way to do this, especially in
> remote debug case. (This can be done via DSDT override, but not all
> users have the knowledge of building a customized kernel)
>
> If this is not a problem, then I think it is safe to remove this
> interface because I suspect I am the only user of this interface.
> Because there are some special tricks I got from Erik, to make it fully
> work after some certain ACPICA release. And this is not documented in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/firmware-guide/acpi/method-customizing.rst#n58
> Say, to generate the AML code of the method, we need to
> 1. compile the table with external declarations.
> 2. If step 1 compiles successfully, remove the external declarations
> from the table and compile with -f.

Interesting.

This basically means that ACPICA broke backwards compatibility with
this interface at one point and it's been necessary to work around
that manually since then and nobody cared to update the documentation.

Oh well.

Let me send a patch to remove it then and we'll see what happens.