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

[Linus Torvalds]

On Fri, Aug 23, 2019 at 2:16 AM Ingo Molnar <mingo@xxxxxxxxxx> 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