Re: [PATCH net v2] bridge: cfm: reject invalid CCM interval at configuration time

[Ido Schimmel <idosch@nvidia.com>]

On Sat, Apr 04, 2026 at 05:03:24PM -0700, Xiang Mei wrote:
> ccm_tx_work_expired() re-arms itself via queue_delayed_work() using
> the configured exp_interval converted by interval_to_us(). When
> exp_interval is BR_CFM_CCM_INTERVAL_NONE or out of range,
> interval_to_us() returns 0, causing the worker to fire immediately in
> a tight loop that allocates skbs until OOM.
> 
> Fix this by validating exp_interval at configuration time:
> 
>  - Constrain IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL to [1, 7] in the
>    netlink policy so userspace cannot set an invalid value.
> 
>  - Reject starting CCM TX in br_cfm_cc_ccm_tx() when exp_interval has
>    not yet been configured (defaults to 0 from kzalloc).
> 
> Fixes: a806ad8ee2aa ("bridge: cfm: Kernel space implementation of CFM. CCM frame TX added.")

Nit: Doesn't matter in practice, but let's blame commit 2be665c3940d
("bridge: cfm: Netlink SET configuration Interface.") instead as I don't
think this bug could be triggered before exposing the netlink API.

> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
> v2: Move validation out of the datapath and into configuration
> 
>  net/bridge/br_cfm.c         | 6 ++++++
>  net/bridge/br_cfm_netlink.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_cfm.c b/net/bridge/br_cfm.c
> index 118c7ea48c35..dea56fffa1c1 100644
> --- a/net/bridge/br_cfm.c
> +++ b/net/bridge/br_cfm.c
> @@ -805,6 +805,12 @@ int br_cfm_cc_ccm_tx(struct net_bridge *br, const u32 instance,
>  		goto save;
>  	}
>  
> +	if (!interval_to_us(mep->cc_config.exp_interval)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Invalid CCM interval");
> +		return -EINVAL;
> +	}
> +
>  	/* Start delayed work to transmit CCM frames. It is done with zero delay
>  	 * to send first frame immediately
>  	 */
> diff --git a/net/bridge/br_cfm_netlink.c b/net/bridge/br_cfm_netlink.c
> index 2faab44652e7..1bb33c8f587b 100644
> --- a/net/bridge/br_cfm_netlink.c
> +++ b/net/bridge/br_cfm_netlink.c
> @@ -34,7 +34,7 @@ br_cfm_cc_config_policy[IFLA_BRIDGE_CFM_CC_CONFIG_MAX + 1] = {
>  	[IFLA_BRIDGE_CFM_CC_CONFIG_UNSPEC]	 = { .type = NLA_REJECT },
>  	[IFLA_BRIDGE_CFM_CC_CONFIG_INSTANCE]	 = { .type = NLA_U32 },
>  	[IFLA_BRIDGE_CFM_CC_CONFIG_ENABLE]	 = { .type = NLA_U32 },
> -	[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] = { .type = NLA_U32 },
> +	[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_INTERVAL] = NLA_POLICY_RANGE(NLA_U32, 1, 7),

Use BR_CFM_CCM_INTERVAL_3_3_MS and BR_CFM_CCM_INTERVAL_10_MIN instead of
the magic numbers?

The Sashiko review points out that blocking BR_CFM_CCM_INTERVAL_NONE
might break user space, but it seems weird to allow passing a value that
is interpreted the same as an invalid one. Worst case, if someone
complains, we can revert and go back to v1.

>  	[IFLA_BRIDGE_CFM_CC_CONFIG_EXP_MAID]	 = {
>  	.type = NLA_BINARY, .len = CFM_MAID_LENGTH },
>  };
> -- 
> 2.43.0
>