ACPICA: Fix use-after-free in acpi_ps_parse_aml()

  KASAN reports a use-after-free problem and causes kernel panic
triggered by: modprobe acpiphp_ibm

BUG: KASAN:
use-after-free in acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
Read of size 8 at addr ffff888002f843f0 by task modprobe/519

CPU: 2 PID: 519 Comm: modprobe Not tainted 6.0.0+
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
    Call Trace:
    <TASK>
    acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
    acpi_ds_method_error (drivers/acpi/acpica/dsmethod.c:232)
    acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
    ...
    </TASK>

    Allocated by task 519:
    ...
    __kasan_kmalloc (mm/kasan/common.c:526)
    acpi_ds_create_walk_state (drivers/acpi/acpica/dswstate.c:519)
    acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:498)
    acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
    ...

    Freed by task 519:
    ...
    __kmem_cache_free+0xb6/0x3c0
    acpi_ds_delete_walk_state (drivers/acpi/acpica/dswstate.c:722)
    acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:586)
    acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
    ...
---[ end Kernel panic - not syncing: Fatal exception ]---

In the error path in acpi_ps_parse_aml():

    acpi_ds_call_control_method()
        acpi_ds_create_walk_state()
            acpi_ds_push_walk_state()
	    # thread->walk_state_list = walk_state

        acpi_ds_init_aml_walk # *fail*
        goto cleanup:
        acpi_ds_delete_walk_state() # ACPI_FREE(walk_state)

    acpi_ds_method_error()
        acpi_ds_dump_method_stack()
        # using freed thread->walk_state_list

Briefly, the walk_state is pushed to thread, and freed without being poped.
Then it is used in acpi_ds_dump_method_stack() and causes use-after-free.

Add acpi_ds_pop_walk_state(thread) to the error path to fix the problem.

Fixes: 0bac4295526c ("ACPICA: Dispatcher: Move stack traversal code to dispatcher")

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 drivers/acpi/acpica/dsmethod.c | 1 +
 1 file changed, 1 insertion(+)
  

On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> KASAN reports a use-after-free problem and causes kernel panic
> triggered by: modprobe acpiphp_ibm
>
> BUG: KASAN:
> use-after-free in acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
> Read of size 8 at addr ffff888002f843f0 by task modprobe/519
>
> CPU: 2 PID: 519 Comm: modprobe Not tainted 6.0.0+
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>     Call Trace:
>     <TASK>
>     acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
>     acpi_ds_method_error (drivers/acpi/acpica/dsmethod.c:232)
>     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>     ...
>     </TASK>
>
>     Allocated by task 519:
>     ...
>     __kasan_kmalloc (mm/kasan/common.c:526)
>     acpi_ds_create_walk_state (drivers/acpi/acpica/dswstate.c:519)
>     acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:498)
>     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>     ...
>
>     Freed by task 519:
>     ...
>     __kmem_cache_free+0xb6/0x3c0
>     acpi_ds_delete_walk_state (drivers/acpi/acpica/dswstate.c:722)
>     acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:586)
>     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>     ...
> ---[ end Kernel panic - not syncing: Fatal exception ]---
>
> In the error path in acpi_ps_parse_aml():
>
>     acpi_ds_call_control_method()
>         acpi_ds_create_walk_state()
>             acpi_ds_push_walk_state()
>             # thread->walk_state_list = walk_state
>
>         acpi_ds_init_aml_walk # *fail*
>         goto cleanup:
>         acpi_ds_delete_walk_state() # ACPI_FREE(walk_state)
>
>     acpi_ds_method_error()
>         acpi_ds_dump_method_stack()
>         # using freed thread->walk_state_list
>
> Briefly, the walk_state is pushed to thread, and freed without being poped.
> Then it is used in acpi_ds_dump_method_stack() and causes use-after-free.
>
> Add acpi_ds_pop_walk_state(thread) to the error path to fix the problem.
>
> Fixes: 0bac4295526c ("ACPICA: Dispatcher: Move stack traversal code to dispatcher")
>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>

This should be submitted to the upstream project on GitHub, but it
looks bad enough, so I'll take care of this.

Applied as 6.1-rc material, thanks!

> ---
>  drivers/acpi/acpica/dsmethod.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
> index ae2e768830bf..19da7fc73186 100644
> --- a/drivers/acpi/acpica/dsmethod.c
> +++ b/drivers/acpi/acpica/dsmethod.c
> @@ -581,6 +581,7 @@ acpi_ds_call_control_method(struct acpi_thread_state *thread,
>
>         acpi_ds_terminate_control_method(obj_desc, next_walk_state);
>         acpi_ds_delete_walk_state(next_walk_state);
> +       acpi_ds_pop_walk_state(thread);
>
>         return_ACPI_STATUS(status);
>  }
> --

Bob, this looks correct to me, but I may be missing something in which
case please let me know.
  

On Fri, Oct 28, 2022 at 5:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
> >
> > KASAN reports a use-after-free problem and causes kernel panic
> > triggered by: modprobe acpiphp_ibm
> >
> > BUG: KASAN:
> > use-after-free in acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
> > Read of size 8 at addr ffff888002f843f0 by task modprobe/519
> >
> > CPU: 2 PID: 519 Comm: modprobe Not tainted 6.0.0+
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> >     Call Trace:
> >     <TASK>
> >     acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
> >     acpi_ds_method_error (drivers/acpi/acpica/dsmethod.c:232)
> >     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >     ...
> >     </TASK>
> >
> >     Allocated by task 519:
> >     ...
> >     __kasan_kmalloc (mm/kasan/common.c:526)
> >     acpi_ds_create_walk_state (drivers/acpi/acpica/dswstate.c:519)
> >     acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:498)
> >     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >     ...
> >
> >     Freed by task 519:
> >     ...
> >     __kmem_cache_free+0xb6/0x3c0
> >     acpi_ds_delete_walk_state (drivers/acpi/acpica/dswstate.c:722)
> >     acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:586)
> >     acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >     ...
> > ---[ end Kernel panic - not syncing: Fatal exception ]---
> >
> > In the error path in acpi_ps_parse_aml():
> >
> >     acpi_ds_call_control_method()
> >         acpi_ds_create_walk_state()
> >             acpi_ds_push_walk_state()
> >             # thread->walk_state_list = walk_state
> >
> >         acpi_ds_init_aml_walk # *fail*
> >         goto cleanup:
> >         acpi_ds_delete_walk_state() # ACPI_FREE(walk_state)
> >
> >     acpi_ds_method_error()
> >         acpi_ds_dump_method_stack()
> >         # using freed thread->walk_state_list
> >
> > Briefly, the walk_state is pushed to thread, and freed without being poped.
> > Then it is used in acpi_ds_dump_method_stack() and causes use-after-free.
> >
> > Add acpi_ds_pop_walk_state(thread) to the error path to fix the problem.
> >
> > Fixes: 0bac4295526c ("ACPICA: Dispatcher: Move stack traversal code to dispatcher")
> >
> > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>
> This should be submitted to the upstream project on GitHub, but it
> looks bad enough, so I'll take care of this.
>
> Applied as 6.1-rc material, thanks!
>
> > ---
> >  drivers/acpi/acpica/dsmethod.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
> > index ae2e768830bf..19da7fc73186 100644
> > --- a/drivers/acpi/acpica/dsmethod.c
> > +++ b/drivers/acpi/acpica/dsmethod.c
> > @@ -581,6 +581,7 @@ acpi_ds_call_control_method(struct acpi_thread_state *thread,
> >
> >         acpi_ds_terminate_control_method(obj_desc, next_walk_state);
> >         acpi_ds_delete_walk_state(next_walk_state);
> > +       acpi_ds_pop_walk_state(thread);

On second thought, though, should it be popped before deleting?
Otherwise it looks like there will be still use-after-free, because
acpi_ds_pop_walk_state() accesses the walk_state at the top of the
queue.

Moreover, it is not correct to pop the walk state if next_walk_state
is NULL AFAICS.

I'm dropping this one.


> >
> >         return_ACPI_STATUS(status);
> >  }
> > --
>
> Bob, this looks correct to me, but I may be missing something in which
> case please let me know.
  

Hi,

On 2022/11/6 3:00, Rafael J. Wysocki wrote:
> On Fri, Oct 28, 2022 at 5:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>>> KASAN reports a use-after-free problem and causes kernel panic
>>> triggered by: modprobe acpiphp_ibm
>>>
>>> BUG: KASAN:
>>> use-after-free in acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
>>> Read of size 8 at addr ffff888002f843f0 by task modprobe/519
>>>
>>> CPU: 2 PID: 519 Comm: modprobe Not tainted 6.0.0+
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>>      Call Trace:
>>>      <TASK>
>>>      acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
>>>      acpi_ds_method_error (drivers/acpi/acpica/dsmethod.c:232)
>>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>>>      ...
>>>      </TASK>
>>>
>>>      Allocated by task 519:
>>>      ...
>>>      __kasan_kmalloc (mm/kasan/common.c:526)
>>>      acpi_ds_create_walk_state (drivers/acpi/acpica/dswstate.c:519)
>>>      acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:498)
>>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>>>      ...
>>>
>>>      Freed by task 519:
>>>      ...
>>>      __kmem_cache_free+0xb6/0x3c0
>>>      acpi_ds_delete_walk_state (drivers/acpi/acpica/dswstate.c:722)
>>>      acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:586)
>>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
>>>      ...
>>> ---[ end Kernel panic - not syncing: Fatal exception ]---
>>>
>>> In the error path in acpi_ps_parse_aml():
>>>
>>>      acpi_ds_call_control_method()
>>>          acpi_ds_create_walk_state()
>>>              acpi_ds_push_walk_state()
>>>              # thread->walk_state_list = walk_state
>>>
>>>          acpi_ds_init_aml_walk # *fail*
>>>          goto cleanup:
>>>          acpi_ds_delete_walk_state() # ACPI_FREE(walk_state)
>>>
>>>      acpi_ds_method_error()
>>>          acpi_ds_dump_method_stack()
>>>          # using freed thread->walk_state_list
>>>
>>> Briefly, the walk_state is pushed to thread, and freed without being poped.
>>> Then it is used in acpi_ds_dump_method_stack() and causes use-after-free.
>>>
>>> Add acpi_ds_pop_walk_state(thread) to the error path to fix the problem.
>>>
>>> Fixes: 0bac4295526c ("ACPICA: Dispatcher: Move stack traversal code to dispatcher")
>>>
>>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>> This should be submitted to the upstream project on GitHub, but it
>> looks bad enough, so I'll take care of this.
>>
>> Applied as 6.1-rc material, thanks!
>>
>>> ---
>>>   drivers/acpi/acpica/dsmethod.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
>>> index ae2e768830bf..19da7fc73186 100644
>>> --- a/drivers/acpi/acpica/dsmethod.c
>>> +++ b/drivers/acpi/acpica/dsmethod.c
>>> @@ -581,6 +581,7 @@ acpi_ds_call_control_method(struct acpi_thread_state *thread,
>>>
>>>          acpi_ds_terminate_control_method(obj_desc, next_walk_state);
>>>          acpi_ds_delete_walk_state(next_walk_state);
>>> +       acpi_ds_pop_walk_state(thread);
> On second thought, though, should it be popped before deleting?
> Otherwise it looks like there will be still use-after-free, because
> acpi_ds_pop_walk_state() accesses the walk_state at the top of the
> queue.

You are right it is wrong and sorry I didn't notice that.

I have reproduced same problem on current tree... Have no idea why I 
missed it before.


I noticed that this patch have been on next-tree so I submitted another 
one to fix it.

See "ACPICA: Fix pop_walk_state called after walk_state is deleted"


Thanks for your time!

Best,

Chen

> Moreover, it is not correct to pop the walk state if next_walk_state
> is NULL AFAICS.
>
> I'm dropping this one.
>
>
>>>          return_ACPI_STATUS(status);
>>>   }
>>> --
>> Bob, this looks correct to me, but I may be missing something in which
>> case please let me know.
  

On Mon, Nov 7, 2022 at 10:30 AM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> Hi,
>
> On 2022/11/6 3:00, Rafael J. Wysocki wrote:
> > On Fri, Oct 28, 2022 at 5:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
> >>> KASAN reports a use-after-free problem and causes kernel panic
> >>> triggered by: modprobe acpiphp_ibm
> >>>
> >>> BUG: KASAN:
> >>> use-after-free in acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
> >>> Read of size 8 at addr ffff888002f843f0 by task modprobe/519
> >>>
> >>> CPU: 2 PID: 519 Comm: modprobe Not tainted 6.0.0+
> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> >>>      Call Trace:
> >>>      <TASK>
> >>>      acpi_ds_dump_method_stack (drivers/acpi/acpica/dsdebug.c:145)
> >>>      acpi_ds_method_error (drivers/acpi/acpica/dsmethod.c:232)
> >>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >>>      ...
> >>>      </TASK>
> >>>
> >>>      Allocated by task 519:
> >>>      ...
> >>>      __kasan_kmalloc (mm/kasan/common.c:526)
> >>>      acpi_ds_create_walk_state (drivers/acpi/acpica/dswstate.c:519)
> >>>      acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:498)
> >>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >>>      ...
> >>>
> >>>      Freed by task 519:
> >>>      ...
> >>>      __kmem_cache_free+0xb6/0x3c0
> >>>      acpi_ds_delete_walk_state (drivers/acpi/acpica/dswstate.c:722)
> >>>      acpi_ds_call_control_method (drivers/acpi/acpica/dsmethod.c:586)
> >>>      acpi_ps_parse_aml (drivers/acpi/acpica/psparse.c:607)
> >>>      ...
> >>> ---[ end Kernel panic - not syncing: Fatal exception ]---
> >>>
> >>> In the error path in acpi_ps_parse_aml():
> >>>
> >>>      acpi_ds_call_control_method()
> >>>          acpi_ds_create_walk_state()
> >>>              acpi_ds_push_walk_state()
> >>>              # thread->walk_state_list = walk_state
> >>>
> >>>          acpi_ds_init_aml_walk # *fail*
> >>>          goto cleanup:
> >>>          acpi_ds_delete_walk_state() # ACPI_FREE(walk_state)
> >>>
> >>>      acpi_ds_method_error()
> >>>          acpi_ds_dump_method_stack()
> >>>          # using freed thread->walk_state_list
> >>>
> >>> Briefly, the walk_state is pushed to thread, and freed without being poped.
> >>> Then it is used in acpi_ds_dump_method_stack() and causes use-after-free.
> >>>
> >>> Add acpi_ds_pop_walk_state(thread) to the error path to fix the problem.
> >>>
> >>> Fixes: 0bac4295526c ("ACPICA: Dispatcher: Move stack traversal code to dispatcher")
> >>>
> >>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> >> This should be submitted to the upstream project on GitHub, but it
> >> looks bad enough, so I'll take care of this.
> >>
> >> Applied as 6.1-rc material, thanks!
> >>
> >>> ---
> >>>   drivers/acpi/acpica/dsmethod.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
> >>> index ae2e768830bf..19da7fc73186 100644
> >>> --- a/drivers/acpi/acpica/dsmethod.c
> >>> +++ b/drivers/acpi/acpica/dsmethod.c
> >>> @@ -581,6 +581,7 @@ acpi_ds_call_control_method(struct acpi_thread_state *thread,
> >>>
> >>>          acpi_ds_terminate_control_method(obj_desc, next_walk_state);
> >>>          acpi_ds_delete_walk_state(next_walk_state);
> >>> +       acpi_ds_pop_walk_state(thread);
> > On second thought, though, should it be popped before deleting?
> > Otherwise it looks like there will be still use-after-free, because
> > acpi_ds_pop_walk_state() accesses the walk_state at the top of the
> > queue.
>
> You are right it is wrong and sorry I didn't notice that.
>
> I have reproduced same problem on current tree... Have no idea why I
> missed it before.
>
>
> I noticed that this patch have been on next-tree so I submitted another
> one to fix it.
>
> See "ACPICA: Fix pop_walk_state called after walk_state is deleted"

At this point I have my own version of the fix, please see:

https://patchwork.kernel.org/project/linux-acpi/patch/2669303.mvXUDI8C0e@kreacher/