Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Benjamin LaHaise]

On Wed, Dec 13, 2017 at 06:11:12AM -0800, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
> > Below information is reported by a lower kernel version, and I saw the
> > problem still exist in current version.
> 
> I think you're right, but what an awful interface we have here!
> The user must not only fetch it, they must validate it separately?
> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
> Why not this:

That looks far better!

Acked-by: Benjamin LaHaise <bcrl@xxxxxxxxx>

		-ben

> diff --git a/fs/aio.c b/fs/aio.c
> index 4adbdcbe753a..fdd16cf897c8 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1788,8 +1788,9 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
>  	struct timespec64	ts;
>  
>  	if (timeout) {
> -		if (unlikely(get_timespec64(&ts, timeout)))
> -			return -EFAULT;
> +		int error = get_valid_timespec64(&ts, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
> @@ -1805,9 +1806,9 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
>  	struct timespec64 t;
>  
>  	if (timeout) {
> -		if (compat_get_timespec64(&t, timeout))
> -			return -EFAULT;
> -
> +		int error = compat_get_valid_timespec64(&t, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 0fc36406f32c..578fc0f208d9 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -172,6 +172,26 @@ extern int get_compat_itimerspec64(struct itimerspec64 *its,
>  extern int put_compat_itimerspec64(const struct itimerspec64 *its,
>  			struct compat_itimerspec __user *uits);
>  
> +static inline __must_check
> +int compat_get_valid_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check
> +int compat_get_strict_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  struct compat_iovec {
>  	compat_uptr_t	iov_base;
>  	compat_size_t	iov_len;
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4b62a2c0a661..506d87483d04 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -18,6 +18,26 @@ int get_itimerspec64(struct itimerspec64 *it,
>  int put_itimerspec64(const struct itimerspec64 *it,
>  			struct itimerspec __user *uit);
>  
> +static inline __must_check int get_valid_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check int get_strict_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  extern time64_t mktime64(const unsigned int year, const unsigned int mon,
>  			const unsigned int day, const unsigned int hour,
>  			const unsigned int min, const unsigned int sec);
> 

-- 
"Thought is the essence of where you are now."