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

[Rodolfo Giometti <giometti@enneenne.com>]

On 10/06/2026 17:48, Calvin Owens wrote:
> 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.

I agree with you.

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

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming