Re: [PATCH] ACPICA: Fix use-after-free in acpi_ps_parse_aml()

[Chen Zhongjin]

Hi,

On 2022/11/6 3:00, Rafael J. Wysocki wrote:
> On Fri, Oct 28, 2022 at 5:46 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@xxxxxxxxxx> 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@xxxxxxxxxx>
> > 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.