Re: mlx5 attr.max_sge checks

[Leon Romanovsky <leon@kernel.org>]

On Thu, Mar 14, 2024 at 11:29:49PM +0300, listdansp wrote:
> -------- 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;

Let me repeat my question.
Can you please provide an example of such values?

Thanks

> 
> Best regards,
> Danila
>