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

[Jan Kara]

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@xxxxxxxxxx>

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:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/select.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/select.c b/fs/select.c
> index 75978b18f48f8..bf71c9838dfe1 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -708,6 +708,17 @@ static int kern_select(int n, fd_set __user *inp, fd_set __user *outp,
>  		if (copy_from_user(&tv, tvp, sizeof(tv)))
>  			return -EFAULT;
>  
> +		/*
> +		 * Reject negative components before normalisation. The seconds
> +		 * sum below is performed in signed long and a crafted negative
> +		 * timeval can wrap to a positive value that passes
> +		 * timespec64_valid() and turns into an effectively-infinite
> +		 * deadline via timespec64_add_safe()'s saturation, instead of
> +		 * the -EINVAL POSIX requires for negative timeouts.
> +		 */
> +		if (tv.tv_sec < 0 || tv.tv_usec < 0)
> +			return -EINVAL;
> +
>  		to = &end_time;
>  		if (poll_select_set_timeout(to,
>  				tv.tv_sec + (tv.tv_usec / USEC_PER_SEC),
> 
> ---
> base-commit: 9974969c14031a097d6b45bcb7a06bb4aa525c40
> change-id: 20260429-timeval-8a3498dde479
> 
> Best regards,
> --  
> Breno Leitao <leitao@xxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR