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

[Rodolfo Giometti <giometti@enneenne.com>]

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;

> 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);

	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