linux-kernel - Re: [PATCH 4.13 2/2] waitid(): Add missing access

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] [day] [month] [year] [list]

Message-ID: <8f8d5da2-3e37-12de-f9d8-9fd56c4bb98a@caviumnetworks.com>
Date:   Fri, 13 Oct 2017 16:55:12 -0700
From:   David Daney <ddaney@...iumnetworks.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>
Cc:     stable@...r.kernel.org, Chris Salls <chrissalls5@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 4.13 2/2] waitid(): Add missing access_ok() checks

On 10/12/2017 02:26 PM, Greg Kroah-Hartman wrote:
> 4.13-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Kees Cook <keescook@...omium.org>
> 
> commit 96ca579a1ecc943b75beba58bebb0356f6cc4b51 upstream.
> 
> Adds missing access_ok() checks.
> 
> CVE-2017-5123
> 
> Reported-by: Chris Salls <chrissalls5@...il.com>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Acked-by: Al Viro <viro@...iv.linux.org.uk>
> Fixes: 4c48abe91be0 ("waitid(): switch copyout of siginfo to unsafe_put_user()")
> Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> 
> ---
>   kernel/exit.c |    6 ++++++
>   1 file changed, 6 insertions(+)
> 
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1611,6 +1611,9 @@ SYSCALL_DEFINE5(waitid, int, which, pid_
>   	if (!infop)
>   		return err;
>   
> +	if (!access_ok(VERIFY_WRITE, infop, sizeof(*infop)))
> +		goto Efault;

Not to be a pedant, but...

In the case that access_ok() fails, we invoke user_access_end() at the 
goto target without first invoking user_access_begin().  On x86 this 
imbalance is probably not a problem.

For other architectures that may want to implement 
user_access_{begin,end}() in the future, I think we should either 
specify that unbalanced calls to these two functions are expected and 
must work, or balance them here and specify that they must be balanced.


Thanks,
David Daney