diff mbox series

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

Message ID DM4PR12MB8558CE01707ED1DD3305A9FCBEF62@DM4PR12MB8558.namprd12.prod.outlook.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] ptp: Add file permission checks on PHC | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 6 maintainers not CCed: maheshb@google.com shuah@kernel.org andrew+netdev@lunn.ch linux-kselftest@vger.kernel.org edumazet@google.com pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: braces {} are not necessary for single statement blocks WARNING: line length of 96 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-02-06--12-00 (tests: 891)

Commit Message

Wojtek Wasko Feb. 6, 2025, 11:03 a.m. UTC
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(-)

Comments

Vadim Fedorenko Feb. 6, 2025, 10:23 p.m. UTC | #1
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>
Richard Cochran Feb. 7, 2025, 3:26 p.m. UTC | #2
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
diff mbox series

Patch

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");
+			}
 		}
 	}