[PATCH RESEND net] ptp: Ensure info->enable callback is always set

[PATCH RESEND net] ptp: Ensure info->enable callback is always set

[Thomas Weißschuh]

The ioctl and sysfs handlers unconditionally call the ->enable callback.
Not all drivers implement that callback, leading to NULL dereferences.
Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c.

Instead use a dummy callback if no better was specified by the driver.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/ptp/ptp_clock.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
 		return info->gettime64(info, ts);
 }
 
+static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on)
+{
+	return -EOPNOTSUPP;
+}
+
 static void ptp_aux_kworker(struct kthread_work *work)
 {
 	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
@@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
 	}
 
+	if (!ptp->info->enable)
+		ptp->info->enable = ptp_enable;
+
 	if (ptp->info->do_aux_work) {
 		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
 		ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index);

---
base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82
change-id: 20250122-ptp-enable-831339c62428

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

Re: [PATCH RESEND net] ptp: Ensure info->enable callback is always set

[Michal Swiatkowski]

On Thu, Jan 23, 2025 at 08:22:40AM +0100, Thomas Weißschuh wrote:
> The ioctl and sysfs handlers unconditionally call the ->enable callback.
> Not all drivers implement that callback, leading to NULL dereferences.
> Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c.
> 
> Instead use a dummy callback if no better was specified by the driver.
> 
> Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/ptp/ptp_clock.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
>  		return info->gettime64(info, ts);
>  }
>  
> +static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  static void ptp_aux_kworker(struct kthread_work *work)
>  {
>  	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
> @@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
>  	}
>  
> +	if (!ptp->info->enable)
> +		ptp->info->enable = ptp_enable;
> +
>  	if (ptp->info->do_aux_work) {
>  		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
>  		ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index);
> 
> ---
> base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82
> change-id: 20250122-ptp-enable-831339c62428
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

What about other ops, did you check it too? Looks like it isn't needed,
but it sometimes hard to follow.

Thanks

Re: [PATCH RESEND net] ptp: Ensure info->enable callback is always set

[Thomas Weißschuh]

On 2025-01-23 10:38:37+0100, Michal Swiatkowski wrote:
> On Thu, Jan 23, 2025 at 08:22:40AM +0100, Thomas Weißschuh wrote:
> > The ioctl and sysfs handlers unconditionally call the ->enable callback.
> > Not all drivers implement that callback, leading to NULL dereferences.
> > Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c.
> > 
> > Instead use a dummy callback if no better was specified by the driver.
> > 
> > Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  drivers/ptp/ptp_clock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
> >  		return info->gettime64(info, ts);
> >  }
> >  
> > +static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> >  static void ptp_aux_kworker(struct kthread_work *work)
> >  {
> >  	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
> > @@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> >  			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> >  	}
> >  
> > +	if (!ptp->info->enable)
> > +		ptp->info->enable = ptp_enable;
> > +
> >  	if (ptp->info->do_aux_work) {
> >  		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> >  		ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index);
> > 
> > ---
> > base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82
> > change-id: 20250122-ptp-enable-831339c62428
> > 
> > Best regards,
> > -- 
> > Thomas Weißschuh <linux@weissschuh.net>
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> 
> What about other ops, did you check it too? Looks like it isn't needed,
> but it sometimes hard to follow.

I couldn't find any missing, but I'm not familiar with the subsystem and
didn't check too hard.

Note:

A follow-up fix would be to actually guard the users of ->enable and
error out. For sysfs the attributes could be hidden completely.
That would be a nicer user interface but more code change which are not
so easily backportable.


Thomas

Re: [PATCH RESEND net] ptp: Ensure info->enable callback is always set

[Richard Cochran]

On Thu, Jan 23, 2025 at 02:19:46PM +0100, Thomas Weißschuh wrote:
> On 2025-01-23 10:38:37+0100, Michal Swiatkowski wrote:
> > What about other ops, did you check it too? Looks like it isn't needed,
> > but it sometimes hard to follow.
> 
> I couldn't find any missing, but I'm not familiar with the subsystem and
> didn't check too hard.

Initially all of the callbacks were required, but that requirement
became relaxed over time with getcycles64().

Now that we have more and more drivers, it wouldn't hurt to let
ptp_clock_register() check that the needed callbacks are valid.

> Note:
> 
> A follow-up fix would be to actually guard the users of ->enable and
> error out.

Yes, I would place checks at the call sites, within ptp_ioctl().

Thanks,
Richard

Re: [PATCH RESEND net] ptp: Ensure info->enable callback is always set

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 23 Jan 2025 08:22:40 +0100 you wrote:
> The ioctl and sysfs handlers unconditionally call the ->enable callback.
> Not all drivers implement that callback, leading to NULL dereferences.
> Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c.
> 
> Instead use a dummy callback if no better was specified by the driver.
> 
> Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> [...]

Here is the summary with links:
  - [RESEND,net] ptp: Ensure info->enable callback is always set
    https://git.kernel.org/netdev/net/c/fd53aa40e65f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html