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

[netdev-bot+sashiko@kernel.org]

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com