* [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 related [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 related [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).