Re: [PATCH rdma-next 18/23] RDMA/mlx5: Validate firmware RQ/SQ state before sqrq_trans[] lookup

[Yishai Hadas <yishaih@nvidia.com>]

On 07/11/2026 22:25, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> 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).
> 
> - [Medium] The new check in sqrq_state_to_qp_state()
>    (drivers/infiniband/hw/mlx5/qp.c:5084) bounds the firmware SQ/RQ state…
> 
> --
> 
> RDMA/mlx5: Validate firmware RQ/SQ state before sqrq_trans[] lookup
> 
> This commit adds a bounds check to sqrq_state_to_qp_state() in
> drivers/infiniband/hw/mlx5/qp.c. Any SQ or RQ state returned by QUERY_SQ
> or QUERY_RQ that is at or above the sqrq_trans[] table dimension is now
> rejected with -EIO, which prevents an out-of-bounds read of the 5x5
> table.
> 
>> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
>> index 19cbf6b2b979d..05e8e75d734fb 100644
>> --- a/drivers/infiniband/hw/mlx5/qp.c
>> +++ b/drivers/infiniband/hw/mlx5/qp.c
>> @@ -5081,6 +5081,9 @@ static int sqrq_state_to_qp_state(u8 sq_state, u8 rq_state,
>>   		},
>>   	};
>>   
>> +	if (sq_state >= MLX5_SQ_NUM_STATE || rq_state >= MLX5_RQ_NUM_STATE)
>> +		return -EIO;
>> +
>>   	*qp_state = sqrq_trans[rq_state][sq_state];
> 
> [Severity: Medium]
> Should this check compare against the hardware state encodings instead of
> the table size?
> 
> The hardware only defines three values in include/linux/mlx5/mlx5_ifc.h.
> The RQC values are the same:
> 
>      MLX5_SQC_STATE_RST  = 0x0,
>      MLX5_SQC_STATE_RDY  = 0x1,
>      MLX5_SQC_STATE_ERR  = 0x3,
> 
> The table dimension comes from a driver-only sentinel in
> include/linux/mlx5/qp.h:
> 
>      MLX5_SQ_STATE_NA	= MLX5_SQC_STATE_ERR + 1,
>      MLX5_SQ_NUM_STATE	= MLX5_SQ_STATE_NA + 1,
> 
> That means firmware values 2 and 4 still get past the new check.
> 
> query_raw_packet_qp_rq_state() passes the raw 4-bit field through
> unchanged, and mlx5_core_query_sq_state() does the same for the SQ:
> 
>      *rq_state = MLX5_GET(rqc, rqc, state);
> 
> query_raw_packet_qp_state() sets the NA sentinel by default and only
> overwrites it for queues that exist:
> 
>      u8 sq_state = MLX5_SQ_STATE_NA;
>      u8 rq_state = MLX5_RQ_STATE_NA;
> 
>      if (qp->sq.wqe_cnt) {
>          err = query_raw_packet_qp_sq_state(dev, sq, &sq_state);
>      ...
> 
> Take an SQ-only raw packet QP where firmware returns SQ state 4. Doesn't
> that value alias MLX5_SQ_STATE_NA and land on sqrq_trans[NA][NA]?
> 
>      [MLX5_SQ_STATE_NA]	= MLX5_QP_STATE_BAD,
> 
> That entry reaches WARN(1, "Buggy Raw Packet QP state, ...") through
> ib_query_qp()->mlx5_ib_query_qp()->query_raw_packet_qp_state(). A legal
> SQ value on that QP always maps to MLX5_QP_STATE. The RQ-only case with
> RQ state 4 behaves the same way.
> 
> Value 2 indexes a row or column that is never initialized, so it reads
> as 0, which is IB_QPS_RESET. Would ib_query_qp() then report RESET with
> no error and no WARN? On an SQ-only QP, any legal value would have
> returned qp->state.
> 
> On a panic_on_warn system, a device-triggered WARN here becomes a panic.
> 
> Would it close this gap to accept only the RST, RDY and ERR encodings,
> and only for the queues that were actually queried?
> 
>>   
>>   	if (*qp_state == MLX5_QP_STATE_BAD) {
> 

No. This patch prevents the out of bounds read. The values left inside
the table can only give a wrong QP state or the existing WARN, which the 
NIC can also reach with legal encodings, e.g. an RQ in RST with an SQ in 
RDY.

Yishai