Re: [PATCH 4.13 2/2] waitid(): Add missing access_ok() checks

[David Daney]

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@xxxxxxxxxxxx>
>
> commit 96ca579a1ecc943b75beba58bebb0356f6cc4b51 upstream.
>
> Adds missing access_ok() checks.
>
> CVE-2017-5123
>
> Reported-by: Chris Salls <chrissalls5@xxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Acked-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Fixes: 4c48abe91be0 ("waitid(): switch copyout of siginfo to unsafe_put_user()")
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> ---
>   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