linux-kernel - [PATCH] RFC: ptp: add comment about register access race

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250227141749.3767032-1-arnd@kernel.org>
Date: Thu, 27 Feb 2025 15:17:27 +0100
From: Arnd Bergmann <arnd@...nel.org>
To: Richard Cochran <richardcochran@...il.com>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Tianfei Zhang <tianfei.zhang@...el.com>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Thomas Weißschuh <linux@...ssschuh.net>,
	Calvin Owens <calvin@...nvd.org>,
	Philipp Stanner <pstanner@...hat.com>,
	netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-fpga@...r.kernel.org
Subject: [PATCH] RFC: ptp: add comment about register access race

From: Arnd Bergmann <arnd@...db.de>

While reviewing a patch to the ioread64_hi_lo() helpers, I noticed
that there are several PTP drivers that use multiple register reads
to access a 64-bit hardware register in a racy way.

There are usually safe ways of doing this, but at least these four
drivers do that.  A third register read obviously makes the hardware
access 50% slower. If the low word counds nanoseconds and a single
register read takes on the order of 1µs, the resulting value is
wrong in one of 4 million cases, which is pretty rare but common
enough that it would be observed in practice.

Link: https://lore.kernel.org/all/96829b49-62a9-435b-9e35-fe3ef01d1c67@app.fastmail.com/
Signed-off-by: Arnd Bergmann <arnd@...db.de>
---
Sorry I hadn't sent this out as a proper patch so far. Any ideas
what we should do here?
---
 drivers/net/ethernet/xscale/ptp_ixp46x.c | 8 ++++++++
 drivers/ptp/ptp_dfl_tod.c                | 4 ++++
 drivers/ptp/ptp_ocp.c                    | 4 ++++
 drivers/ptp/ptp_pch.c                    | 8 ++++++++
 4 files changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/xscale/ptp_ixp46x.c b/drivers/net/ethernet/xscale/ptp_ixp46x.c
index 94203eb46e6b..e9c8589fdef0 100644
--- a/drivers/net/ethernet/xscale/ptp_ixp46x.c
+++ b/drivers/net/ethernet/xscale/ptp_ixp46x.c
@@ -43,6 +43,10 @@ static u64 ixp_systime_read(struct ixp46x_ts_regs *regs)
 	u64 ns;
 	u32 lo, hi;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	lo = __raw_readl(&regs->systime_lo);
 	hi = __raw_readl(&regs->systime_hi);
 
@@ -61,6 +65,10 @@ static void ixp_systime_write(struct ixp46x_ts_regs *regs, u64 ns)
 	hi = ns >> 32;
 	lo = ns & 0xffffffff;
 
+	/*
+	 * This can equally overflow when the low half of the new
+	 * nanosecond value is close to 0xffffffff.
+	 */
 	__raw_writel(lo, &regs->systime_lo);
 	__raw_writel(hi, &regs->systime_hi);
 }
diff --git a/drivers/ptp/ptp_dfl_tod.c b/drivers/ptp/ptp_dfl_tod.c
index f699d541b360..1eed76c3a256 100644
--- a/drivers/ptp/ptp_dfl_tod.c
+++ b/drivers/ptp/ptp_dfl_tod.c
@@ -104,6 +104,10 @@ static int coarse_adjust_tod_clock(struct dfl_tod *dt, s64 delta)
 	if (delta == 0)
 		return 0;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	nanosec = readl(base + TOD_NANOSEC);
 	seconds_lsb = readl(base + TOD_SECONDSL);
 	seconds_msb = readl(base + TOD_SECONDSH);
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index b651087f426f..cdf2ebe7c9bc 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1229,6 +1229,10 @@ __ptp_ocp_gettime_locked(struct ptp_ocp *bp, struct timespec64 *ts,
 		sts->post_ts = ns_to_timespec64(ns - bp->ts_window_adjust);
 	}
 
+	/*
+	 * Caution: Split access to nanoseconds/seconds, which has a small
+	 * race when the low half overflows while we read it.
+	 */
 	time_ns = ioread32(&bp->reg->time_ns);
 	time_sec = ioread32(&bp->reg->time_sec);
 
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index b8a9a54a176c..a90c206e320b 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -147,6 +147,10 @@ static u64 pch_systime_read(struct pch_ts_regs __iomem *regs)
 {
 	u64 ns;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	ns = ioread64_lo_hi(&regs->systime_lo);
 
 	return ns << TICKS_NS_SHIFT;
@@ -154,6 +158,10 @@ static u64 pch_systime_read(struct pch_ts_regs __iomem *regs)
 
 static void pch_systime_write(struct pch_ts_regs __iomem *regs, u64 ns)
 {
+	/*
+	 * This can equally overflow when the low half of the new
+	 * nanosecond value is close to 0xffffffff.
+	 */
 	iowrite64_lo_hi(ns >> TICKS_NS_SHIFT, &regs->systime_lo);
 }
 
-- 
2.39.5