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]

Message-ID: <20190826111944.GA39308@gmail.com>
Date:   Mon, 26 Aug 2019 13:19:44 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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.


* Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp> wrote:

> On 2019/08/26 1:54, Linus Torvalds wrote:
> > On Sat, Aug 24, 2019 at 10:50 PM Tetsuo Handa
> > <penguin-kernel@...ove.sakura.ne.jp> wrote:
> >>
> >> @@ -142,7 +144,7 @@ static ssize_t read_mem(struct file *file, char __user *buf,
> >>                 sz = size_inside_page(p, count);
> >>                 cond_resched();
> >>                 err = -EINTR;
> >> -               if (fatal_signal_pending(current))
> >> +               if (signal_pending(current))
> >>                         goto failed;
> >>
> >>                 err = -EPERM;
> > 
> > So from a "likelihood of breaking" standpoint, I'd really like to make
> > sure that the "signal_pending()" checks come at the *end* of the loop.
> > 
> > That way, if somebody is doing a 4-byte read from MMIO, he'll never see -EINTR.
> > 
> > I'm specifically thinking of tools like user-space 'lspci' etc, which
> > I wouldn't be surprised could happen.
> > 
> > Also, just in case things break, I do agree with Ingo that this should
> > be split up into several patches.
> 
> Thinking from how read_mem() returns error code instead of returning bytes
> already processed, any sane users will not try to read so much memory (like 2GB).
> If userspace programs want to read so much memory, there must have been attempts
> to improve performance. I guess that userspace program somehow knows which region
> to read and tries to read only meaningful pages (which would not become hundreds MB).
> Thus, I don't think we want to make /dev/{mem,kmem} intrruptible. Just making killable
> in case insane userspace program (like fuzzer) tried to read/write so much memory
> will be sufficient...

Basically making IO primitives interruptible is the norm and it's a 
quality of implementation issue: it's only a historic accident that 
/dev/mem read()s aren't.

So let's try and make it interruptible as the #3 patch I sent did - of 
course if anything breaks we'll have to undo it. But if we can get away 
with then by all means let's do so - even shorter reads can generate 
nasty long processing latencies.

Ok?

Thanks,

     Ingo