linux-kernel - Re: [PATCH] ACPICA: Fix use-after-free in acpi_ps_parse

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 7 Nov 2022 18:51:08 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Chen Zhongjin <chenzhongjin@...wei.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>, robert.moore@...el.com,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        rafael.j.wysocki@...el.com, lenb@...nel.org, lv.zheng@...el.com
Subject: Re: [PATCH] ACPICA: Fix use-after-free in acpi_ps_parse_aml()

On Mon, Nov 7, 2022 at 10:30 AM Chen Zhongjin <chenzhongjin@...wei.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@...nel.org> wrote:
> >> On Wed, Oct 19, 2022 at 9:38 AM Chen Zhongjin <chenzhongjin@...wei.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@...wei.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/