Re: [PATCH] pps: Don't try to wait for negative timeouts in PPS_FETCH
From: Rodolfo Giometti
Date: Thu Jun 11 2026 - 07:16:54 EST
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@xxxxxxxxxx>
Closes: https://sashiko.dev/#/patchset/cover.1779733602.git.calvin%40wbinvd.org?part=3
Signed-off-by: Calvin Owens <calvin@xxxxxxxxxx>
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@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxx
Embedded Systems phone: +39 349 2432127
UNIX programming
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxx
Embedded Systems phone: +39 349 2432127
UNIX programming