Re: mlx5 attr.max_sge checks

[Patrisious Haddad <phaddad@nvidia.com>]

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@kernel.org>
> To: listdansp <listdansp@mail.ru>
> 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@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?
>
> 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
>>>
>
>
>
>