[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-api
Subject:    Re: [PATCH 3/6] exec: Check for a pending fatal signal instead of core_state
From:       Kees Cook <keescook () chromium ! org>
Date:       2021-09-24 15:38:41
Message-ID: 202109240834.8D3A18AC32 () keescook
[Download RAW message or body]

On Thu, Sep 23, 2021 at 07:10:43PM -0500, Eric W. Biederman wrote:
> 
> Prevent exec continuing when a fatal signal is pending by replacing
> mmap_read_lock with mmap_read_lock_killable.  This is always the right
> thing to do as userspace will never observe an exec complete when
> there is a fatal signal pending.
> 
> With that change it becomes unnecessary to explicitly test for a core
> dump in progress.  In coredump_wait zap_threads arranges under
> mmap_write_lock for all tasks that use a mm to also have SIGKILL
> pending, which means mmap_read_lock_killable will always return -EINTR
> when old_mm->core_state is present.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/exec.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index a098c133d8d7..b6079f1a098e 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -987,16 +987,14 @@ static int exec_mmap(struct mm_struct *mm)
>  
>  	if (old_mm) {
>  		/*
> -		 * Make sure that if there is a core dump in progress
> -		 * for the old mm, we get out and die instead of going
> -		 * through with the exec.  We must hold mmap_lock around
> -		 * checking core_state and changing tsk->mm.
> +		 * If there is a pending fatal signal perhaps a signal
> +		 * whose default action is to create a coredump get
> +		 * out and die instead of going through with the exec.
>  		 */
> -		mmap_read_lock(old_mm);
> -		if (unlikely(old_mm->core_state)) {
> -			mmap_read_unlock(old_mm);
> +		ret = mmap_read_lock_killable(old_mm);

This appears to ultimately call into rwsem_down_read_slowpath(), which
checks signal_pending_state() (and returns the EINTR from before),
so this looks right to me.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> +		if (ret) {
>  			up_write(&tsk->signal->exec_update_lock);
> -			return -EINTR;
> +			return ret;
>  		}
>  	}
>  
> -- 
> 2.20.1
> 

-- 
Kees Cook
[prev in list] [next in list] [prev in thread] [next in thread]