On 4/3/2024 3:28 PM, listdansp wrote:
External email: Use caution opening links or attachments
-------- Original Message --------
Subject: Re: mlx5 attr.max_sge checks
From: Leon Romanovsky <leon@xxxxxxxxxx>
To: listdansp <listdansp@xxxxxxx>
Date: 17.03.2024
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?
Hi,
I have not any HCA and can’t reproduce this case on practice but in my
estimation, any user space call such as
struct ibv_pd *pd;
struct ibv_srq *srq;
struct ibv_srq_init_attr srq_init_attr;
srq_init_attr.attr.max_wr = 1;
srq_init_attr.attr.max_sge = 0x0FFFFFFF; // any value greater than
0x0FFFFFFE will cause overflow
srq = ibv_create_srq(pd, &srq_init_attr);
Hey Danila,
This won't cause an over-flow since it is protected by
userspace(rdma-core) meaning this max_sge wouldn't even reach the kernel
to cause an over-flow since it would fail inside the rdma-core , the
example you gave for instance would fail inside=> mlx5_create_srq : "if
(attr->attr.max_sge > max_sge)".
And as even pointed out earlier in this mail, most checks are done
inside rdma-core , which guarantees that kernel is safe to operate when
called, and can avoid redundant checks.
So I think this is actually a false positive since it doesn't take
rdma-core checks into account.
Please feel free to share if you have any other examples that you think
can actually cause a problem.
Thanks, Patrisious.
will cause overflow on 64 bits platforms.
Best regards,
Danila
Thanks
Best regards,
Danila
