* [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs
@ 2025-01-31 18:29 Wojtek Wasko
2025-02-01 16:02 ` Simon Horman
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Wojtek Wasko @ 2025-01-31 18:29 UTC (permalink / raw)
To: netdev@vger.kernel.org; +Cc: richardcochran@gmail.com
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;
+ }
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)
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs
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
2025-02-02 6:24 ` Richard Cochran
2 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2025-02-01 16:02 UTC (permalink / raw)
To: Wojtek Wasko; +Cc: netdev, Richard Cochran
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;
>
...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs
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
2025-02-02 6:24 ` Richard Cochran
2 siblings, 0 replies; 4+ messages in thread
From: Vadim Fedorenko @ 2025-02-01 16:22 UTC (permalink / raw)
To: Wojtek Wasko, netdev@vger.kernel.org
Cc: richardcochran@gmail.com, Jakub Kicinski, Simon Horman
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC net-next] ptp: Add file permission checks on PHC chardevs
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
@ 2025-02-02 6:24 ` Richard Cochran
2 siblings, 0 replies; 4+ messages in thread
From: Richard Cochran @ 2025-02-02 6:24 UTC (permalink / raw)
To: Wojtek Wasko; +Cc: netdev@vger.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
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-02-02 6:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2025-02-02 6:24 ` Richard Cochran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).