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

[PATCH net-next v2 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 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 clocks: Store file pointer in 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 v2 1/3] posix clocks: Store file pointer in clock context

[Wojtek Wasko]

Dynamic clocks (e.g. PTP clocks) need access to the permissions with
which the clock was opened to enforce proper access control.

Native POSIX clocks have access to this information via
posix_clock_desc. However, it is not accessible from the implementation
of dynamic clocks.

Add struct file* to POSIX clock context for access from dynamic clocks.

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 for opening 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 custom
+ * 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 v2 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 v2 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 v2 0/3] Permission checks for dynamic POSIX clocks

[Richard Cochran]

On Tue, Feb 11, 2025 at 05:09:10PM +0200, Wojtek Wasko wrote:
> 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 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 clocks: Store file pointer in clock context
>   ptp: Add file permission checks on PHCs
>   testptp: Add option to open PHC in readonly mode

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net-next v2 0/3] Permission checks for dynamic POSIX clocks

[Vadim Fedorenko]

On 11/02/2025 15:09, Wojtek Wasko wrote:
> 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 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 clocks: Store file pointer in 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(-)
> 

For the series:
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net-next v2 1/3] posix clocks: Store file pointer in clock context

[Paolo Abeni]

Posix clock maintainers have not being CC-ed, adding them.

The whole series is available at:

https://lore.kernel.org/all/20250211150913.772545-1-wwasko@nvidia.com/

On 2/11/25 4:09 PM, Wojtek Wasko wrote:
> Dynamic clocks (e.g. PTP clocks) need access to the permissions with
> which the clock was opened to enforce proper access control.
> 
> Native POSIX clocks have access to this information via
> posix_clock_desc. However, it is not accessible from the implementation
> of dynamic clocks.
> 
> Add struct file* to POSIX clock context for access from dynamic clocks.
> 
> 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 for opening 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 custom
> + * 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) {

Re: [PATCH net-next v2 1/3] posix clocks: Store file pointer in clock context

[Thomas Gleixner]

On Thu, Feb 13 2025 at 12:37, Paolo Abeni wrote:
> Posix clock maintainers have not being CC-ed, adding them.

Tx!

> $Subject: posix clocks: Store file pointer in clock context

s/posix_clocks:/posix-clock:/

s/clock context/struct posix_clock_context/

>> Dynamic clocks (e.g. PTP clocks) need access to the permissions with
>> which the clock was opened to enforce proper access control.
>> 
>> Native POSIX clocks have access to this information via
>> posix_clock_desc. However, it is not accessible from the implementation
>> of dynamic clocks.

>> Add struct file* to POSIX clock context for access from dynamic
>> clocks.

What is a native posix clock? posix_clock_desc is used in the context of
dynamic posix clocks, no?

I assume this wants to say:

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

  The character device operations (open, read, poll, ioctl) do not have
  a generic permission control and the PTP 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 PTP clock callbacks
  can access it.

Or something like that, right?
 
>> @@ -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 for opening 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 custom
>> + * ioctl calls.

s/custom ioctl calls/ioctl() calls/

Other than that this looks sane.

Thanks,

        tglx

RE: [PATCH net-next v2 1/3] posix clocks: Store file pointer in clock context

[Wojtek Wasko]

On Thu, Feb 13 2025 at 12:37, Paolo Abeni wrote:
> Posix clock maintainers have not being CC-ed, adding them.
Thank you! Initially this patch was fully contained in the ptp subsystem which is why I missed that.

On Thu, Feb 13 2025 at 23:17, Thomas Gleixner wrote:
> Other than that this looks sane.
I'll wait a day to see if there's any feedback from other posix-clock maintainers and post a v3 with revised wording.

Thanks,
Wojtek