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@xxxxxxxxxx> > To: listdansp <listdansp@xxxxxxx> > 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 >
