| Message ID | 20250217095005.1453413-3-wwasko@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Permission checks for dynamic POSIX clocks | expand |
On Mon, Feb 17 2025 at 11:50, Wojtek Wasko wrote: > Many devices implement highly accurate clocks, which the kernel manages > as PTP Hardware Clocks (PHCs). Userspace applications rely on these > clocks to timestamp events, trace workload execution, correlate > timescales across devices, and keep various clocks in sync. > > The kernel’s current implementation of PTP clocks does not enforce file > permissions checks for most device operations except for POSIX clock > operations, where file mode is verified in the POSIX layer before > forwarding the call to the PTP subsystem. Consequently, it is common > practice to not give unprivileged userspace applications any access to > PTP clocks whatsoever by giving the PTP chardevs 600 permissions. An > example of users running into this limitation is documented in [1]. > > Add permission checks for functions that modify the state of a PTP > device. Continue enforcing permission checks for POSIX clock operations > (settime, adjtime) in the POSIX layer. One limitation remains: querying > the adjusted frequency of a PTP device (using adjtime() with an empty > modes field) is not supported for chardevs opened without WRITE > permissions, as the POSIX layer mandates WRITE access for any adjtime > operation. That's a fixable problem, no?
On Mon, Feb 17 2025 at 21:24, Thomas Gleixner wrote: > > One limitation > > remains: querying the adjusted frequency of a PTP device (using > > adjtime() with an empty modes field) is not supported for chardevs > > opened without WRITE permissions, as the POSIX layer mandates WRITE > > access for any adjtime operation. > > That's a fixable problem, no? Absolutely, but to be honest I wasn't sure about how to properly change the access check in adjtime given it's a "generic" API. I ended up with something along the lines of: if (tx->modes & ~(ADJ_NANO | ADJ_MICRO)) /* require WRITE */ being that ADJ_NANO and ADJ_MICRO by themselves don't mean the clock will be modified. So the modes field is not really "empty" per se and the check becomes less self-explanatory. But then maybe I'm overthinking it. If you think the above modes check is the right way to go, I'll send a v4 (using the opportunity to add the Reviewed-By and Acked-By tags from v2 I missed in v3 - I'm new to kernel development, sorry). Thanks, W
On Wed, Feb 19 2025 at 09:45, Wojtek Wasko wrote: > On Mon, Feb 17 2025 at 21:24, Thomas Gleixner wrote: >> > One limitation >> > remains: querying the adjusted frequency of a PTP device (using >> > adjtime() with an empty modes field) is not supported for chardevs >> > opened without WRITE permissions, as the POSIX layer mandates WRITE >> > access for any adjtime operation. >> >> That's a fixable problem, no? > > Absolutely, but to be honest I wasn't sure about how to properly change > the access check in adjtime given it's a "generic" API. I ended up with > something along the lines of: > > if (tx->modes & ~(ADJ_NANO | ADJ_MICRO)) > /* require WRITE */ > > being that ADJ_NANO and ADJ_MICRO by themselves don't mean the clock will > be modified. So the modes field is not really "empty" per se and the check > becomes less self-explanatory. ADJ_NANO and ADJ_MICRO modify the internal status. A read only operation has to have tx->modes == 0 and the result will be served in the NANO/MICRO representation which was set by the control application which can write. adjtimex(2) is clearly saying: "The modes field determines which parameters, if any, to set." Consequently modes != 0 requires CAP_SYS_TIME, while modes == 0 is unpriviledged. So requiring WRITE for the FD based posix clocks is not asked too much. Thanks, tglx
Thanks for the explanation, that made it clear. So in v4 I'll modify the condition for dynamic clocks adjtime to require WRITE only if modes != 0. Thanks, W
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index bf6468c56419..4380e6ddb849 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -205,6 +205,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, case PTP_EXTTS_REQUEST: case PTP_EXTTS_REQUEST2: + if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) { + err = -EACCES; + break; + } memset(&req, 0, sizeof(req)); if (copy_from_user(&req.extts, (void __user *)arg, @@ -246,6 +250,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, case PTP_PEROUT_REQUEST: case PTP_PEROUT_REQUEST2: + if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) { + err = -EACCES; + break; + } memset(&req, 0, sizeof(req)); if (copy_from_user(&req.perout, (void __user *)arg, @@ -314,6 +322,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, case PTP_ENABLE_PPS: case PTP_ENABLE_PPS2: + if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) { + err = -EACCES; + break; + } memset(&req, 0, sizeof(req)); if (!capable(CAP_SYS_TIME)) @@ -456,6 +468,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd, case PTP_PIN_SETFUNC: case PTP_PIN_SETFUNC2: + if ((pccontext->fp->f_mode & FMODE_WRITE) == 0) { + err = -EACCES; + break; + } if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) { err = -EFAULT; break;
Many devices implement highly accurate clocks, which the kernel manages as PTP Hardware Clocks (PHCs). Userspace applications rely on these clocks to timestamp events, trace workload execution, correlate timescales across devices, and keep various clocks in sync. The kernel’s current implementation of PTP clocks does not enforce file permissions checks for most device operations except for POSIX clock operations, where file mode is verified in the POSIX layer before forwarding the call to the PTP subsystem. Consequently, it is common practice to not give unprivileged userspace applications any access to PTP clocks whatsoever by giving the PTP chardevs 600 permissions. An example of users running into this limitation is documented in [1]. Add permission checks for functions that modify the state of a PTP device. Continue enforcing permission checks for POSIX clock operations (settime, adjtime) in the POSIX layer. One limitation remains: querying the adjusted frequency of a PTP device (using adjtime() with an empty modes field) is not supported for chardevs opened without WRITE permissions, as the POSIX layer mandates WRITE access for any adjtime operation. [1] https://lists.nwtime.org/sympa/arc/linuxptp-users/2024-01/msg00036.html Signed-off-by: Wojtek Wasko <wwasko@nvidia.com> --- drivers/ptp/ptp_chardev.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)