[PATCH net-next v3 0/3] Permission checks for dynamic POSIX clocks

[PATCH net-next v3 0/3] Permission checks for dynamic POSIX clocks

[Wojtek Wasko]

Dynamic clocks - such as PTP clocks - extend beyond the standard POSIX
clock API by using ioctl calls. While file permissions are enforced for
standard POSIX operations, they are not implemented for ioctl calls,
since the POSIX layer cannot differentiate between calls which modify
the clock's state (like enabling PPS output generation) and those that
don't (such as retrieving the clock's PPS capabilities).

On the other hand, drivers implementing the dynamic clocks lack the
necessary information context to enforce permission checks themselves.

Add a struct file pointer to the POSIX clock context and use it to
implement the appropriate permission checks on PTP chardevs. Add a
readonly option to testptp.

Changes in v3:
- Reword the log message for commit against posix-clock and fix
  documentation of struct posix_clock_context, as suggested by Thomas

Changes in v2:
- Store file pointer in POSIX clock context rather than fmode in the PTP
  clock's private data, as suggested by Richard.
- Move testptp.c changes into separate patch.

Wojtek Wasko (3):
  posix-clock: Store file pointer in struct posix_clock_context
  ptp: Add file permission checks on PHCs
  testptp: add option to open PHC in readonly mode

 drivers/ptp/ptp_chardev.c             | 16 ++++++++++++
 include/linux/posix-clock.h           |  6 ++++-
 kernel/time/posix-clock.c             |  1 +
 tools/testing/selftests/ptp/testptp.c | 37 +++++++++++++++++----------
 4 files changed, 45 insertions(+), 15 deletions(-)

-- 
2.39.3

[PATCH net-next v3 1/3] posix-clock: Store file pointer in struct posix_clock_context

[Wojtek Wasko]

File descriptor based pc_clock_*() operations of dynamic posix clocks
have access to the file pointer and implement permission checks in the
generic code before invoking the relevant dynamic clock callback.

Character device operations (open, read, poll, ioctl) do not implement a
generic permission control and the dynamic clock callbacks have no
access to the file pointer to implement them.

Extend struct posix_clock_context with a struct file pointer and
initialize it in posix_clock_open(), so that all dynamic clock callbacks
can access it.

Signed-off-by: Wojtek Wasko <wwasko@nvidia.com>
---
 include/linux/posix-clock.h | 6 +++++-
 kernel/time/posix-clock.c   | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index ef8619f48920..40fa204baafc 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -95,10 +95,13 @@ struct posix_clock {
  * struct posix_clock_context - represents clock file operations context
  *
  * @clk:              Pointer to the clock
+ * @fp:               Pointer to the file used to open the clock
  * @private_clkdata:  Pointer to user data
  *
  * Drivers should use struct posix_clock_context during specific character
- * device file operation methods to access the posix clock.
+ * device file operation methods to access the posix clock. In particular,
+ * the file pointer can be used to verify correct access mode for ioctl()
+ * calls.
  *
  * Drivers can store a private data structure during the open operation
  * if they have specific information that is required in other file
@@ -106,6 +109,7 @@ struct posix_clock {
  */
 struct posix_clock_context {
 	struct posix_clock *clk;
+	struct file *fp;
 	void *private_clkdata;
 };
 
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 1af0bb2cc45c..4e114e34a6e0 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -129,6 +129,7 @@ static int posix_clock_open(struct inode *inode, struct file *fp)
 		goto out;
 	}
 	pccontext->clk = clk;
+	pccontext->fp = fp;
 	if (clk->ops.open) {
 		err = clk->ops.open(pccontext, fp->f_mode);
 		if (err) {
-- 
2.39.3

[PATCH net-next v3 2/3] ptp: Add file permission checks on PHCs

[Wojtek Wasko]

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(+)

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

[PATCH net-next v3 3/3] testptp: add option to open PHC in readonly mode

[Wojtek Wasko]

PTP Hardware Clocks no longer require WRITE permission to perform
readonly operations, such as listing device capabilities or listening to
EXTTS events once they have been enabled by a process with WRITE
permissions.

Add '-r' option to testptp to open the PHC in readonly mode instead of
the default read-write mode. Skip enabling EXTTS if readonly mode is
requested.

Signed-off-by: Wojtek Wasko <wwasko@nvidia.com>
---
 tools/testing/selftests/ptp/testptp.c | 37 +++++++++++++++++----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index 58064151f2c8..edc08a4433fd 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -140,6 +140,7 @@ static void usage(char *progname)
 		" -H val     set output phase to 'val' nanoseconds (requires -p)\n"
 		" -w val     set output pulse width to 'val' nanoseconds (requires -p)\n"
 		" -P val     enable or disable (val=1|0) the system clock PPS\n"
+		" -r         open the ptp clock in readonly mode\n"
 		" -s         set the ptp clock time from the system time\n"
 		" -S         set the system time from the ptp clock time\n"
 		" -t val     shift the ptp clock time by 'val' seconds\n"
@@ -188,6 +189,7 @@ int main(int argc, char *argv[])
 	int pin_index = -1, pin_func;
 	int pps = -1;
 	int seconds = 0;
+	int readonly = 0;
 	int settime = 0;
 	int channel = -1;
 	clockid_t ext_clockid = CLOCK_REALTIME;
@@ -200,7 +202,7 @@ int main(int argc, char *argv[])
 
 	progname = strrchr(argv[0], '/');
 	progname = progname ? 1+progname : argv[0];
-	while (EOF != (c = getopt(argc, argv, "cd:e:f:F:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xy:z"))) {
+	while (EOF != (c = getopt(argc, argv, "cd:e:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xy:z"))) {
 		switch (c) {
 		case 'c':
 			capabilities = 1;
@@ -252,6 +254,9 @@ int main(int argc, char *argv[])
 		case 'P':
 			pps = atoi(optarg);
 			break;
+		case 'r':
+			readonly = 1;
+			break;
 		case 's':
 			settime = 1;
 			break;
@@ -308,7 +313,7 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	fd = open(device, O_RDWR);
+	fd = open(device, readonly ? O_RDONLY : O_RDWR);
 	if (fd < 0) {
 		fprintf(stderr, "opening %s: %s\n", device, strerror(errno));
 		return -1;
@@ -436,14 +441,16 @@ int main(int argc, char *argv[])
 	}
 
 	if (extts) {
-		memset(&extts_request, 0, sizeof(extts_request));
-		extts_request.index = index;
-		extts_request.flags = PTP_ENABLE_FEATURE;
-		if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request)) {
-			perror("PTP_EXTTS_REQUEST");
-			extts = 0;
-		} else {
-			puts("external time stamp request okay");
+		if (!readonly) {
+			memset(&extts_request, 0, sizeof(extts_request));
+			extts_request.index = index;
+			extts_request.flags = PTP_ENABLE_FEATURE;
+			if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request)) {
+				perror("PTP_EXTTS_REQUEST");
+				extts = 0;
+			} else {
+				puts("external time stamp request okay");
+			}
 		}
 		for (; extts; extts--) {
 			cnt = read(fd, &event, sizeof(event));
@@ -455,10 +462,12 @@ int main(int argc, char *argv[])
 			       event.t.sec, event.t.nsec);
 			fflush(stdout);
 		}
-		/* Disable the feature again. */
-		extts_request.flags = 0;
-		if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request)) {
-			perror("PTP_EXTTS_REQUEST");
+		if (!readonly) {
+			/* Disable the feature again. */
+			extts_request.flags = 0;
+			if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request)) {
+				perror("PTP_EXTTS_REQUEST");
+			}
 		}
 	}
 
-- 
2.39.3

Re: [PATCH net-next v3 1/3] posix-clock: Store file pointer in struct posix_clock_context

[Thomas Gleixner]

On Mon, Feb 17 2025 at 11:50, Wojtek Wasko wrote:

> File descriptor based pc_clock_*() operations of dynamic posix clocks
> have access to the file pointer and implement permission checks in the
> generic code before invoking the relevant dynamic clock callback.
>
> Character device operations (open, read, poll, ioctl) do not implement a
> generic permission control and the dynamic clock callbacks have no
> access to the file pointer to implement them.
>
> Extend struct posix_clock_context with a struct file pointer and
> initialize it in posix_clock_open(), so that all dynamic clock callbacks
> can access it.
>
> Signed-off-by: Wojtek Wasko <wwasko@nvidia.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH net-next v3 2/3] ptp: Add file permission checks on PHCs

[Thomas Gleixner]

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?

RE: [PATCH net-next v3 2/3] ptp: Add file permission checks on PHCs

[Wojtek Wasko]

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

RE: [PATCH net-next v3 2/3] ptp: Add file permission checks on PHCs

[Thomas Gleixner]

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

Re: [PATCH net-next v3 2/3] ptp: Add file permission checks on PHCs

[Wojtek Wasko]

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