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>
 <CA+Px+wXrTjHVXYHENX0t2F0RHP-cLane9YLyMKRDNnkrki8BUw@mail.gmail.com>
In-Reply-To: <CA+Px+wXrTjHVXYHENX0t2F0RHP-cLane9YLyMKRDNnkrki8BUw@mail.gmail.com>
From:   Tzung-Bi Shih <tzungbi@google.com>
Date:   Mon, 25 Oct 2021 17:30:23 +0800
Message-ID: <CA+Px+wWjcySEn5i3=GMGtZ4+hT5Z9L+QLLyxbv1WB7Pi1nPu9A@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

Hi Guenter,

On Tue, Oct 19, 2021 at 1:24 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> 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 "--".

Do you think the backtrace is sufficient to determine the UAF happens
on the wait queue?

How about we keep the fix as is since we have a constantly reproducing
step for the UAF.  And look forward to the approach "disconnect memory
allocations from the ec device" if we could discover more UAFs?