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

From: Vadim Fedorenko <[email-masked]>
Date: Sat, 1 Feb 2025 16:22:14 +0000

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 <[email-masked]>
> ---
>   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)