Re: [PATCH] pps: Don't try to wait for negative timeouts in PPS_FETCH

[Calvin Owens <calvin@wbinvd.org>]

On Saturday 05/30 at 11:50 +0200, Rodolfo Giometti wrote:
> On 29/05/2026 18:21, Calvin Owens wrote:
> > If userspace passes a negative timeout to PPS_FETCH, it triggers a
> > kernel splat from schedule_timeout():
> > 
> >      schedule_timeout: wrong timeout value fffffffffff0bfb4
> >      CPU: 17 UID: 0 PID: 4720 Comm: a.out Not tainted 7.1.0-rc5-x86-kvm-00150-g331d97e36b37 #1 PREEMPT_RT
> >      Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-20240910_120124-localhost 04/01/2014
> >      Call Trace:
> >       <TASK>
> >       dump_stack_lvl+0x4b/0x70
> >       schedule_timeout+0xb7/0xe0
> >       pps_cdev_pps_fetch.isra.0+0x93/0x150
> >       pps_cdev_ioctl+0x70/0x310
> >       __x64_sys_ioctl+0x7b/0xc0
> >       do_syscall_64+0xb6/0xfc0
> >       entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > 
> > Sashiko imagines this to be some sort of security problem, which is
> > obviously really silly. But I think it is still worth fixing, so buggy
> > userspace code can't trigger the splat.
> > 
> > Silence the splat by skipping the wait if the ticks count is negative.
> > The current behavior is to return -ETIMEDOUT in that case, so keep that
> > return value in case any userspace code might rely on it.
> > 
> > Fixes: eae9d2ba0cfc ("LinuxPPS: core support")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Closes: https://sashiko.dev/#/patchset/cover.1779733602.git.calvin%40wbinvd.org?part=3
> > Signed-off-by: Calvin Owens <calvin@wbinvd.org>

Hi Rodolfo,

Thanks for taking a look at this and the others.

> > ---
> >   drivers/pps/pps.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> > index de1122bb69ea..6755901fbdae 100644
> > --- a/drivers/pps/pps.c
> > +++ b/drivers/pps/pps.c
> > @@ -65,17 +65,19 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
> >   	if (fdata->timeout.flags & PPS_TIME_INVALID)
> >   		err = wait_event_interruptible(pps->queue,
> >   				ev != pps->last_ev);
> >   	else {
> > -		unsigned long ticks;
> > +		long ticks;
> >   		dev_dbg(&pps->dev, "timeout %lld.%09d\n",
> >   				(long long) fdata->timeout.sec,
> >   				fdata->timeout.nsec);
> >   		ticks = fdata->timeout.sec * HZ;
> >   		ticks += fdata->timeout.nsec / (NSEC_PER_SEC / HZ);
> > -		if (ticks != 0) {
> > +		if (ticks < 0) {
> > +			return -ETIMEDOUT;
> > +		} else if (ticks > 0) {
> >   			err = wait_event_interruptible_timeout(
> >   					pps->queue,
> >   					ev != pps->last_ev,
> >   					ticks);
> 
> Should the problem originate from user-space data, I deem it more
> appropriate to verify them directly, rather than relying on computed data.
> 
> 	unsigned long ticks;
> 
> 	if (fdata->timeout.sec < 0 || fdata->timeout.nsec < 0)
> 		return -ETIMEDOUT;

I agree your way is nicer to read.

Pedantically, it's a user visable behavior change: today, the user
can pass absurd values for sec and nsec, and everything works so long as
the math works out to positive ticks (e.g. sec=ULONG_MAX/HZ,
nsec=2*HZ*NSEC_PER_SEC).

If it was just that, it wouldn't really matter IMO, but...

> 	dev_dbg(&pps->dev, "timeout %lld.%09d\n",
> 			(long long) fdata->timeout.sec,
> 			fdata->timeout.nsec);
> 		
> 	ticks = fdata->timeout.sec * HZ;

...the multiplication by HZ could also overflow even if sec is positive,
so I think userspace would still be able to trigger the splat this way.

> 	ticks += fdata->timeout.nsec / (NSEC_PER_SEC / HZ);
> 
> 	if (ticks > 0) {
> 
> Ciao,
> 
> Rodolfo
> 
> -- 
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming