[PATCH net-next] net: stmmac: ptp: limit n_per_out

[PATCH net-next] net: stmmac: ptp: limit n_per_out

[Russell King (Oracle)]

ptp_clock_ops.n_per_out sets the number of PPS outputs, which the PTP
subsystem uses to validate userspace input, such as the index number
used in a PTP_CLK_REQ_PEROUT request.

stmmac_enable() uses this to index the priv->pps array, which is an
array of size STMMAC_PPS_MAX. ptp_clock_ops.n_per_out is initialised
using priv->dma_cap.pps_out_num, which is a three bit field read from
hardware.

Documentation that I've checked suggests that values >= 5 are reserved,
but that doesn't mean such values won't appear, and if they do, we
can overrun the priv->pps array in stmmac_enable().

stmmac_ptp_register() has protection against this in its loop, but it
doesn't act to limit ptp_clock_ops.n_per_out.

Fix this by introducing a local variable, pps_out_num which is limited
to STMMAC_PPS_MAX, and use that when initialising the array and setting
priv->ptp_clock_ops.n_per_out.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---

This could be a user exploitable bug (although one has to be root
so the gun is already pointing at one's foot.) This is the commit
which introduced the problem:

Fixes: 9a8a02c9d46d ("net: stmmac: Add Flexible PPS support")

 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 3e30172fa129..cf5506bf2198 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -334,13 +334,14 @@ const struct ptp_clock_info dwmac1000_ptp_clock_ops = {
  */
 void stmmac_ptp_register(struct stmmac_priv *priv)
 {
+	unsigned int pps_out_num = priv->dma_cap.pps_out_num;
 	int i;
 
-	for (i = 0; i < priv->dma_cap.pps_out_num; i++) {
-		if (i >= STMMAC_PPS_MAX)
-			break;
+	if (pps_out_num > STMMAC_PPS_MAX)
+		pps_out_num = STMMAC_PPS_MAX;
+
+	for (i = 0; i < pps_out_num; i++)
 		priv->pps[i].available = true;
-	}
 
 	/* Calculate the clock domain crossing (CDC) error if necessary */
 	priv->plat->cdc_error_adj = 0;
@@ -350,8 +351,8 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 	/* Update the ptp clock parameters based on feature discovery, when
 	 * available
 	 */
-	if (priv->dma_cap.pps_out_num)
-		priv->ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
+	if (pps_out_num)
+		priv->ptp_clock_ops.n_per_out = pps_out_num;
 
 	if (priv->dma_cap.aux_snapshot_n)
 		priv->ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n;
-- 
2.47.3

Re: [PATCH net-next] net: stmmac: ptp: limit n_per_out

[Simon Horman]

On Mon, Feb 23, 2026 at 12:20:47PM +0000, Russell King (Oracle) wrote:
> ptp_clock_ops.n_per_out sets the number of PPS outputs, which the PTP
> subsystem uses to validate userspace input, such as the index number
> used in a PTP_CLK_REQ_PEROUT request.
> 
> stmmac_enable() uses this to index the priv->pps array, which is an
> array of size STMMAC_PPS_MAX. ptp_clock_ops.n_per_out is initialised
> using priv->dma_cap.pps_out_num, which is a three bit field read from
> hardware.
> 
> Documentation that I've checked suggests that values >= 5 are reserved,
> but that doesn't mean such values won't appear, and if they do, we
> can overrun the priv->pps array in stmmac_enable().
> 
> stmmac_ptp_register() has protection against this in its loop, but it
> doesn't act to limit ptp_clock_ops.n_per_out.
> 
> Fix this by introducing a local variable, pps_out_num which is limited
> to STMMAC_PPS_MAX, and use that when initialising the array and setting
> priv->ptp_clock_ops.n_per_out.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> 
> This could be a user exploitable bug (although one has to be root
> so the gun is already pointing at one's foot.) This is the commit
> which introduced the problem:

Hi Russell,

From the description I assumed that for this problem to manifest
out-of-range values would need to be turned by hardware.
But maybe I misunderstand things.

Could you elaborate on the vector you have in mind?

> 
> Fixes: 9a8a02c9d46d ("net: stmmac: Add Flexible PPS support")

...

Re: [PATCH net-next] net: stmmac: ptp: limit n_per_out

[Russell King (Oracle)]

On Tue, Feb 24, 2026 at 09:26:29AM +0000, Simon Horman wrote:
> On Mon, Feb 23, 2026 at 12:20:47PM +0000, Russell King (Oracle) wrote:
> > ptp_clock_ops.n_per_out sets the number of PPS outputs, which the PTP
> > subsystem uses to validate userspace input, such as the index number
> > used in a PTP_CLK_REQ_PEROUT request.
> > 
> > stmmac_enable() uses this to index the priv->pps array, which is an
> > array of size STMMAC_PPS_MAX. ptp_clock_ops.n_per_out is initialised
> > using priv->dma_cap.pps_out_num, which is a three bit field read from
> > hardware.
> > 
> > Documentation that I've checked suggests that values >= 5 are reserved,
> > but that doesn't mean such values won't appear, and if they do, we
> > can overrun the priv->pps array in stmmac_enable().
> > 
> > stmmac_ptp_register() has protection against this in its loop, but it
> > doesn't act to limit ptp_clock_ops.n_per_out.
> > 
> > Fix this by introducing a local variable, pps_out_num which is limited
> > to STMMAC_PPS_MAX, and use that when initialising the array and setting
> > priv->ptp_clock_ops.n_per_out.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > 
> > This could be a user exploitable bug (although one has to be root
> > so the gun is already pointing at one's foot.) This is the commit
> > which introduced the problem:
> 
> Hi Russell,
> 
> From the description I assumed that for this problem to manifest
> out-of-range values would need to be turned by hardware.
> But maybe I misunderstand things.
> 
> Could you elaborate on the vector you have in mind?

priv->dma_cap.pps_out_num is initialised from hardware:

dwmac4.h:#define GMAC_HW_FEAT_PPSOUTNUM         GENMASK(26, 24)
dwmac4_dma.c:   dma_cap->pps_out_num = (hw_cap & GMAC_HW_FEAT_PPSOUTNUM) >> 24;
dwxgmac2.h:#define XGMAC_HWFEAT_PPSOUTNUM               GENMASK(26, 24)
dwxgmac2_dma.c: dma_cap->pps_out_num = (hw_cap & XGMAC_HWFEAT_PPSOUTNUM) >> 24;

As can be seen, these are three bit fields, and as noted in my commit
description, values in this field above 4 appear to be reserved, but
"reserved" doesn't mean they will never be seen.

Meanwhile, priv->pps[] is defined as:

#define STMMAC_PPS_MAX          4
        struct stmmac_pps_cfg pps[STMMAC_PPS_MAX];

The code in stmmac_ptp_register() takes account of that, and is careful
not to overrun the priv->pps[] array:

	for (i = 0; i < priv->dma_cap.pps_out_num; i++) {
		if (i >= STMMAC_PPS_MAX)
			break;
		priv->pps[i].available = true;
	}

but the code there goes on to assign it to the number of per_out:

	if (priv->dma_cap.pps_out_num)
		priv->ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;

Core PTP code uses this to validate user input. This limits the value
that can appear in rq->perout.index for a PTP clock ->enable() call
for PTP_CLK_REQ_PEROUT.

stmmac_enable() implements this method, and with no bounds checks,
does this:

                cfg = &priv->pps[rq->perout.index];

                cfg->start.tv_sec = rq->perout.start.sec;
                cfg->start.tv_nsec = rq->perout.start.nsec;

Thus, if priv->dma_cap.pps_out_num were to indicate e.g. 7, then
there is nothing to prevent rq->perout.index being e.g. 6, and thus
overflowing the priv->pps[] array. This will likely write to other
struct stmmac_priv members if it were to occur.

So... there's two views one can take here:

- hardware will never indicate values > 4. If that's the case, then
  what's the point of the limiting check in stmmac_ptp_register() ?

- hardware might one day support more than 4 outputs, resulting in
  priv->dma_cap.pps_out_num being greater than 4. This will be a
  silent overrun until someone attempts to configure an output > 4,
  at which point non-PPS data of struct stmmac_priv will be
  overwritten.

Either code should care about values > 4, or it shouldn't. The current
code cares about it in one place but then ignores it in all other
places where the index is under userspace control, allowing the
potential for array overrun.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: stmmac: ptp: limit n_per_out

[Simon Horman]

On Tue, Feb 24, 2026 at 10:02:02AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 24, 2026 at 09:26:29AM +0000, Simon Horman wrote:
> > On Mon, Feb 23, 2026 at 12:20:47PM +0000, Russell King (Oracle) wrote:

...

> > > This could be a user exploitable bug (although one has to be root
> > > so the gun is already pointing at one's foot.) This is the commit
> > > which introduced the problem:
> > 
> > Hi Russell,
> > 
> > From the description I assumed that for this problem to manifest
> > out-of-range values would need to be turned by hardware.
> > But maybe I misunderstand things.
> > 
> > Could you elaborate on the vector you have in mind?

...

> Either code should care about values > 4, or it shouldn't. The current
> code cares about it in one place but then ignores it in all other
> places where the index is under userspace control, allowing the
> potential for array overrun.

Hi Russell,

Thanks for the clarification.

Personally I think it would be best if the Kernel took a robust approach
and assumed that hw may provide out of range values. But in my experience
this is generally not the approach taken by drivers. And it's not a hill I
which to spend too much time occupying.

IOW, I don't think the current practice is to treat such cases as bugs.

On the other hand, I agree that the code should be consistent.
And I would lean towards verifying rather than not, although again,
I don't think that one can find plenty of cases where the Kernel
doesn't do that.

Which is to say that I agree with the approach taken by your patch.
But I lean towards it not being a fix.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next] net: stmmac: ptp: limit n_per_out

[Jakub Kicinski]

On Mon, 23 Feb 2026 12:20:47 +0000 Russell King (Oracle) wrote:
> +	if (pps_out_num > STMMAC_PPS_MAX)
> +		pps_out_num = STMMAC_PPS_MAX;

How would you feel about tossing a dev_info() / dev_warn() in here?
If nothing else if will prevent script kiddies from sending a patch
converting this if to

	pps_out_num = min(pps_out_num, STMMAC_PPS_MAX);

which presumably you don't prefer.