Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221019073443.248215-1-chenzhongjin@huawei.com>
 <CAJZ5v0hV2AFEgiuxxbDFUWLa0ZthSz3a=-9U4pjXm-GmmSgexw@mail.gmail.com>
 <CAJZ5v0jPCGoss6X5bmv9Nw9ZxrDxirEEMh6UKSgOoArs2d9ffA@mail.gmail.com> <c01b8c85-59d0-20a8-5e72-4e628a84bf05@huawei.com>
In-Reply-To: <c01b8c85-59d0-20a8-5e72-4e628a84bf05@huawei.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 7 Nov 2022 18:51:08 +0100
Message-ID: <CAJZ5v0ixwximCOMPwqMhkBKuTd2yNtbQsphKzpk2jmRZfrj9wA@mail.gmail.com>
Subject: Re: [PATCH] ACPICA: Fix use-after-free in acpi_ps_parse_aml()
To:     Chen Zhongjin <chenzhongjin@huawei.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>, robert.moore@intel.com,
        linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        rafael.j.wysocki@intel.com, lenb@kernel.org, lv.zheng@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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/