Re: [PATCH V1 rdma-next 03/24] net/mlx5: Add capability filtering in adversarial mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] net/mlx5: unclamped log_max_flow_counter_bulk causes OOB heap
  read
- [High] net/mlx5: unclamped max_tc in mlx5_adversarial_filter_general()
  overflows stack
- [Medium] net/mlx5: unclamped log_max_*_list caps in
  mlx5_adversarial_filter_general()
--

commit 3a4fed029c43a049522bf33112212c850561f40e
Author: Dragos Tatulea <dtatulea@nvidia.com>

net/mlx5: Add capability filtering in adversarial mode

This commit introduces capability filtering and range validation for HCA
capabilities queried from firmware when running in adversarial mode.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/caps_filter.c b/drivers/net/ethernet/mellanox/mlx5/core/caps_filter.c
> new file mode 100644
> index 0000000000000..023f996bc95ea
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/caps_filter.c
> @@ -0,0 +1,381 @@
[ ... ]
> +static int mlx5_adversarial_filter_general(struct mlx5_core_dev *dev, void *dst,
> +					   void *src)
> +{
[ ... ]
> +	/* 0 is invalid, 8 is for MLX5_MAX_PORTS */
> +	MLX5_HCA_CAP_CLAMP(cmd_hca_cap, num_ports, 1, 8);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_msg);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, max_tc);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, dcbx);

[Severity: High]
Should max_tc be clamped to IEEE_8021QAZ_MAX_TCS (8)?

max_tc is a 4-bit field, so untrusted firmware can report up to 15 and make
mlx5_max_tc() return up to 14.  When qos and ets are also enabled,
mlx5e_ets_init() and mlx5e_dcbnl_ieee_setets_core() run during mlx5e probe
and loop up to mlx5_max_tc() + 1, overflowing the 8-entry arrays in struct
ieee_ets, tc_group[], tc_tx_bw[], and qetc_reg on the stack:

drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c:mlx5e_ets_init() {
	ets.ets_cap = mlx5_max_tc(priv->mdev) + 1;
	for (i = 0; i < ets.ets_cap; i++) {
		ets.tc_tx_bw[i] = MLX5E_MAX_BW_ALLOC;
		ets.tc_tsa[i] = IEEE_8021QAZ_TSA_VENDOR;
		ets.prio_tc[i] = i;
	}
...
}

[ ... ]

> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, eq_overrun_count);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, invalid_command_count);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, quota_exceeded_count);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_flow_counter_bulk);

[Severity: High]
Can an unclamped log_max_flow_counter_bulk cause an out-of-bounds heap read
past fc_stats->bulk_query_out?

log_max_flow_counter_bulk is an 8-bit field.  During mlx5_init_fc_stats(),
get_init_bulk_query_len() shifts 1 by log_max_flow_counter_bulk as a signed
int:

drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c:get_init_bulk_query_len() {
	return min_t(int, MLX5_INIT_COUNTERS_BULK,
		     (1 << MLX5_CAP_GEN(dev, log_max_flow_counter_bulk)));
}

A value >= 32 shifts out of bounds, and 31 overflows to INT_MIN
(-2147483648).  mlx5_cmd_fc_get_bulk_query_out_len() then multiplies
bulk_len by MLX5_ST_SZ_BYTES(traffic_counter) (16UL) and truncates the
result to int, returning 16 bytes for fc_stats->bulk_query_out while
fc_stats->bulk_query_len is set to -2147483648.  When flow counters are
queried in mlx5_fc_stats_query_all_counters(), update_counter_cache()
indexes flow_statistics[] past the 16-byte buffer.

> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_rq);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_sq);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_tir);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, basic_cyclic_rcv_wqe);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_rqt);
> +	MLX5_HCA_CAP_CLAMP(cmd_hca_cap, log_max_rqt_size, 0, 13);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, vnic_env_cnt_bar_uar_access);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, vnic_env_cnt_odp_page_fault);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, roce_rw_supported);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_wq_sz);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, vhca_state);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_vlan_list);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_current_mc_list);
> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_current_uc_list);

[Severity: Medium]
Can log_max_vlan_list, log_max_current_mc_list, or log_max_current_uc_list
overflow the signed shift in mlx5e_vport_context_update_vlans() and
mlx5e_vport_context_update_addr_list()?

All three are 5-bit fields.  When any of them is 31, the 1 << 31 computation
in mlx5e_vport_context_update_vlans() or
mlx5e_vport_context_update_addr_list() overflows max_list_size or max_size
to INT_MIN:

drivers/net/ethernet/mellanox/mlx5/core/en_fs.c:mlx5e_vport_context_update_vlans() {
...
	max_list_size = 1 << MLX5_CAP_GEN(fs->mdev, log_max_vlan_list);

	if (list_size > max_list_size) {
		fs_warn(fs, "netdev vlans list size (%d) > (%d) max vport list size, some vlans will be dropped\n",
			list_size, max_list_size);
		list_size = max_list_size;
	}

	vlans = kvcalloc(list_size, sizeof(*vlans), GFP_KERNEL);
...
}

list_size (or size) is then clamped to INT_MIN and passed as a huge size_t
to kvcalloc() or kcalloc() without __GFP_NOWARN, triggering an allocator
WARN_ON_ONCE and failing VLAN and MAC filter updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110134320.34408-1-yishaih@nvidia.com?part=3