Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] aio: make sure the input "timeout" value is valid
@ 2017-12-13 13:42 Zhen Lei
  2017-12-13 14:11 ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Zhen Lei @ 2017-12-13 13:42 UTC (permalink / raw)
  To: Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio, linux-kernel
  Cc: Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang, Zhen Lei

Below information is reported by a lower kernel version, and I saw the
problem still exist in current version.

UBSAN: Undefined behaviour in include/linux/ktime.h:55:34
signed integer overflow:
-4971973988617027584 * 1000000000 cannot be represented in type 'long int'
......
[<ffff80000072ca28>] timespec_to_ktime include/linux/ktime.h:55 [inline]
[<ffff80000072ca28>] read_events+0x4c8/0x5d0 fs/aio.c:1269
[<ffff8000007305bc>] SYSC_io_getevents fs/aio.c:1733 [inline]
[<ffff8000007305bc>] SyS_io_getevents+0xd4/0x218 fs/aio.c:1722

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 fs/aio.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index a062d75..19f7661 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1858,6 +1858,9 @@ static long do_io_getevents(aio_context_t ctx_id,
 	if (timeout) {
 		if (unlikely(get_timespec64(&ts, timeout)))
 			return -EFAULT;
+
+		if (!timespec64_valid(&ts))
+			return -EINVAL;
 	}

 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
@@ -1876,6 +1879,8 @@ static long do_io_getevents(aio_context_t ctx_id,
 		if (compat_get_timespec64(&t, timeout))
 			return -EFAULT;

+		if (!timespec64_valid(&t))
+			return -EINVAL;
 	}

 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
--
1.8.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-13 13:42 [PATCH 1/1] aio: make sure the input "timeout" value is valid Zhen Lei
@ 2017-12-13 14:11 ` Matthew Wilcox
  2017-12-13 15:58   ` Benjamin LaHaise
  2017-12-13 16:27   ` Jeff Moyer
  0 siblings, 2 replies; 11+ messages in thread
From: Matthew Wilcox @ 2017-12-13 14:11 UTC (permalink / raw)
  To: Zhen Lei
  Cc: Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang,
	Deepa Dinamani

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:

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);

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-13 14:11 ` Matthew Wilcox
@ 2017-12-13 15:58   ` Benjamin LaHaise
  2017-12-13 16:27   ` Jeff Moyer
  1 sibling, 0 replies; 11+ messages in thread
From: Benjamin LaHaise @ 2017-12-13 15:58 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Zhen Lei, Alexander Viro, linux-fsdevel, linux-aio, linux-kernel,
	Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang, Deepa Dinamani

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@kvack.org>

		-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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-13 14:11 ` Matthew Wilcox
  2017-12-13 15:58   ` Benjamin LaHaise
@ 2017-12-13 16:27   ` Jeff Moyer
  2017-12-13 19:31     ` Matthew Wilcox
  1 sibling, 1 reply; 11+ messages in thread
From: Jeff Moyer @ 2017-12-13 16:27 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Zhen Lei, Alexander Viro, Benjamin LaHaise, linux-fsdevel,
	linux-aio, linux-kernel, Tianhong Ding, Hanjun Guo, Libin,
	Kefeng Wang, Deepa Dinamani

Matthew Wilcox <willy@infradead.org> writes:

> 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:

Why not go a step further and have get_timespec64 check for validity?
I wonder what caller doesn't want that to happen...

-Jeff

>
> 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);
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to majordomo@kvack.org.  For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-13 16:27   ` Jeff Moyer
@ 2017-12-13 19:31     ` Matthew Wilcox
  2017-12-14  3:18       ` Leizhen (ThunderTown)
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2017-12-13 19:31 UTC (permalink / raw)
  To: Jeff Moyer
  Cc: Zhen Lei, Alexander Viro, Benjamin LaHaise, linux-fsdevel,
	linux-aio, linux-kernel, Tianhong Ding, Hanjun Guo, Libin,
	Kefeng Wang, Deepa Dinamani

On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > 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:
> 
> Why not go a step further and have get_timespec64 check for validity?
> I wonder what caller doesn't want that to happen...

There are some which don't today.  I'm hoping Deepa takes this and goes
off and fixes them all up.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-13 19:31     ` Matthew Wilcox
@ 2017-12-14  3:18       ` Leizhen (ThunderTown)
  2018-01-02 14:51         ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Leizhen (ThunderTown) @ 2017-12-14  3:18 UTC (permalink / raw)
  To: Matthew Wilcox, Jeff Moyer
  Cc: Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang,
	Deepa Dinamani



On 2017/12/14 3:31, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>> Matthew Wilcox <willy@infradead.org> writes:
>>
>>> 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:
>>
>> Why not go a step further and have get_timespec64 check for validity?
>> I wonder what caller doesn't want that to happen...
I tried this before. But I found some places call get_timespec64 in the following function.
If we do the check in get_timespec64, the check will be duplicated.

For example:
static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
....
	if (get_timespec64(&ts, tsp))
		return -EFAULT;

	to = &end_time;
	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))

int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
{
	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};

	if (!timespec64_valid(&ts))
		return -EINVAL;

> 
> There are some which don't today.  I'm hoping Deepa takes this and goes
> off and fixes them all up.
As my search results, just the case I mentioned above, which may cause duplicate check.
So if we don't care the slightly performance drop, maybe we should do timespec64_valid
check in get_timespec64. I can try this in v2. Otherwise, use your method.

> 
> .
> 

-- 
Thanks!
BestRegards

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2017-12-14  3:18       ` Leizhen (ThunderTown)
@ 2018-01-02 14:51         ` Matthew Wilcox
  2018-01-12 19:49           ` Jeff Moyer
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2018-01-02 14:51 UTC (permalink / raw)
  To: Leizhen (ThunderTown)
  Cc: Jeff Moyer, Alexander Viro, Benjamin LaHaise, linux-fsdevel,
	linux-aio, linux-kernel, Tianhong Ding, Hanjun Guo, Libin,
	Kefeng Wang, Deepa Dinamani

On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
> On 2017/12/14 3:31, Matthew Wilcox wrote:
> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
> >> Matthew Wilcox <willy@infradead.org> writes:
> >>
> >>> 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:
> >>
> >> Why not go a step further and have get_timespec64 check for validity?
> >> I wonder what caller doesn't want that to happen...
> I tried this before. But I found some places call get_timespec64 in the following function.
> If we do the check in get_timespec64, the check will be duplicated.
> 
> For example:
> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
> ....
> 	if (get_timespec64(&ts, tsp))
> 		return -EFAULT;
> 
> 	to = &end_time;
> 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
> 
> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
> {
> 	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
> 
> 	if (!timespec64_valid(&ts))
> 		return -EINVAL;

The check is only two comparisons!  Why do we have an interface that can
cause bugs for the sake of saving *two comparisons*?!  Can we talk about
the cost of a cache miss versus the cost of executing these comparisons?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2018-01-02 14:51         ` Matthew Wilcox
@ 2018-01-12 19:49           ` Jeff Moyer
  2018-03-26 20:01             ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff Moyer @ 2018-01-12 19:49 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Leizhen (ThunderTown),
	Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang,
	Deepa Dinamani

Matthew Wilcox <willy@infradead.org> writes:

> On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
>> On 2017/12/14 3:31, Matthew Wilcox wrote:
>> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>> >> Matthew Wilcox <willy@infradead.org> writes:
>> >>
>> >>> 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:
>> >>
>> >> Why not go a step further and have get_timespec64 check for validity?
>> >> I wonder what caller doesn't want that to happen...
>> I tried this before. But I found some places call get_timespec64 in the following function.
>> If we do the check in get_timespec64, the check will be duplicated.
>> 
>> For example:
>> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
>> ....
>> 	if (get_timespec64(&ts, tsp))
>> 		return -EFAULT;
>> 
>> 	to = &end_time;
>> 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
>> 
>> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
>> {
>> 	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
>> 
>> 	if (!timespec64_valid(&ts))
>> 		return -EINVAL;
>
> The check is only two comparisons!  Why do we have an interface that can
> cause bugs for the sake of saving *two comparisons*?!  Can we talk about
> the cost of a cache miss versus the cost of executing these comparisons?

Any update on this?  Willy, I'd be okay with your get_valid_timespec64
patch if you wanted to formally submit that.

-Jeff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2018-01-12 19:49           ` Jeff Moyer
@ 2018-03-26 20:01             ` Arnd Bergmann
  2018-03-26 21:55               ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-03-26 20:01 UTC (permalink / raw)
  To: Jeff Moyer
  Cc: Matthew Wilcox, Leizhen (ThunderTown),
	Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang,
	Deepa Dinamani

On Fri, Jan 12, 2018 at 8:49 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Matthew Wilcox <willy@infradead.org> writes:
>
>> On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
>>> On 2017/12/14 3:31, Matthew Wilcox wrote:
>>> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>>> >> Matthew Wilcox <willy@infradead.org> writes:
>>> >>
>>> >>> 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:
>>> >>
>>> >> Why not go a step further and have get_timespec64 check for validity?
>>> >> I wonder what caller doesn't want that to happen...
>>> I tried this before. But I found some places call get_timespec64 in the following function.
>>> If we do the check in get_timespec64, the check will be duplicated.
>>>
>>> For example:
>>> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
>>> ....
>>>      if (get_timespec64(&ts, tsp))
>>>              return -EFAULT;
>>>
>>>      to = &end_time;
>>>      if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
>>>
>>> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
>>> {
>>>      struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
>>>
>>>      if (!timespec64_valid(&ts))
>>>              return -EINVAL;
>>
>> The check is only two comparisons!  Why do we have an interface that can
>> cause bugs for the sake of saving *two comparisons*?!  Can we talk about
>> the cost of a cache miss versus the cost of executing these comparisons?
>
> Any update on this?  Willy, I'd be okay with your get_valid_timespec64
> patch if you wanted to formally submit that.

I had suggested a more complete helper function at some point,
to take care of all combinations of checking/non-checking, 32/64
bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
of nanoseconds before comparing against 1 billion, but Deepa
thought that was overkill, so I didn't continue that.

For all I can tell, the get_timespec64() helper should almost always
include the check, the one exception I know is utimensat() and related
functions that may encode the special UTIME_NOW and UTIME_OMIT
constants in the nanoseconds.

        Arnd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2018-03-26 20:01             ` Arnd Bergmann
@ 2018-03-26 21:55               ` Matthew Wilcox
  2018-03-27  4:43                 ` Deepa Dinamani
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2018-03-26 21:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jeff Moyer, Leizhen (ThunderTown),
	Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang,
	Deepa Dinamani

On Mon, Mar 26, 2018 at 10:01:30PM +0200, Arnd Bergmann wrote:
> I had suggested a more complete helper function at some point,
> to take care of all combinations of checking/non-checking, 32/64
> bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
> of nanoseconds before comparing against 1 billion, but Deepa
> thought that was overkill, so I didn't continue that.

Yeah, that sounds like a nightmare to use ;-)

> For all I can tell, the get_timespec64() helper should almost always
> include the check, the one exception I know is utimensat() and related
> functions that may encode the special UTIME_NOW and UTIME_OMIT
> constants in the nanoseconds.

So do you endorse the get_valid_timespec64() patch I posted up-thread?
We can't just make get_timespec64 return an errno directly because it'll
require changing all the users.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid
  2018-03-26 21:55               ` Matthew Wilcox
@ 2018-03-27  4:43                 ` Deepa Dinamani
  0 siblings, 0 replies; 11+ messages in thread
From: Deepa Dinamani @ 2018-03-27  4:43 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Arnd Bergmann, Jeff Moyer, Leizhen (ThunderTown),
	Alexander Viro, Benjamin LaHaise, linux-fsdevel, linux-aio,
	linux-kernel, Tianhong Ding, Hanjun Guo, Libin, Kefeng Wang

On Mon, Mar 26, 2018 at 2:55 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Mar 26, 2018 at 10:01:30PM +0200, Arnd Bergmann wrote:
>> I had suggested a more complete helper function at some point,
>> to take care of all combinations of checking/non-checking, 32/64
>> bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
>> of nanoseconds before comparing against 1 billion, but Deepa
>> thought that was overkill, so I didn't continue that.
>
> Yeah, that sounds like a nightmare to use ;-)
>
>> For all I can tell, the get_timespec64() helper should almost always
>> include the check, the one exception I know is utimensat() and related
>> functions that may encode the special UTIME_NOW and UTIME_OMIT
>> constants in the nanoseconds.
>
> So do you endorse the get_valid_timespec64() patch I posted up-thread?
> We can't just make get_timespec64 return an errno directly because it'll
> require changing all the users.

I missed this thread earlier.

I had leaned away from this idea before, because of the special cases
which don't need it. I was also trying to keep the syntax close to
copy_from_user(), which is what was there before.

We could probably just change all the
get_timespec64()/compat_get_timespec64() to do the check using a
simple coccinelle script.
I can own this.

But, this would not be needed if Arnd is posting the other helper function.
Arnd, let me know what you prefer here.

-Deepa

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-03-27  4:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 13:42 [PATCH 1/1] aio: make sure the input "timeout" value is valid Zhen Lei
2017-12-13 14:11 ` Matthew Wilcox
2017-12-13 15:58   ` Benjamin LaHaise
2017-12-13 16:27   ` Jeff Moyer
2017-12-13 19:31     ` Matthew Wilcox
2017-12-14  3:18       ` Leizhen (ThunderTown)
2018-01-02 14:51         ` Matthew Wilcox
2018-01-12 19:49           ` Jeff Moyer
2018-03-26 20:01             ` Arnd Bergmann
2018-03-26 21:55               ` Matthew Wilcox
2018-03-27  4:43                 ` Deepa Dinamani

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).