Re: mlx5 attr.max_sge checks

[listdansp <listdansp@mail.ru>]

-------- Original Message  --------
Subject: Re: mlx5 attr.max_sge checks
From: Leon Romanovsky <leon@kernel.org>
To: listdansp <listdansp@mail.ru>
Date: 20.12.2023

> On Tue, Dec 19, 2023 at 09:56:01PM +0300, listdansp wrote:
>> Hi,
>>
>> While investigating the one report of the static analyzer (svacer), it was
>> discovered that attr.max_sge was not checked for the maximum value in the
>> mlx5_ib_create_srq function. However, this check is present in
>> https://github.com/linux-rdma/rdma-core. Also, checks are present in most
>> other infiniband Linux Kernel drivers. This may lead to incorrect driver
>> operation for example
>> int mlx5_ib_read_wqe_srq(struct mlx5_ib_srq *srq, int wqe_index, void
>> *buffer, size_tbuflen, size_t*bc)
>> {
>> structib_umem*umem= srq->umem;
>> size_twqe_size= 1 << srq->msrq.wqe_shift; // integeroverflowhere
>> if(buflen< wqe_size)
>> return-EINVAL;
>> In my opinion, the only possible solution to this problem may be to add a
>> check to mlx5_ib_create_srq similar to
>> https://github.com/linux-rdma/rdma-core
>> <https://github.com/linux-rdma/rdma-core> like
>> u32 max_sge= MLX5_CAP_GEN(dev->mdev, max_wqe_sz_rq) /
>> sizeof(structmlx5_wqe_data_seg);
>> if (attr->attr.max_sge > max_sge) {
>> mlx5_ib_dbg
>> <https://elixir.bootlin.com/linux/v5.10.169/C/ident/mlx5_ib_dbg>(dev,
>> "max_sge%d, cap %d\n", init_attr
>> <https://elixir.bootlin.com/linux/v5.10.169/C/ident/init_attr>->attr.max_
>> <https://elixir.bootlin.com/linux/v5.10.169/C/ident/max_wr>sge, max_sge);
>> return -EINVAL <https://elixir.bootlin.com/linux/v5.10.169/C/ident/EINVAL>;
>> }
>>
>> I would appreciate your suggestions and comments.
> 
> Can you please provide an example of such values?
> 
> At least in the presented case, the values are supplied by FW and are
> supposed to be right without any overflows.
> 
> Thanks
> 
>>
>> Best regards,
>> Danila
>>
>>

Hi,

In the mlx5_ib_create_srq function, the variable srq->msrq.wqe_shift = 
ilog2(desc_size).
Value of  desc_size is result of desc_size = sizeof(struct 
mlx5_wqe_srq_next_seg) + srq->msrq.max_gs * sizeof(struct 
mlx5_wqe_data_seg);.
The init_attr->attr.max_sge parameter can be set to any 4-byte unsigned 
number.
There is overflow checking
if (desc_size == 0 || srq->msrq.max_gs > desc_size)
return -EINVAL;
but it works correctly only for 32-bit platforms because size_t 
desc_size; and for 64 bits platforms sizeof(size_t) is 8.
So, result of srq->msrq.wqe_shift = ilog2(desc_size) may be greater than 
31 and will cause overflow in size_t wqe_size = 1 << srq->msrq.wqe_shift;

Best regards,
Danila