On Thu, Dec 1, 2016 at 6:13 PM, Josh Poimboeuf <jpoim...@redhat.com> wrote: > On Thu, Dec 01, 2016 at 05:51:52PM +0100, Dmitry Vyukov wrote: >> On Thu, Dec 1, 2016 at 5:45 PM, Josh Poimboeuf <jpoim...@redhat.com> wrote: >> > On Thu, Dec 01, 2016 at 08:58:21AM -0600, Josh Poimboeuf wrote: >> >> On Thu, Dec 01, 2016 at 12:05:34PM +0300, Andrey Ryabinin wrote: >> >> > >> >> > >> >> > On 12/01/2016 02:10 AM, Josh Poimboeuf wrote: >> >> > > Resuming from a suspend operation is showing a KASAN false positive >> >> > > warning: >> >> > > >> >> > >> >> > > KASAN instrumentation poisons the stack when entering a function and >> >> > > unpoisons it when exiting the function. However, in the suspend path, >> >> > > some functions never return, so their stack never gets unpoisoned, >> >> > > resulting in stale KASAN shadow data which can cause false positive >> >> > > warnings like the one above. >> >> > > >> >> > > Reported-by: Scott Bauer <scott.ba...@intel.com> >> >> > > Tested-by: Scott Bauer <scott.ba...@intel.com> >> >> > > Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com> >> >> > > --- >> >> > > arch/x86/kernel/acpi/sleep.c | 3 +++ >> >> > > include/linux/kasan.h | 7 +++++++ >> >> > > 2 files changed, 10 insertions(+) >> >> > > >> >> > > diff --git a/arch/x86/kernel/acpi/sleep.c >> >> > > b/arch/x86/kernel/acpi/sleep.c >> >> > > index 4858733..62bd046 100644 >> >> > > --- a/arch/x86/kernel/acpi/sleep.c >> >> > > +++ b/arch/x86/kernel/acpi/sleep.c >> >> > > @@ -115,6 +115,9 @@ int x86_acpi_suspend_lowlevel(void) >> >> > > pause_graph_tracing(); >> >> > > do_suspend_lowlevel(); >> >> > > unpause_graph_tracing(); >> >> > > + >> >> > > + kasan_unpoison_stack_below_sp(); >> >> > > + >> >> > >> >> > I think this might be too late. We may hit stale poison in the first C >> >> > function called >> >> > after resume (restore_processor_state()). Thus the shadow must be >> >> > unpoisoned prior such call, >> >> > i.e. somewhere in do_suspend_lowlevel() after .Lresume_point. >> >> >> >> Yeah, I think you're right. Will spin a v2. >> > >> > So I tried calling kasan_unpoison_task_stack_below() from >> > do_suspend_lowlevel(), but it hung on the resume. Presumably because >> > restore_processor_state() does some important setup which would be >> > needed before calling into kasan_unpoison_task_stack_below(). For >> > example, setting up the gs register. So it's a bit of a catch-22. >> > >> > It could probably be fixed properly by rewriting do_suspend_lowlevel() >> > to call restore_processor_state() with the temporary stack before >> > switching to the original stack and doing the unpoison. >> > >> > (And there are some other issues with do_suspend_lowlevel() and I'd love >> > to try taking a scalpel to it. But I have too many knives in the air >> > already to want to try to attempt that right now...) >> > >> > Unless somebody else wants to take a stab at it, my original patch is >> > probably good enough for now, since restore_processor_state() doesn't >> > seem to be triggering any KASAN warnings. >> >> restore_processor_state/__restore_processor_state does not seem to >> have any local variables, so KASAN does not do any stack checks there. > > Actually, looking at the object code, it uses a lot of stack space and > has several calls to __asan_report_load*() functions. Probably due to > inlining of other functions which have stack variables.
That can be loads of heap variables (or other non-stack data). KASAN will emit these checks for lots of loads, but they don't necessary go to stack. >> We could disable KASAN instrumentation of the file, or of particular >> functions. > > I don't think that would be sufficient unless it were disabled for > __restore_processor_state() and all the functions it calls (and the > functions they call, etc), which wouldn't necessarily be > straightforward. > >> Or we could call kasan_unpoison_shadow() on the stack range >> before switching to it. > > I tried that already, but it hung because restore_processor_state() > hadn't been called yet (the catch-22 I mentioned aboved). Ah, I see, we just can't execute normal C code at that point...