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

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:24, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 3 · Medium: 3 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The adversarial-mode filter copies cmd_hca_cap.max_tc from
>    firmware with MLX5_HCA_CAP_ALLOW and never clamps or checks it…
> - [High] net/mlx5: unchecked minimum EQ caps cause divide-by-zero in
>    alloc_cpu_rmap()
> - [High] net/mlx5: unclamped log_max_flow_counter_bulk causes OOB heap
>    read
> - [Medium] The commit message says the patch will "trust only a subset of
>    capabilities" and will "Start off by clearing all firmware…
> - [Medium] In adversarial mode, dev->caps.hca[MLX5_CAP_GENERAL]->cur no
>    longer reflects firmware's current configuration: -…
> - [Medium] mlx5_adversarial_filter_general_2() checks
>    log_min_mkey_entity_size only against (max_mkey_log_entity_size_mtt ?:…
> - [Low] The num_ports clamp uses the literal 8 as its upper bound, and
>    the comment says the bound comes from MLX5_MAX_PORTS (`/* 0 is…
> 
> --
> 
> net/mlx5: Add capability filtering in adversarial mode
> 
> This adds mlx5_store_caps(). In adversarial mode it replaces the raw
> memcpy of firmware HCA capabilities with per-field allow-list filtering
> for the general, general_2 and ethernet offload capability types. A few
> numeric capabilities are clamped or range-checked, and a failed check
> aborts driver load. The remaining capability types are either passed
> through whole or zeroed.
> 
>> 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..67b2603397450
>> --- /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)
>> +{
> 
> [ ... ]
> 
>> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, port_type);
>> +	/* 0 is invalid, 8 is for MLX5_MAX_PORTS */
>> +	MLX5_HCA_CAP_CLAMP(cmd_hca_cap, num_ports, 1, 8);
> 
> [Severity: Low]
> This isn't a bug, but the comment says the bound comes from MLX5_MAX_PORTS.
> Could the clamp use MLX5_MAX_PORTS directly instead of the literal 8? The
> two values agree today.
> 
>> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, log_max_msg);
>> +	MLX5_HCA_CAP_ALLOW(cmd_hca_cap, max_tc);
> 
> [Severity: High]
> Should max_tc be clamped or checked here too? It is a 4 bit field, so
> firmware can report up to 15. mlx5_max_tc() turns it into a loop bound:
> 
> drivers/net/ethernet/mellanox/mlx5/core/port.c:mlx5_max_tc() {
> 	u8 num_tc = MLX5_CAP_GEN(mdev, max_tc) ? : 8;
> 
> 	return num_tc - 1;
> }
> 
> That can return up to 14, but the callers assume at most 7 and use it to
> index 8-entry arrays.
> 
> This path runs at netdev init with no user action. It is gated only by
> the qos and ets caps, and both are allow-listed above:
> 

This capability is not used by RDMA.
Currently, netdev doesn't handle adversarial flows.


> [ ... ]
> 
>> +static int mlx5_adversarial_filter_general_2(struct mlx5_core_dev *dev, void *dst,
>> +					     void *src)
>> +{
> 
> [ ... ]
> 
>> +	val = MLX5_GET(cmd_hca_cap_2, dst, max_mkey_log_entity_size_mtt) ?: 31;
>> +	err = MLX5_HCA_CAP_CHECK(cmd_hca_cap_2, log_min_mkey_entity_size, 0,
>> +				 val);
>> +	if (err)
>> +		return err;
>> +
>> +	/* The fixed buffer min is don't-care when its valid bit is clear. */
>> +	if (!MLX5_GET(cmd_hca_cap_2, dst,
>> +		      min_mkey_log_entity_size_fixed_buffer_valid))
>> +		return 0;
> 
> [Severity: Medium]
> log_min_mkey_entity_size is only compared against the MTT max here. Is
> that enough for the code that consumes it?
> 
> In mlx5_ib, get_min_log_entity_size_cap() uses log_min_mkey_entity_size
> as the KSM minimum exactly when the fixed buffer valid bit is clear. The
> KSM maximum comes from max_mkey_log_entity_size_fixed_buffer. This
> combination passes the filter:
> 
>    max_mkey_log_entity_size_mtt = 0
>    log_min_mkey_entity_size = 20
>    max_mkey_log_entity_size_fixed_buffer = 12
>    min_mkey_log_entity_size_fixed_buffer_valid = 0
> 
> mlx5_umem_mkc_find_best_pgsz() then builds GENMASK_ULL(12, 20). That mask
> is empty, so KSM mkey registration fails.
> 
> There is a second case. The filter accepts log_min_mkey_entity_size up to
> 31, and mlx5e_mpwrq_min_page_shift() uses it directly. For a regular RQ,
> mlx5e_mpwrq_page_shift() hits this whenever the value is above PAGE_SHIFT:
> 
> 	if (WARN_ON_ONCE(!xsk && req_page_shift < min_page_shift))
> 
> Should the filter also bound log_min_mkey_entity_size by
> max_mkey_log_entity_size_fixed_buffer and by PAGE_SHIFT?
> 

The PAGE_SHIFT bound will be added in V1. The KSM case only makes the 
registration fail.

> [ ... ]
> 
>> +	switch (cap_type) {
>> +	case MLX5_CAP_GENERAL:
>> +		return mlx5_adversarial_filter_general(dev, dst, src);
>> +	case MLX5_CAP_GENERAL_2:
>> +		return mlx5_adversarial_filter_general_2(dev, dst, src);
>> +	case MLX5_CAP_ETHERNET_OFFLOADS:
>> +		return mlx5_adversarial_filter_per_protocol(dev, dst, src);
>> +
>> +	case MLX5_CAP_ATOMIC:
>> +	case MLX5_CAP_ROCE:
>> +	case MLX5_CAP_FLOW_TABLE:
>> +	case MLX5_CAP_DEV_EVENT:
>> +		memcpy(dst, src, MLX5_UN_SZ_BYTES(hca_cap_union));
>> +		break;
> 
> [Severity: Medium]
> The commit message says:
> 
>    When in adversarial mode, trust only a subset of capabilities
>    and do some additional validations.
> 
>    Start off by clearing all firmware capabilities and enabling
>    them one by one.
> 
> Is the whole-structure pass-through for MLX5_CAP_ATOMIC, MLX5_CAP_ROCE,
> MLX5_CAP_FLOW_TABLE and MLX5_CAP_DEV_EVENT intended? MLX5_CAP_FLOW_TABLE
> in particular carries numeric limits that the steering code relies on.
> 
> There is a related case. mlx5_adversarial_filter_general() allow-lists
> sbcam_reg, qcam_reg, mcam_reg and pcam_reg. In mlx5_query_hca_caps() those
> bits gate mlx5_get_pcam_reg(), mlx5_get_mcam_access_reg_group() and
> mlx5_get_qcam_reg(). These store firmware data straight into
> dev->caps.pcam, dev->caps.mcam and dev->caps.qcam without going through
> mlx5_store_caps().
> 
> If this is deliberate, could the commit message say which capability
> areas are still trusted wholesale?

Yes, it is deliberate. This series filters the caps that the RDMA paths 
use as sizes and limits.

> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> index f9e10464a71b5..e76a4486b5a31 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> @@ -325,12 +325,12 @@ int mlx5_core_get_caps_mode(struct mlx5_core_dev *dev, enum mlx5_cap_type cap_ty
>>   
>>   	switch (cap_mode) {
>>   	case HCA_CAP_OPMOD_GET_MAX:
>> -		memcpy(dev->caps.hca[cap_type]->max, hca_caps,
>> -		       MLX5_UN_SZ_BYTES(hca_cap_union));
>> +		err = mlx5_store_caps(dev, cap_type,
>> +				      dev->caps.hca[cap_type]->max, hca_caps);
>>   		break;
>>   	case HCA_CAP_OPMOD_GET_CUR:
>> -		memcpy(dev->caps.hca[cap_type]->cur, hca_caps,
>> -		       MLX5_UN_SZ_BYTES(hca_cap_union));
>> +		err = mlx5_store_caps(dev, cap_type,
>> +				      dev->caps.hca[cap_type]->cur, hca_caps);
>>   		break;
> 
> [Severity: Medium]
> In adversarial mode, dev->caps.hca[MLX5_CAP_GENERAL]->cur now holds the
> filtered image:
> 
> - it is zeroed, and only allow-listed fields are copied back
> - num_of_uars_per_page is forced
> - num_ports, num_lag_ports, num_vhca_ports and log_max_rqt_size are
>    clamped
> 
> handle_hca_cap() still uses ->cur as the template for SET_HCA_CAP:
> 
> drivers/net/ethernet/mellanox/mlx5/core/main.c:handle_hca_cap() {
>      ...
> 	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> 				   capability);
> 	memcpy(set_hca_cap, dev->caps.hca[MLX5_CAP_GENERAL]->cur,
> 	       MLX5_ST_SZ_BYTES(cmd_hca_cap));
>      ...
> }
> 
> It then writes the whole structure back with set_caps().
> 
> Does this program every firmware-writable field to 0 if it is not on the
> allow-list and handle_hca_cap() does not set it explicitly? Are the forced
> and clamped values also written back to the device?
> 
> If so, adversarial mode changes the device configuration, not only what
> the driver trusts, and the commit message doesn't mention it. Is that
> intended, or should handle_hca_cap() start from the unfiltered firmware
> values?

Right, in adversarial mode SET_HCA_CAP starts from the filtered values.
The default mode is not affected.

> [Severity: High]
> Should log_max_flow_counter_bulk be clamped like log_max_rqt_size?
> It is an 8 bit field, so firmware can report up to 255.

This capability is not used by RDMA. Currently, netdev doesn't handle 
adversarial flows.

> [Severity: High]
> Should the EQ count be range checked as well? log_max_eq and
> max_num_eqs are allow-listed here, and max_num_eqs_24b is allow-listed
> in mlx5_adversarial_filter_general_2(). Nothing makes sure the device
> reports more EQs than the driver reserves for async EQs.
> 
> mlx5_max_eq_cap_get() returns max_num_eqs_24b if it is set, otherwise
> max_num_eqs, otherwise 1 << log_max_eq. get_num_eqs() then takes the
> minimum of two values:
> 
> - the IRQ table's completion vector count
> - max_dev_eqs - MLX5_MAX_ASYNC_EQS
> 
> MLX5_MAX_ASYNC_EQS is 3 or 4, depending on
> CONFIG_INFINIBAND_ON_DEMAND_PAGING.
> 
> Suppose firmware reports exactly MLX5_MAX_ASYNC_EQS, for example
> max_num_eqs = 4 with ODP enabled. Then eq_table->max_comp_eqs becomes
> 0, and probe continues into:
> 
> mlx5_load()
>    mlx5_eq_table_create()
>      alloc_rmap()
>        alloc_irq_cpu_rmap(eq_table->max_comp_eqs)
>          alloc_cpu_rmap(0, GFP_KERNEL)
> 
> alloc_cpu_rmap() fills the near[] table with:
> 
> 	rmap->near[cpu].index = cpu % size;
> 
> Doesn't that divide by zero during driver load? This path is taken
> with CONFIG_RFS_ACCEL on non-SF devices and needs no user action.
> 
> If firmware reports fewer EQs than MLX5_MAX_ASYNC_EQS, max_comp_eqs
> goes negative instead. alloc_cpu_rmap() rejects that value as too
> large. But when the rmap is not allocated, what do
> mlx5_comp_vectors_max() and the completion EQ code make of a negative
> count?
> 
> With trusted firmware this predates the patch. The three fields come
> from two different cap types, so one MLX5_HCA_CAP_CHECK() in a single
> filter function can't cover them all. Would it be simpler to reject
> max_dev_eqs <= MLX5_MAX_ASYNC_EQS in get_num_eqs() or
> mlx5_eq_table_create()? Or is there a good place in the filter for
> this check?
> 

OK, it will be fixed as part of V1.

Yishai