Re: [PATCH] fs/select: reject negative timeval components in kern_select()

[Breno Leitao <leitao@debian.org>]

On Thu, Apr 30, 2026 at 09:33:01AM +0200, Jan Kara wrote:
> On Wed 29-04-26 06:09:37, Breno Leitao wrote:
> > kern_select() normalises the user-supplied struct __kernel_old_timeval
> > with
> >
> > 	tv.tv_sec + (tv.tv_usec / USEC_PER_SEC)
> > 	(tv.tv_usec % USEC_PER_SEC) * NSEC_PER_USEC
> >
> > before calling poll_select_set_timeout() -> timespec64_valid().  Both
> > operands of the seconds sum are unbounded user-controlled signed long.
> > A crafted pair where tv_usec is a negative multiple of USEC_PER_SEC
> > drives the sum across the wrap boundary - e.g.
> >
> > 	{ .tv_sec = LONG_MIN, .tv_usec = -1000000 }
> >
> > yields sec = LONG_MAX, nsec = 0, which passes timespec64_valid() and
> > then flows through timespec64_add_safe(), which saturates the absolute
> > deadline to TIME64_MAX (clamped further to KTIME_MAX downstream).
> > select(2) therefore blocks effectively forever instead of returning
> > -EINVAL as POSIX requires for a negative timeout.
> >
> > Only the legacy __NR_select syscall takes this path.  pselect6, ppoll,
> > poll and epoll_pwait2 all hand the user's two fields directly to
> > poll_select_set_timeout(), which validates *before* doing any
> > arithmetic:
> >
> > 	/* fs/select.c:271 -- the validator */
> > 	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;
> > 		...
> > 	}
> >
> > 	/* include/linux/time64.h:97 -- timespec64_valid */
> > 	if (ts->tv_sec < 0)                              return false;
> > 	if ((unsigned long)ts->tv_nsec >= NSEC_PER_SEC)  return false;
> >
> > 	/* fs/select.c:744  do_pselect() (pselect6, pselect6_time32) */
> > 	if (get_timespec64(&ts, tsp)) return -EFAULT;
> > 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec)) return -EINVAL;
> >
> > 	/* fs/select.c:1097 ppoll */
> > 	if (get_timespec64(&ts, tsp)) return -EFAULT;
> > 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec)) return -EINVAL;
> >
> > 	/* fs/select.c:1065 poll -- timeout_msecs is int; >= 0 gates the math */
> > 	if (timeout_msecs >= 0)
> > 		poll_select_set_timeout(to, timeout_msecs / MSEC_PER_SEC,
> > 		                        NSEC_PER_MSEC * (timeout_msecs % MSEC_PER_SEC));
> >
> > 	/* fs/eventpoll.c:2512 epoll_pwait2 */
> > 	if (get_timespec64(&ts, timeout)) return -EFAULT;
> > 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec)) return -EINVAL;
> >
> > In every one of these the wrap-prone arithmetic from kern_select()
> > simply does not exist; the user fields reach timespec64_valid()
> > unmodified.  glibc routes the C-library select() through pselect6,
> > so the bug is reachable only via a direct syscall(__NR_select, ...).
> >
> > The pre-validation negative check that used to live here was lost
> > when the syscall was switched to the poll_select_set_timeout() helper.
> > Restore it: reject tv_sec < 0 || tv_usec < 0 up front, mirroring what
> > glibc does in userspace.  do_compat_select() has the same arithmetic
> > pattern but is only reachable on 32-bit compat and from a different
> > syscall entry; left for a follow-up so this change stays minimal.
> >
> > Reproducer (returns -1/EINVAL on a fixed kernel; blocks indefinitely
> > on an unfixed one):
> >
> > 	struct timeval tv = { .tv_sec = LONG_MIN, .tv_usec = -1000000 };
> > 	fd_set r;
> > 	int pfd[2];
> > 	pipe(pfd);
> > 	FD_ZERO(&r);
> > 	FD_SET(pfd[0], &r);
> > 	syscall(__NR_select, pfd[0] + 1, &r, NULL, NULL, &tv);
> >
> > Fixes: 4d36a9e65d49 ("select: deal with math overflow from borderline valid userland data")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
>
> Looks good. I just wonder whether we shouldn't also check that tv.tv_usec <
> USEC_PER_SEC. But in any case feel free to add:

Good question. I opted not to add that check because it would represent
an ABI change—for example, it would start rejecting { .tv_sec = 0,
.tv_usec = 2000000 }.

That said, glibc already performs this check, so applications using libc
already have this constraint.

I'm comfortable with either approach.

Thanks for the review,
--breno