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

[Simon Horman <horms@kernel.org>]

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;
>  

...