Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211018090247.3779368-1-tzungbi@google.com> <CABXOdTfs4BPcx=A=Np--zVymJZ5g6S2P=Q5JE2T63zpJKUgJJA@mail.gmail.com>
In-Reply-To: <CABXOdTfs4BPcx=A=Np--zVymJZ5g6S2P=Q5JE2T63zpJKUgJJA@mail.gmail.com>
From:   Tzung-Bi Shih <tzungbi@google.com>
Date:   Tue, 19 Oct 2021 13:24:28 +0800
Message-ID: <CA+Px+wXrTjHVXYHENX0t2F0RHP-cLane9YLyMKRDNnkrki8BUw@mail.gmail.com>
Subject: Re: [RESEND PATCH] platform/chrome: cros_ec_debugfs: detach log
 reader wq from devm
To:     Guenter Roeck <groeck@google.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 18, 2021 at 4:59 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Mon, Oct 18, 2021 at 2:03 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > Debugfs console_log uses devm memory (see struct cros_ec_debugfs in
> > cros_ec_console_log fops).  However, lifecycles of device and debugfs
> > are independent.  An use-after-free issue is observed if userland
> > program operates the debugfs after the memory has been freed.
> >
>
> It would help to see the backtrace. Without it, it is difficult to
> determine where the UAF is observed. Also, most if not all of the
> touched functions access struct cros_ec_debugfs all over the place,
> not only for the wait queue, so I am not sure if moving the wait queue
> out of the data structure is the correct fix. It might instead be
> necessary to disconnect memory allocations from the ec device.

A trimmed backtrace is in the commit message, but the more verbose one:
[  426.174308] Call trace:
[  426.174314]  dump_backtrace+0x0/0x3ec
[  426.174318]  show_stack+0x20/0x2c
[  426.174324]  dump_stack+0x11c/0x1ac
[  426.174329]  print_address_description+0x7c/0x510
[  426.174333]  kasan_report+0x134/0x174
[  426.174337]  __asan_report_load4_noabort+0x44/0x50
[  426.174341]  do_raw_spin_lock+0x214/0x308
[  426.174345]  _raw_spin_lock_irqsave+0x68/0xf0
[  426.174350]  remove_wait_queue+0x3c/0x10c
[  426.174355]  ep_unregister_pollwait+0x120/0x170
[  426.174358]  ep_remove+0x60/0x2a0
[  426.174362]  do_epoll_ctl+0x590/0x7f4

I guess only the wait queue in the struct cros_ec_debugfs has
deep-coupled to console_log debugfs.  There are 2 more file operation
scenarios appended after the "--".

> > An userland program example in Python code:
> > ... import select
> > ... p = select.epoll()
> > ... f = open('/sys/kernel/debug/cros_scp/console_log')
> > ... p.register(f, select.POLLIN)
> > ... p.poll(1)
> > [(4, 1)]                    # 4=fd, 1=select.POLLIN
> >
> > [ shutdown cros_scp at the point ]
> >
> > ... p.poll(1)
> > [(4, 16)]                   # 4=fd, 16=select.POLLHUP
> > ... p.unregister(f)
> >
> > An use-after-free issue raises here.  It called epoll_ctl with
> > EPOLL_CTL_DEL which in turn to use the workqueue in the devm (i.e.
> > log_wq).
> >
> > The calling stack:
> > do_raw_spin_lock
> > _raw_spin_lock_irqsave
> > remove_wait_queue
> > ep_unregister_pollwait
> > ep_remove
> > do_epoll_ctl

Here is the trimmed backtrace.

[...]
> > ---
> > As for 2 other cases:
> >
> > Case 1. userland program opens the debugfs after the device has been removed
> >
> > ENOENT.  cros_ec_debugfs_remove() calls debugfs_remove_recursive().
> >
> > Case 2. userland program is reading when the device is removing
> >
> > If the userland program is waiting in cros_ec_console_log_read(), the device
> > removal will wait for it.
> >
> > See the calling stack for the case:
> > wait_for_completion
> > __debugfs_file_removed
> > remove_one
> > simple_recursive_removal
> > debugfs_remove
> > cros_ec_debugfs_remove
> > platform_drv_remove
> > device_release_driver_internal
> > device_release_driver
> > bus_remove_device
> > device_del
> > platform_device_del
> > platform_device_unregister

Here are the 2 other cases.