From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
To: Wojtek Wasko <wwasko@nvidia.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: "richardcochran@gmail.com" <richardcochran@gmail.com>,
Jakub Kicinski <kuba@kernel.org>, Simon Horman <horms@kernel.org>
Subject: Re: [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs
Date: Sat, 1 Feb 2025 16:22:14 +0000 [thread overview]
Message-ID: <8085eef9-9724-4148-bdfe-2b7e2066997d@linux.dev> (raw)
In-Reply-To: <DM4PR12MB8558AB3C0DEA666EE334D8BCBEE82@DM4PR12MB8558.namprd12.prod.outlook.com>
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)
next prev parent reply other threads:[~2025-02-01 16:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-31 18:29 [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs Wojtek Wasko
2025-02-01 16:02 ` Simon Horman
2025-02-01 16:22 ` Vadim Fedorenko [this message]
2025-02-02 6:24 ` Richard Cochran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8085eef9-9724-4148-bdfe-2b7e2066997d@linux.dev \
--to=vadim.fedorenko@linux.dev \
--cc=horms@kernel.org \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=wwasko@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.