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

[Calvin Owens <calvin@wbinvd.org>]

On Monday 06/01 at 09:04 +0200, Rodolfo Giometti wrote:
> On 30/05/2026 16:54, Calvin Owens wrote:
> > 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.
> 
> We can also do as follow:
> 
> 	/* Canonical validation of nanoseconds */
> 	if (fdata->timeout.nsec < 0 || fdata->timeout.nsec >= NSEC_PER_SEC)
> 		return -EINVAL;
> 
> 	/* Preserve historical API behavior for negative seconds */
> 	if (fdata->timeout.sec < 0)
> 		return -ETIMEDOUT;

Thinking about this a little more... code in the kernel generally doesn't
try to enforce tv_nsec < NSEC_PER_SEC from userspace today.

I think nobody cares: let's just not worry about tv_nsec unless you
really think a real user will care.

If it's worth fixing, it ought to be done more systematically throughout
the kernel, I feel like adding the check in random places makes the
situation more confusing overall.

But again... I seriously doubt any real user cares. I'll look into the
history around this a bit more, but I'm very disinclined to think it's
worth the reviewer time to try and push it more broadly...

This KC code also depends on CONFIG_HZ_PERIODIC, which no distro ships,
so any user must necessarily be building their own kernel.

> > 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) {
> 
> We can do as follow:
> 
> 	/* Safe conversion using standard kernel API */
> 	ts.tv_sec = fdata->timeout.sec;
> 	ts.tv_nsec = fdata->timeout.nsec;
> 	ticks = timespec64_to_jiffies(&ts);

Ah that helper is nicer, thanks.

This plus the (tv_sec < 0) change should be sufficient, I'll send that
along soon unless you reply that you want the tv_nsec check too.

> 	if (ticks > 0) {
> 		err = wait_event_interruptible_timeout(
> 				pps->queue,
> 				ev != pps->last_ev,
> 				ticks);
> 	}
> 
> Ciao,
> 
> Rodolfo
> 
> -- 
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming