Re: [PATCH v4 1/6] timer: kasan: record timer stack

From: Walter Wu <walter-zh.wu@mediatek.com>
To: Thomas Gleixner <tglx@linutronix.de>
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
Date: Fri, 25 Sep 2020 17:15:46 +0800	[thread overview]
Message-ID: <1601025346.2255.2.camel@mtksdccf07> (raw)
In-Reply-To: <87lfgyutf8.fsf@nanos.tec.linutronix.de>

On Fri, 2020-09-25 at 10:55 +0200, Thomas Gleixner wrote:
> Walter,
> 
> On Fri, Sep 25 2020 at 15:18, Walter Wu wrote:
> > On Thu, 2020-09-24 at 23:41 +0200, Thomas Gleixner wrote:
> >> > For timers it has turned out to be useful to record the stack trace
> >> > of the timer init call.
> >> 
> >> In which way? And what kind of bug does it catch which cannot be catched
> >> by existing debug mechanisms already?
> >> 
> > We only provide another debug mechanisms to debug use-after-free or
> > double-free, it can be displayed together in KASAN report and have a
> > chance to debug, and it doesn't need to enable existing debug mechanisms
> > at the same time. then it has a chance to resolve issue.
> 
> Again. KASAN can only cover UAF, but there are a dozen other ways to
> wreck the system with wrong usage of timers which can't be caught by
> KASAN.
> 
> >> > Because if the UAF root cause is in timer init, then user can see
> >> > KASAN report to get where it is registered and find out the root
> >> > cause.
> >> 
> >> What? If the UAF root cause is in timer init, then registering it after
> >> using it in that very same function is pretty pointless.
> >> 
> > See [1], the call stack shows UAF happen at dummy_timer(), it is the
> > callback function and set by timer_setup(), if KASAN report shows the
> > timer call stack, it should be useful for programmer.
> 
> The report you linked to has absolutely nothing to do with a timer
> related UAF. The timer callback calls kfree_skb() on something which is
> already freed. So the root cause of this is NOT in timer init as you
> claimed above. The timer callback is just exposing a problem in the URB
> management of this driver. IOW the recording of the timer init stack is
> completely useless for decoding this problem.
> 
> >> There is a lot of handwaving how useful this is, but TBH I don't see the
> >> value at all.
> >> 
> >> DEBUG_OBJECTS_TIMERS does a lot more than crashing on UAF. If KASAN
> >> provides additional value over DEBUG_OBJECTS_TIMERS then spell it out,
> >> but just saying that you don't need to enable DEBUG_OBJECTS_TIMERS is
> >> not making an argument for that change.
> >> 
> > We don't want to replace DEBUG_OBJECTS_TIMERS with this patches, only
> > hope to use low overhead(compare with DEBUG_OBJECTS_TIMERS) to debug
> 
> KASAN has lower overhead than DEBUG_OBJECTS_TIMERS? Maybe in a different
> universe.
> 

I mean KASAN + our patch vs KASAN + DEBUG_OBJECTS_TIMERS. The front one
have the information to the original caller and help to debug. It is
smaller overhead than the one behind.

> That said, I'm not opposed to the change per se, but without a sensible
> justification this is just pointless.
> 
> Sprinkling kasan_foo() all over the place and claiming it's useful
> without a valid example does not provide any value.
> 
> Quite the contrary it gives the completely wrong sense what KASAN can do
> and what not.
> 

I agree your saying, so that I need to find out a use case to explain to
you.

Thanks

Walter

> Thanks,
> 
>         tglx
> 