| Message ID | 1513172572-16724-1-git-send-email-thunder.leizhen@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] aio: make sure the input "timeout" value is valid | expand |
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);
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."
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>
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.
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
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?
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
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
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.
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
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);
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(+) -- 1.8.3