* Re: mlx5 attr.max_sge checks [not found] <c78ab477-5b54-82b5-1d5f-8b0022195f78@mail.ru> @ 2023-12-20 8:07 ` Leon Romanovsky 2024-03-14 20:29 ` listdansp 0 siblings, 1 reply; 6+ messages in thread From: Leon Romanovsky @ 2023-12-20 8:07 UTC (permalink / raw) To: listdansp; +Cc: dledford, jgg, linux-rdma 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mlx5 attr.max_sge checks 2023-12-20 8:07 ` mlx5 attr.max_sge checks Leon Romanovsky @ 2024-03-14 20:29 ` listdansp 2024-03-17 8:35 ` Leon Romanovsky 0 siblings, 1 reply; 6+ messages in thread From: listdansp @ 2024-03-14 20:29 UTC (permalink / raw) To: Leon Romanovsky; +Cc: dledford, jgg, linux-rdma -------- 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; Best regards, Danila ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mlx5 attr.max_sge checks 2024-03-14 20:29 ` listdansp @ 2024-03-17 8:35 ` Leon Romanovsky 2024-04-03 12:28 ` listdansp 0 siblings, 1 reply; 6+ messages in thread From: Leon Romanovsky @ 2024-03-17 8:35 UTC (permalink / raw) To: listdansp; +Cc: dledford, jgg, linux-rdma 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mlx5 attr.max_sge checks 2024-03-17 8:35 ` Leon Romanovsky @ 2024-04-03 12:28 ` listdansp 2024-05-05 8:58 ` Patrisious Haddad 0 siblings, 1 reply; 6+ messages in thread From: listdansp @ 2024-04-03 12:28 UTC (permalink / raw) To: Leon Romanovsky; +Cc: dledford, jgg, linux-rdma -------- 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); will cause overflow on 64 bits platforms. Best regards, Danila > > Thanks > >> >> Best regards, >> Danila >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mlx5 attr.max_sge checks 2024-04-03 12:28 ` listdansp @ 2024-05-05 8:58 ` Patrisious Haddad 0 siblings, 0 replies; 6+ messages in thread From: Patrisious Haddad @ 2024-05-05 8:58 UTC (permalink / raw) To: listdansp, Leon Romanovsky; +Cc: dledford, jgg, linux-rdma, msanalla 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 >>> > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1703013183-24379-mlmmj-3a1ea6ac@vger.kernel.org>]
* mlx5 attr.max_sge checks [not found] <1703013183-24379-mlmmj-3a1ea6ac@vger.kernel.org> @ 2023-12-19 19:39 ` listdansp 0 siblings, 0 replies; 6+ messages in thread From: listdansp @ 2023-12-19 19:39 UTC (permalink / raw) To: leon, dledford, jgg; +Cc: linux-rdma 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_t buflen, size_t *bc) { struct ib_umem *umem = srq->umem; size_t wqe_size = 1 << srq->msrq.wqe_shift; // integer overflow here 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 like u32 max_sge = MLX5_CAP_GEN(dev->mdev, max_wqe_sz_rq) / sizeof(struct mlx5_wqe_data_seg); if (attr->attr.max_sge > max_sge) { mlx5_ib_dbg(dev, "max_sge %d, cap %d\n", init_attr->attr.max_sge, max_sge); return -EINVAL; } I would appreciate your suggestions and comments. Best regards, Danila ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-05 8:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <c78ab477-5b54-82b5-1d5f-8b0022195f78@mail.ru> 2023-12-20 8:07 ` mlx5 attr.max_sge checks Leon Romanovsky 2024-03-14 20:29 ` listdansp 2024-03-17 8:35 ` Leon Romanovsky 2024-04-03 12:28 ` listdansp 2024-05-05 8:58 ` Patrisious Haddad [not found] <1703013183-24379-mlmmj-3a1ea6ac@vger.kernel.org> 2023-12-19 19:39 ` listdansp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).