diff mbox series

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

Message ID DM4PR12MB8558AB3C0DEA666EE334D8BCBEE82@DM4PR12MB8558.namprd12.prod.outlook.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,net-next] ptp: Add file permission checks on PHC chardevs | 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 fail Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: kuba@kernel.org andrew+netdev@lunn.ch pabeni@redhat.com edumazet@google.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 fail Errors and warnings before: 0 this patch: 1
netdev/checkpatch warning WARNING: return of an errno should typically be negative (ie: return -EACCES)
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

Commit Message

Wojtek Wasko Jan. 31, 2025, 6:29 p.m. UTC 
Udev sets strict 600 permissions on /dev/ptp* devices, preventing
unprivileged users from accessing the time [1]. This patch enables
more granular permissions and allows readonly access to the PTP clocks.

Add permission checking for ioctls which modify the state of device.
Notably, require WRITE for polling as it is only used for later reading
timestamps from the queue (there is no peek support). POSIX clock
operations (settime, adjtime) are checked in the POSIX layer.

[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 | 66 +++++++++++++++++++++++++++++++++++----
 drivers/ptp/ptp_private.h |  5 +++
 2 files changed, 65 insertions(+), 6 deletions(-)

Comments

Simon Horman Feb. 1, 2025, 4:02 p.m. UTC | #1 
On Fri, Jan 31, 2025 at 06:29:23PM +0000, Wojtek Wasko wrote:
> Udev sets strict 600 permissions on /dev/ptp* devices, preventing
> unprivileged users from accessing the time [1]. This patch enables
> more granular permissions and allows readonly access to the PTP clocks.
> 
> Add permission checking for ioctls which modify the state of device.
> Notably, require WRITE for polling as it is only used for later reading
> timestamps from the queue (there is no peek support). POSIX clock
> operations (settime, adjtime) are checked in the POSIX layer.
> 
> [1] https://lists.nwtime.org/sympa/arc/linuxptp-users/2024-01/msg00036.html
> 
> Signed-off-by: Wojtek Wasko <wwasko@nvidia.com>

...

> @@ -516,9 +554,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 ptp_private_ctxdata *ctxdata;
>  	struct timestamp_event_queue *queue;
>  
> -	queue = pccontext->private_clkdata;
> +	ctxdata = pccontext->private_clkdata;
> +	if (!ctxdata)
> +		return EPOLLERR;
> +	if ((ctxdata->fmode & FMODE_WRITE) == 0)
> +		return EACCES;

Hi Wojtek,

This is not a full review, but rather, something to take into account
if this idea goes forwards:

The return type of this function is __poll_t, not int.
So I think this should be EPOLLERR rather than EACCESS.

> +	queue = ctxdata->queue;
>  	if (!queue)
>  		return EPOLLERR;
>  

...
Vadim Fedorenko Feb. 1, 2025, 4:22 p.m. UTC | #2 
On 31/01/2025 18:29, Wojtek Wasko wrote:
> Udev sets strict 600 permissions on /dev/ptp* devices, preventing
> unprivileged users from accessing the time [1]. This patch enables
> more granular permissions and allows readonly access to the PTP clocks.
> 
> Add permission checking for ioctls which modify the state of device.
> Notably, require WRITE for polling as it is only used for later reading
> timestamps from the queue (there is no peek support). POSIX clock
> operations (settime, adjtime) are checked in the POSIX layer.
> 
> [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 | 66 +++++++++++++++++++++++++++++++++++----
>   drivers/ptp/ptp_private.h |  5 +++
>   2 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index bf6468c56419..5e6f404b9282 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -108,15 +108,22 @@ 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;
>   
> +	ctxdata = kzalloc(sizeof(*ctxdata), GFP_KERNEL);
> +	if (!ctxdata)
> +		return -EINVAL;
>   	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> -	if (!queue)
> +	if (!queue) {
> +		kfree(ctxdata);
>   		return -EINVAL;
> +	}

Given that struct ptp_private_ctxdata is quite simple, we can
potentially embed struct timestamp_event_queue into it and avoid
double allocation here?

>   	queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
>   	if (!queue->mask) {
> +		kfree(ctxdata);
>   		kfree(queue);
>   		return -EINVAL;
>   	}
> @@ -125,7 +132,9 @@ 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;
> +	ctxdata->queue = queue;
> +	ctxdata->fmode = fmode;
> +	pccontext->private_clkdata = ctxdata;
>   
>   	/* Debugfs contents */
>   	sprintf(debugfsname, "0x%p", queue);
> @@ -142,7 +151,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);
> @@ -154,6 +164,7 @@ int ptp_release(struct posix_clock_context *pccontext)
>   	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
>   	bitmap_free(queue->mask);
>   	kfree(queue);
> +	kfree(ctxdata);
>   	return 0;
>   }
>   
> @@ -167,6 +178,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 +192,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 +218,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 +264,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 +336,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 +482,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;
> @@ -485,10 +515,18 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
>   		break;
>   
>   	case PTP_MASK_CLEAR_ALL:
> +		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
> +			err = -EACCES;
> +			break;
> +		}
>   		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
>   		break;
>   
>   	case PTP_MASK_EN_SINGLE:
> +		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
> +			err = -EACCES;
> +			break;
> +		}
>   		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
>   			err = -EFAULT;
>   			break;
> @@ -516,9 +554,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 ptp_private_ctxdata *ctxdata;
>   	struct timestamp_event_queue *queue;
>   
> -	queue = pccontext->private_clkdata;
> +	ctxdata = pccontext->private_clkdata;
> +	if (!ctxdata)
> +		return EPOLLERR;
> +	if ((ctxdata->fmode & FMODE_WRITE) == 0)
> +		return EACCES;
> +	queue = ctxdata->queue;
>   	if (!queue)
>   		return EPOLLERR;
>   
> @@ -534,13 +578,23 @@ 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;
> +	ctxdata = pccontext->private_clkdata;
> +	if (!ctxdata) {
> +		result = -EINVAL;
> +		goto exit;
> +	}
> +	if ((ctxdata->fmode & FMODE_WRITE) == 0) {
> +		result = -EACCES;
> +		goto exit;
> +	}
> +	queue = ctxdata->queue;
>   	if (!queue) {
>   		result = -EINVAL;
>   		goto exit;
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 18934e28469e..fb4fa5c8c1c7 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -64,6 +64,11 @@ struct ptp_clock {
>   	struct dentry *debugfs_root;
>   };
>   
> +struct ptp_private_ctxdata {
> +	struct timestamp_event_queue *queue;
> +	fmode_t fmode;
> +};
> +
>   #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
>   #define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc)
>   #define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work)
Richard Cochran Feb. 2, 2025, 6:24 a.m. UTC | #3 
On Fri, Jan 31, 2025 at 06:29:23PM +0000, Wojtek Wasko wrote:
> Udev sets strict 600 permissions on /dev/ptp* devices, preventing

Udev is user space, isn't it?

> unprivileged users from accessing the time [1]. This patch enables
> more granular permissions and allows readonly access to the PTP clocks.
> 
> Add permission checking for ioctls which modify the state of device.
> Notably, require WRITE for polling as it is only used for later reading
> timestamps from the queue (there is no peek support). POSIX clock
> operations (settime, adjtime) are checked in the POSIX layer.

This change log is not comprehensible.
A proper change log has three parts:

1. context

2. problem   (What is the issue?)

3. solution  (How does the patch address the issue?)

Thanks,
Richard
diff mbox series

Patch

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index bf6468c56419..5e6f404b9282 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -108,15 +108,22 @@  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;
 
+	ctxdata = kzalloc(sizeof(*ctxdata), GFP_KERNEL);
+	if (!ctxdata)
+		return -EINVAL;
 	queue = kzalloc(sizeof(*queue), GFP_KERNEL);
-	if (!queue)
+	if (!queue) {
+		kfree(ctxdata);
 		return -EINVAL;
+	}
 	queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
 	if (!queue->mask) {
+		kfree(ctxdata);
 		kfree(queue);
 		return -EINVAL;
 	}
@@ -125,7 +132,9 @@  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;
+	ctxdata->queue = queue;
+	ctxdata->fmode = fmode;
+	pccontext->private_clkdata = ctxdata;
 
 	/* Debugfs contents */
 	sprintf(debugfsname, "0x%p", queue);
@@ -142,7 +151,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);
@@ -154,6 +164,7 @@  int ptp_release(struct posix_clock_context *pccontext)
 	spin_unlock_irqrestore(&ptp->tsevqs_lock, flags);
 	bitmap_free(queue->mask);
 	kfree(queue);
+	kfree(ctxdata);
 	return 0;
 }
 
@@ -167,6 +178,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 +192,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 +218,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 +264,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 +336,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 +482,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;
@@ -485,10 +515,18 @@  long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 		break;
 
 	case PTP_MASK_CLEAR_ALL:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
 		bitmap_clear(tsevq->mask, 0, PTP_MAX_CHANNELS);
 		break;
 
 	case PTP_MASK_EN_SINGLE:
+		if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+			err = -EACCES;
+			break;
+		}
 		if (copy_from_user(&i, (void __user *)arg, sizeof(i))) {
 			err = -EFAULT;
 			break;
@@ -516,9 +554,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 ptp_private_ctxdata *ctxdata;
 	struct timestamp_event_queue *queue;
 
-	queue = pccontext->private_clkdata;
+	ctxdata = pccontext->private_clkdata;
+	if (!ctxdata)
+		return EPOLLERR;
+	if ((ctxdata->fmode & FMODE_WRITE) == 0)
+		return EACCES;
+	queue = ctxdata->queue;
 	if (!queue)
 		return EPOLLERR;
 
@@ -534,13 +578,23 @@  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;
+	ctxdata = pccontext->private_clkdata;
+	if (!ctxdata) {
+		result = -EINVAL;
+		goto exit;
+	}
+	if ((ctxdata->fmode & FMODE_WRITE) == 0) {
+		result = -EACCES;
+		goto exit;
+	}
+	queue = ctxdata->queue;
 	if (!queue) {
 		result = -EINVAL;
 		goto exit;
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..fb4fa5c8c1c7 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -64,6 +64,11 @@  struct ptp_clock {
 	struct dentry *debugfs_root;
 };
 
+struct ptp_private_ctxdata {
+	struct timestamp_event_queue *queue;
+	fmode_t fmode;
+};
+
 #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
 #define cc_to_vclock(d) container_of((d), struct ptp_vclock, cc)
 #define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work)