[PATCH net-next] ptp: Add file permission checks on PHC

[PATCH net-next] ptp: Add file permission checks on PHC

[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].

This patch adds permission checks for functions that modify the state of
a PTP device. POSIX clock operations (settime, adjtime) continue to be
checked 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             | 52 ++++++++++++++++++++-------
 drivers/ptp/ptp_private.h             |  5 +++
 tools/testing/selftests/ptp/testptp.c | 37 +++++++++++--------
 3 files changed, 67 insertions(+), 27 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index bf6468c56419..c86a31395cdf 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -108,16 +108,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
+	struct ptp_private_ctxdata *ctxdata;
 	struct timestamp_event_queue *queue;
 	char debugfsname[32];
 	unsigned long flags;
 
-	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
-	if (!queue)
+	ctxdata = kzalloc(sizeof(*ctxdata), GFP_KERNEL);
+	if (!ctxdata)
 		return -EINVAL;
+	ctxdata->fmode = fmode;
+
+	queue = &ctxdata->queue;
 	queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
 	if (!queue->mask) {
-		kfree(queue);
+		kfree(ctxdata);
 		return -EINVAL;
 	}
 	bitmap_set(queue->mask, 0, PTP_MAX_CHANNELS);
@@ -125,7 +129,7 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
 	spin_lock_irqsave(&ptp->tsevqs_lock, flags);
 	list_add_tail(&queue->qlist, &ptp->tsevqs);
 	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
-	pccontext->private_clkdata = queue;
+	pccontext->private_clkdata = ctxdata;
 
 	/* Debugfs contents */
 	sprintf(debugfsname, "0x%p", queue);
@@ -142,7 +146,8 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
 
 int ptp_release(struct posix_clock_context *pccontext)
 {
-	struct timestamp_event_queue *queue = pccontext->private_clkdata;
+	struct ptp_private_ctxdata *ctxdata = pccontext->private_clkdata;
+	struct timestamp_event_queue *queue = &ctxdata->queue;
 	unsigned long flags;
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
@@ -153,7 +158,7 @@ int ptp_release(struct posix_clock_context *pccontext)
 	list_del(&queue->qlist);
 	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
 	bitmap_free(queue->mask);
-	kfree(queue);
+	kfree(ctxdata);
 	return 0;
 }
 
@@ -167,6 +172,7 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_private_ctxdata *ctxdata;
 	struct timestamp_event_queue *tsevq;
 	struct ptp_system_timestamp sts;
 	struct ptp_clock_request req;
@@ -180,7 +186,8 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	if (in_compat_syscall() && cmd != PTP_ENABLE_PPS && cmd != PTP_ENABLE_PPS2)
 		arg = (unsigned long)compat_ptr(arg);
 
-	tsevq = pccontext->private_clkdata;
+	ctxdata = pccontext->private_clkdata;
+	tsevq = &ctxdata->queue;
 
 	switch (cmd) {
 
@@ -205,6 +212,11 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 
 	case PTP_EXTTS_REQUEST:
 	case PTP_EXTTS_REQUEST2:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
+
 		memset(&req, 0, sizeof(req));
 
 		if (copy_from_user(&req.extts, (void __user *)arg,
@@ -246,6 +258,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 
 	case PTP_PEROUT_REQUEST:
 	case PTP_PEROUT_REQUEST2:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
 		memset(&req, 0, sizeof(req));
 
 		if (copy_from_user(&req.perout, (void __user *)arg,
@@ -314,6 +330,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 
 	case PTP_ENABLE_PPS:
 	case PTP_ENABLE_PPS2:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
 		memset(&req, 0, sizeof(req));
 
 		if (!capable(CAP_SYS_TIME))
@@ -456,6 +476,10 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 
 	case PTP_PIN_SETFUNC:
 	case PTP_PIN_SETFUNC2:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
 			err = -EFAULT;
 			break;
@@ -516,15 +540,15 @@ __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
-	struct timestamp_event_queue *queue;
+	struct ptp_private_ctxdata *ctxdata;
 
-	queue = pccontext->private_clkdata;
-	if (!queue)
+	ctxdata = pccontext->private_clkdata;
+	if (!ctxdata)
 		return EPOLLERR;
 
 	poll_wait(fp, &ptp->tsev_wq, wait);
 
-	return queue_cnt(queue) ? EPOLLIN : 0;
+	return queue_cnt(&ctxdata->queue) ? EPOLLIN : 0;
 }
 
 #define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
@@ -534,17 +558,19 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
 {
 	struct ptp_clock *ptp =
 		container_of(pccontext->clk, struct ptp_clock, clock);
+	struct ptp_private_ctxdata *ctxdata;
 	struct timestamp_event_queue *queue;
 	struct ptp_extts_event *event;
 	unsigned long flags;
 	size_t qcnt, i;
 	int result;
 
-	queue = pccontext->private_clkdata;
-	if (!queue) {
+	ctxdata = pccontext->private_clkdata;
+	if (!ctxdata) {
 		result = -EINVAL;
 		goto exit;
 	}
+	queue = &ctxdata->queue;
 
 	if (cnt % sizeof(struct ptp_extts_event) != 0) {
 		result = -EINVAL;
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..b14e7d26a11c 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -35,6 +35,11 @@ struct timestamp_event_queue {
 	struct debugfs_u32_array dfs_bitmap;
 };
 
+struct ptp_private_ctxdata {
+	struct timestamp_event_queue queue;
+	fmode_t fmode;
+};
+
 struct ptp_clock {
 	struct posix_clock clock;
 	struct device dev;
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.27.0

Re: [PATCH net-next] ptp: Add file permission checks on PHC

[Vadim Fedorenko]

On 06/02/2025 11:03, 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].
> 
> This patch adds permission checks for functions that modify the state of
> a PTP device. POSIX clock operations (settime, adjtime) continue to be
> checked 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             | 52 ++++++++++++++++++++-------
>   drivers/ptp/ptp_private.h             |  5 +++
>   tools/testing/selftests/ptp/testptp.c | 37 +++++++++++--------
>   3 files changed, 67 insertions(+), 27 deletions(-)

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

Re: [PATCH net-next] ptp: Add file permission checks on PHC

[Richard Cochran]

Wojtek,

The commit message is much impoved, thanks.

On Thu, Feb 06, 2025 at 11:03:35AM +0000, 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].
> 
> This patch adds permission checks for functions that modify the state of

Can you change the wording to imperative voice please?
(grep for "this patch" under Documentation)

> a PTP device. POSIX clock operations (settime, adjtime) continue to be
> checked 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.

> @@ -108,16 +108,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
>  {
>  	struct ptp_clock *ptp =
>  		container_of(pccontext->clk, struct ptp_clock, clock);
> +	struct ptp_private_ctxdata *ctxdata;
>  	struct timestamp_event_queue *queue;
>  	char debugfsname[32];
>  	unsigned long flags;
>  
> -	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> -	if (!queue)
> +	ctxdata = kzalloc(sizeof(*ctxdata), GFP_KERNEL);
> +	if (!ctxdata)
>  		return -EINVAL;
> +	ctxdata->fmode = fmode;

This will fix the issue only for the PTP "sub-class" of posix clock...

> +struct ptp_private_ctxdata {
> +	struct timestamp_event_queue queue;
> +	fmode_t fmode;
> +};

Can you please move the `fmode` into `posix_clock_context` ?
(or maybe even the whole `struct file`)
(the change to posix-clock.c can be a separate patch)

In that way,

1) Future implementations of posix clock ops will not repeat the same bug.

2) Less churn in ptp_open()


Thanks,
Richard