linux-kernel - Re: [PATCH] /dev/mem: Bail out upon SIGKILL when reading memory.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 23 Aug 2019 09:39:34 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        syzbot <syzbot+8ab2d0f39fb79fe6ca40@...kaller.appspotmail.com>
Subject: Re: [PATCH] /dev/mem: Bail out upon SIGKILL when reading memory.

On Fri, Aug 23, 2019 at 2:16 AM Ingo Molnar <mingo@...nel.org> wrote:
>
> > +      */
> > +     if (fatal_signal_pending(current)) {
> > +             if (!(error_code & X86_PF_USER))
> > +                     no_context(regs, error_code, address, 0, 0);
>
> Since the 'signal' parameter to no_context() is 0, will there be another
> signal generated? I don't think so - so the comment looks wrong to me,
> unless I'm missing something.

The old case only handled the kernel case - which never added a signal
at all, it just failed the access (and results in EFAULT or similar,
but nobody cares since the whole point is that the process is going to
be killed).

The *changed* case handles user space accesses too - by just returning
without any new signal being generated. The old case would fall
through to the "generate SIGSEGV", which seems pointless and wrong
(and also possibly generates misleading messages in the kernel logs).

> Other than that, what we are skipping here if a KILL signal is pending is
> the printout of oops information if the fault is a kernel one.
>
> Not sure that's a good idea in general: carefully timing a KILL signal
> would allow the 'stealth probing' of otherwise oops generating addresses?

That sounds really like a non-issue to me. Plus the old code ALREADY
did that exact thing. The only _new_ case is that it does is silently
for user page faults.

> I.e. I'm not sure this hunk is necessary or even a good idea.

As mentioned, the new code doesn't change the case you are talking about at all.

The new code only changes the case of this happening from user space,
when it doesn't generate a pointless signal and a pointless possible
show_signal_msg() garbage for a user mode access that was denied due
to the new

> > +     if (flags & FAULT_FLAG_KILLABLE) {
> > +             if (fatal_signal_pending(current))
> > +                     return VM_FAULT_SIGSEGV;
> > +     }

code in handle_mm_fault().

And you said that new code looks fine to you, but you didn't seem to
realize that it causes stupid incorrect kernel messages to be printed
("segfault at xyz" etc) that are completely wrong.

Because it's not a "real" SIGSEGV. It gets repressed by the fact that
there's a fatal signal pending.

Otherwise we'd have to add a whole new case of VM_FAULT_xyz..

             Linus