[PATCH] Revert "ACPI: custom_method: fix memory leaks"

[PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Kees Cook]

This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.

While /sys/kernel/debug/acpi/custom_method is already a privileged-only
API providing proxied arbitrary write access to kernel memory[1][2],
with existing race conditions[3] in buffer allocation and use that could
lead to memory leaks and use-after-free conditions, the above commit
appears to accidentally make the use-after-free conditions even easier
to accomplish. ("buf" is a global variable and prior kfree()s would set
buf back to NULL.)

This entire interface needs to be reworked (if not entirely removed).

[1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
[2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
[3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/

Cc: Wenwen Wang <wenwen@cs.uga.edu>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/acpi/custom_method.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index 7b54dc95d36b..36d95a02cd30 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -53,10 +53,8 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
 	if ((*ppos > max_size) ||
 	    (*ppos + count > max_size) ||
 	    (*ppos + count < count) ||
-	    (count > uncopied_bytes)) {
-		kfree(buf);
+	    (count > uncopied_bytes))
 		return -EINVAL;
-	}
 
 	if (copy_from_user(buf + (*ppos), user_buf, count)) {
 		kfree(buf);
@@ -76,7 +74,6 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
 		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
 	}
 
-	kfree(buf);
 	return count;
 }
 
-- 
2.25.1

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Greg Kroah-Hartman]

On Sun, May 02, 2021 at 10:23:26AM -0700, Kees Cook wrote:
> This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> 
> While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> API providing proxied arbitrary write access to kernel memory[1][2],
> with existing race conditions[3] in buffer allocation and use that could
> lead to memory leaks and use-after-free conditions, the above commit
> appears to accidentally make the use-after-free conditions even easier
> to accomplish. ("buf" is a global variable and prior kfree()s would set
> buf back to NULL.)
> 
> This entire interface needs to be reworked (if not entirely removed).
> 
> [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> 
> Cc: Wenwen Wang <wenwen@cs.uga.edu>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/acpi/custom_method.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> index 7b54dc95d36b..36d95a02cd30 100644
> --- a/drivers/acpi/custom_method.c
> +++ b/drivers/acpi/custom_method.c
> @@ -53,10 +53,8 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
>  	if ((*ppos > max_size) ||
>  	    (*ppos + count > max_size) ||
>  	    (*ppos + count < count) ||
> -	    (count > uncopied_bytes)) {
> -		kfree(buf);
> +	    (count > uncopied_bytes))
>  		return -EINVAL;
> -	}
>  
>  	if (copy_from_user(buf + (*ppos), user_buf, count)) {
>  		kfree(buf);
> @@ -76,7 +74,6 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
>  		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
>  	}
>  
> -	kfree(buf);
>  	return count;
>  }
>  
> -- 
> 2.25.1
> 

Thanks for the revert, I'll queue it up on my larger "umn.edu reverts"
branch that I'll be sending out for review in a day or so.

greg k-h

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Rafael J. Wysocki]

On Mon, May 3, 2021 at 6:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, May 02, 2021 at 10:23:26AM -0700, Kees Cook wrote:
> > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> >
> > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > API providing proxied arbitrary write access to kernel memory[1][2],
> > with existing race conditions[3] in buffer allocation and use that could
> > lead to memory leaks and use-after-free conditions, the above commit
> > appears to accidentally make the use-after-free conditions even easier
> > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > buf back to NULL.)
> >
> > This entire interface needs to be reworked (if not entirely removed).
> >
> > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> >
> > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  drivers/acpi/custom_method.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> > index 7b54dc95d36b..36d95a02cd30 100644
> > --- a/drivers/acpi/custom_method.c
> > +++ b/drivers/acpi/custom_method.c
> > @@ -53,10 +53,8 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
> >       if ((*ppos > max_size) ||
> >           (*ppos + count > max_size) ||
> >           (*ppos + count < count) ||
> > -         (count > uncopied_bytes)) {
> > -             kfree(buf);
> > +         (count > uncopied_bytes))
> >               return -EINVAL;
> > -     }
> >
> >       if (copy_from_user(buf + (*ppos), user_buf, count)) {
> >               kfree(buf);
> > @@ -76,7 +74,6 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
> >               add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
> >       }
> >
> > -     kfree(buf);
> >       return count;
> >  }
> >
> > --
>
> Thanks for the revert, I'll queue it up on my larger "umn.edu reverts"
> branch that I'll be sending out for review in a day or so.

This will conflict with the material that I'm going to push on
Thursday that includes the two commits mentioned by Mark elsewhere in
this thread.

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Greg Kroah-Hartman]

On Tue, May 04, 2021 at 04:59:40PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 3, 2021 at 6:55 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, May 02, 2021 at 10:23:26AM -0700, Kees Cook wrote:
> > > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> > >
> > > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > > API providing proxied arbitrary write access to kernel memory[1][2],
> > > with existing race conditions[3] in buffer allocation and use that could
> > > lead to memory leaks and use-after-free conditions, the above commit
> > > appears to accidentally make the use-after-free conditions even easier
> > > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > > buf back to NULL.)
> > >
> > > This entire interface needs to be reworked (if not entirely removed).
> > >
> > > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> > >
> > > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  drivers/acpi/custom_method.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> > > index 7b54dc95d36b..36d95a02cd30 100644
> > > --- a/drivers/acpi/custom_method.c
> > > +++ b/drivers/acpi/custom_method.c
> > > @@ -53,10 +53,8 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
> > >       if ((*ppos > max_size) ||
> > >           (*ppos + count > max_size) ||
> > >           (*ppos + count < count) ||
> > > -         (count > uncopied_bytes)) {
> > > -             kfree(buf);
> > > +         (count > uncopied_bytes))
> > >               return -EINVAL;
> > > -     }
> > >
> > >       if (copy_from_user(buf + (*ppos), user_buf, count)) {
> > >               kfree(buf);
> > > @@ -76,7 +74,6 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
> > >               add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, LOCKDEP_NOW_UNRELIABLE);
> > >       }
> > >
> > > -     kfree(buf);
> > >       return count;
> > >  }
> > >
> > > --
> >
> > Thanks for the revert, I'll queue it up on my larger "umn.edu reverts"
> > branch that I'll be sending out for review in a day or so.
> 
> This will conflict with the material that I'm going to push on
> Thursday that includes the two commits mentioned by Mark elsewhere in
> this thread.

I'll drop this from my patch series now that you all have this covered.

thanks!

greg k-h

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Mark Langsdorf]

In 5/2/21 12:23 PM, Kees Cook wrote:
> This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
>
> While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> API providing proxied arbitrary write access to kernel memory[1][2],
> with existing race conditions[3] in buffer allocation and use that could
> lead to memory leaks and use-after-free conditions, the above commit
> appears to accidentally make the use-after-free conditions even easier
> to accomplish. ("buf" is a global variable and prior kfree()s would set
> buf back to NULL.)
>
> This entire interface needs to be reworked (if not entirely removed).
>
> [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
>
> Cc: Wenwen Wang <wenwen@cs.uga.edu>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

I have two patches submitted to linux-acpi to fix the most obvious bugs 
in the current driver.  I don't think that just reverting this patch in 
its entirety is a good solution: it still leaves the buf allocated in 
-EINVAL, as well as the weird case where a not fully consumed buffer can 
be reallocated without being freed on a subsequent call.

https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/

https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/

I support rewriting this driver in its entirety, but reverting one bad 
patch to leave it in a different buggy state is less than ideal.

--Mark Langsdorf

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
> In 5/2/21 12:23 PM, Kees Cook wrote:
> > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> > 
> > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > API providing proxied arbitrary write access to kernel memory[1][2],
> > with existing race conditions[3] in buffer allocation and use that could
> > lead to memory leaks and use-after-free conditions, the above commit
> > appears to accidentally make the use-after-free conditions even easier
> > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > buf back to NULL.)
> > 
> > This entire interface needs to be reworked (if not entirely removed).
> > 
> > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> > 
> > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> I have two patches submitted to linux-acpi to fix the most obvious bugs in
> the current driver.  I don't think that just reverting this patch in its
> entirety is a good solution: it still leaves the buf allocated in -EINVAL,
> as well as the weird case where a not fully consumed buffer can be
> reallocated without being freed on a subsequent call.
> 
> https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/
> 
> https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/
> 
> I support rewriting this driver in its entirety, but reverting one bad patch
> to leave it in a different buggy state is less than ideal.

It's buggy now, and root-only, so it's a low bar at the moment :)

Do those commits really fix the issues?  Is this debugfs code even
needed at all or can it just be dropped?

thanks,

greg k-h

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Mark Langsdorf]

On 5/3/21 9:51 AM, Greg Kroah-Hartman wrote:
> On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
>> In 5/2/21 12:23 PM, Kees Cook wrote:
>>> This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
>>>
>>> While /sys/kernel/debug/acpi/custom_method is already a privileged-only
>>> API providing proxied arbitrary write access to kernel memory[1][2],
>>> with existing race conditions[3] in buffer allocation and use that could
>>> lead to memory leaks and use-after-free conditions, the above commit
>>> appears to accidentally make the use-after-free conditions even easier
>>> to accomplish. ("buf" is a global variable and prior kfree()s would set
>>> buf back to NULL.)
>>>
>>> This entire interface needs to be reworked (if not entirely removed).
>>>
>>> [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
>>> [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
>>> [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
>>>
>>> Cc: Wenwen Wang <wenwen@cs.uga.edu>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>> I have two patches submitted to linux-acpi to fix the most obvious bugs in
>> the current driver.  I don't think that just reverting this patch in its
>> entirety is a good solution: it still leaves the buf allocated in -EINVAL,
>> as well as the weird case where a not fully consumed buffer can be
>> reallocated without being freed on a subsequent call.
>>
>> https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/
>>
>> https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/
>>
>> I support rewriting this driver in its entirety, but reverting one bad patch
>> to leave it in a different buggy state is less than ideal.
> It's buggy now, and root-only, so it's a low bar at the moment :)
>
> Do those commits really fix the issues?  Is this debugfs code even
> needed at all or can it just be dropped?

One of my commits removes the kfree(buf) at the end of the function, 
which is the code that causes the use after free for short writes.  The 
other adds a kfree(buf) before allocating the buffer, to make sure that 
the buffer is free before allocating it.

There are other bugs in the code that neither my patches nor the revert 
address, like the total lack of protection against concurrent writes.

--Mark Langsdorf

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Greg Kroah-Hartman]

On Mon, May 03, 2021 at 09:58:17AM -0500, Mark Langsdorf wrote:
> On 5/3/21 9:51 AM, Greg Kroah-Hartman wrote:
> > On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
> > > In 5/2/21 12:23 PM, Kees Cook wrote:
> > > > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> > > > 
> > > > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > > > API providing proxied arbitrary write access to kernel memory[1][2],
> > > > with existing race conditions[3] in buffer allocation and use that could
> > > > lead to memory leaks and use-after-free conditions, the above commit
> > > > appears to accidentally make the use-after-free conditions even easier
> > > > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > > > buf back to NULL.)
> > > > 
> > > > This entire interface needs to be reworked (if not entirely removed).
> > > > 
> > > > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > > > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > > > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> > > > 
> > > > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > I have two patches submitted to linux-acpi to fix the most obvious bugs in
> > > the current driver.  I don't think that just reverting this patch in its
> > > entirety is a good solution: it still leaves the buf allocated in -EINVAL,
> > > as well as the weird case where a not fully consumed buffer can be
> > > reallocated without being freed on a subsequent call.
> > > 
> > > https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/
> > > 
> > > https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/
> > > 
> > > I support rewriting this driver in its entirety, but reverting one bad patch
> > > to leave it in a different buggy state is less than ideal.
> > It's buggy now, and root-only, so it's a low bar at the moment :)
> > 
> > Do those commits really fix the issues?  Is this debugfs code even
> > needed at all or can it just be dropped?
> 
> One of my commits removes the kfree(buf) at the end of the function, which
> is the code that causes the use after free for short writes.  The other adds
> a kfree(buf) before allocating the buffer, to make sure that the buffer is
> free before allocating it.
> 
> There are other bugs in the code that neither my patches nor the revert
> address, like the total lack of protection against concurrent writes.

Why would anyone care about concurrent writes for this debugfs file?
Is that a requirement here?

thanks,

greg k-h

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Rafael J. Wysocki]

On Mon, May 3, 2021 at 4:51 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
> > In 5/2/21 12:23 PM, Kees Cook wrote:
> > > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> > >
> > > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > > API providing proxied arbitrary write access to kernel memory[1][2],
> > > with existing race conditions[3] in buffer allocation and use that could
> > > lead to memory leaks and use-after-free conditions, the above commit
> > > appears to accidentally make the use-after-free conditions even easier
> > > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > > buf back to NULL.)
> > >
> > > This entire interface needs to be reworked (if not entirely removed).
> > >
> > > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> > >
> > > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> >
> > I have two patches submitted to linux-acpi to fix the most obvious bugs in
> > the current driver.  I don't think that just reverting this patch in its
> > entirety is a good solution: it still leaves the buf allocated in -EINVAL,
> > as well as the weird case where a not fully consumed buffer can be
> > reallocated without being freed on a subsequent call.
> >
> > https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/
> >
> > https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/
> >
> > I support rewriting this driver in its entirety, but reverting one bad patch
> > to leave it in a different buggy state is less than ideal.
>
> It's buggy now, and root-only, so it's a low bar at the moment :)

So dropping it completely may be a better choice.

IMO let's let the Mark's commits go in now and we'll see later.

Thanks!

Re: [PATCH] Revert "ACPI: custom_method: fix memory leaks"

[Kees Cook]

On Mon, May 03, 2021 at 08:17:14AM -0500, Mark Langsdorf wrote:
> In 5/2/21 12:23 PM, Kees Cook wrote:
> > This reverts commit 03d1571d9513369c17e6848476763ebbd10ec2cb.
> > 
> > While /sys/kernel/debug/acpi/custom_method is already a privileged-only
> > API providing proxied arbitrary write access to kernel memory[1][2],
> > with existing race conditions[3] in buffer allocation and use that could
> > lead to memory leaks and use-after-free conditions, the above commit
> > appears to accidentally make the use-after-free conditions even easier
> > to accomplish. ("buf" is a global variable and prior kfree()s would set
> > buf back to NULL.)
> > 
> > This entire interface needs to be reworked (if not entirely removed).
> > 
> > [1] https://lore.kernel.org/lkml/20110222193250.GA23913@outflux.net/
> > [2] https://lore.kernel.org/lkml/201906221659.B618D83@keescook/
> > [3] https://lore.kernel.org/lkml/20170109231323.GA89642@beast/
> > 
> > Cc: Wenwen Wang <wenwen@cs.uga.edu>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> I have two patches submitted to linux-acpi to fix the most obvious bugs in
> the current driver.  I don't think that just reverting this patch in its
> entirety is a good solution: it still leaves the buf allocated in -EINVAL,
> as well as the weird case where a not fully consumed buffer can be
> reallocated without being freed on a subsequent call.
> 
> https://lore.kernel.org/linux-acpi/20210427185434.34885-1-mlangsdo@redhat.com/
> 
> https://lore.kernel.org/linux-acpi/20210423152818.97077-1-mlangsdo@redhat.com/
> 
> I support rewriting this driver in its entirety, but reverting one bad patch
> to leave it in a different buggy state is less than ideal.

Thanks for working on that! It'd be nice if there was a lock held for
the duration of the "open", then all the concurrency races would go
away. But, I haven't spent a lot of time looking since it's root-only
and already blocked by lockdown, etc.

-- 
Kees Cook