Re: mlx5 attr.max_sge checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



-------- 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;

Best regards,
Danila




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux