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

[Ingo Molnar <mingo@kernel.org>]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Aug 20, 2019 at 3:07 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >
> > syzbot found that a thread can stall for minutes inside read_mem()
> > after that thread was killed by SIGKILL [1]. Reading 2GB at one read()
> > is legal, but delaying termination of killed thread for minutes is bad.
> 
> Side note: we might even just allow regular signals to interrupt
> /dev/mem reads. We already do that for /dev/zero, and the risk of
> breaking something is likely fairly low since nothing should use that
> thing anyway.
> 
> Also, if it takes minutes to delay killing things, that implies that
> we're probably still faulting in pages for the read_mem(). Which
> points to another possible thing we could do in general: just don't
> bother to handle page faults when a fatal signal is pending.
> 
> That situation might happen for other random cases too, and is not
> limited to /dev/mem. So maybe it's worth trying? Does that essentially
> fix the /dev/mem read case too in practice?
> 
> COMPLETELY untested patch attached, it may or may not make a
> difference (and it may or may not work at all ;)
> 
>                 Linus

>  arch/x86/mm/fault.c | 15 ++++++++++++---
>  mm/memory.c         |  5 +++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 9ceacd1156db..d6c029a6cb90 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1033,8 +1033,15 @@ static noinline void
>  mm_fault_error(struct pt_regs *regs, unsigned long error_code,
>  	       unsigned long address, vm_fault_t fault)
>  {
> -	if (fatal_signal_pending(current) && !(error_code & X86_PF_USER)) {
> -		no_context(regs, error_code, address, 0, 0);
> +	/*
> +	 * If we already have a fatal signal, don't bother adding
> +	 * a new one. If it's a kernel access, just make it fail,
> +	 * and if it's a user access just return to let the process
> +	 * die.
> +	 */
> +	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.

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? 
Arguably it would only be a marginally useful facility to any attacker, 
but it doesn't feel right to me to skip the printing of bad faults if 
they occur.

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



>  		return;
>  	}
>  
> @@ -1389,7 +1396,8 @@ void do_user_addr_fault(struct pt_regs *regs,
>  			return;
>  		}
>  retry:
> -		down_read(&mm->mmap_sem);
> +		if (down_read_killable(&mm->mmap_sem))
> +			goto fatal_signal;
>  	} else {
>  		/*
>  		 * The above down_read_trylock() might have succeeded in
> @@ -1455,6 +1463,7 @@ void do_user_addr_fault(struct pt_regs *regs,
>  				goto retry;
>  		}
>  
> +fatal_signal:
>  		/* User mode? Just return to handle the fatal exception */
>  		if (flags & FAULT_FLAG_USER)
>  			return;
> diff --git a/mm/memory.c b/mm/memory.c
> index e2bb51b6242e..7ad62f96b08e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3988,6 +3988,11 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>  					    flags & FAULT_FLAG_REMOTE))
>  		return VM_FAULT_SIGSEGV;
>  
> +	if (flags & FAULT_FLAG_KILLABLE) {
> +		if (fatal_signal_pending(current))
> +			return VM_FAULT_SIGSEGV;
> +	}
> +
>  	/*
>  	 * Enable the memcg OOM handling for faults triggered in user
>  	 * space.  Kernel faults are handled more gracefully.

This part looks good to me, assuming someone tests it and it solves the 
bug :-)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo