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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Walter Wu <walter-zh.wu@mediatek.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        John Stultz <john.stultz@linaro.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Marco Elver <elver@google.com>,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Konovalov <andreyknvl@google.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        wsd_upstream <wsd_upstream@mediatek.com>,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v4 1/6] timer: kasan: record timer stack
In-Reply-To: <1601140312.15228.12.camel@mtksdccf07>
Date:   Wed, 30 Sep 2020 09:18:40 +0200
Message-ID: <87pn63ivfz.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Walter,

On Sun, Sep 27 2020 at 01:11, Walter Wu wrote:
> First, I think the commit log =E2=80=9CBecause if the UAF root cause is i=
n timer
> init =E2=80=A6=E2=80=9D needs to be removed, this patch hopes to help pro=
grammer gets
> timer callback is where is registered. It is useful only if free stack
> is called from timer callback, because programmer can see why & where
> register this function.
>
> Second, see [1], it should satisfies first point. The free stack is from
> timer callback, if we know where register this function, then it should
> be useful to solve UAF.

No. It's completely useless.

The problem has absolutely nothing to do with the timer callback and the
timer_init() invocation which set the timer's callback to 'dummy_timer'.

The timer callback happens to free the object, but the worker thread has
still a reference of some sort.

So the problem is either missing refcounting which allows the timer
callback to free the object or some missing serialization.

Knowing the place which initialized the timer is absolutely not helping
to figure out what's missing here.

Aside of that it's trivial enough to do:

  git grep dummy_timer drivers/usb/gadget/udc/dummy_hcd.c

if you really want to know what initialized it:

 dummy_timer+0x1258/0x32ae drivers/usb/gadget/udc/dummy_hcd.c:1966
 call_timer_fn+0x195/0x6f0 kernel/time/timer.c:1404
 expire_timers kernel/time/timer.c:1449 [inline]

That said, I'm all for adding useful information to KASAN or whatever
reports, but I'm not agreeing with the approach of 'Let's sprinkle
kasan_foo() all over tha place and claim it is useful to decode an UAF'.
Adding irrelevant information to a report is actually counter productive
because it makes people look at the wrong place.

Again: Provide an analysis of such a dump where the timer_init()
function is a key element of solving the problem.

Thanks,

        tglx