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,
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
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
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
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!
linux-stable-mirror@lists.linaro.org
-
Michal Swiatkowski -
patchwork-bot+netdevbpf＠kernel.org -
Richard Cochran -
Thomas Weißschuh