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

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:

quoted

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) <redacted>
---

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!