-----"Jason Gunthorpe" <jgg@xxxxxxxxxx> wrote: -----
>To: "Kamal Heib" <kamalheib1@xxxxxxxxx>
>From: "Jason Gunthorpe" <jgg@xxxxxxxxxx>
>Date: 12/07/2020 09:29PM
>Cc: <linux-rdma@xxxxxxxxxxxxxxx>, "Bernard Metzler"
><bmt@xxxxxxxxxxxxxx>, "Doug Ledford" <dledford@xxxxxxxxxx>
>Subject: [EXTERNAL] Re: [PATCH for-rc] RDMA/siw: Fix
>shift-out-of-bounds when call roundup_pow_of_two()
>
>On Mon, Dec 07, 2020 at 11:37:28AM +0200, Kamal Heib wrote:
>> When running the blktests over siw the following
>shift-out-of-bounds is
>> reported, this is happening because the passed IRD or ORD from the
>ulp
>> could be zero which will lead to unexpected behavior when calling
>> roundup_pow_of_two(), fix that by blocking zero values of ORD or
>IRD.
>>
>> UBSAN: shift-out-of-bounds in ./include/linux/log2.h:57:13
>> shift exponent 64 is too large for 64-bit type 'long unsigned int'
>> CPU: 20 PID: 3957 Comm: kworker/u64:13 Tainted: G S 5.10.0-rc6
>#2
>> Hardware name: Dell Inc. PowerEdge R630/02C2CP, BIOS 2.1.5
>04/11/2016
>> Workqueue: iw_cm_wq cm_work_handler [iw_cm]
>> Call Trace:
>> dump_stack+0x99/0xcb
>> ubsan_epilogue+0x5/0x40
>> __ubsan_handle_shift_out_of_bounds.cold.11+0xb4/0xf3
>> ? down_write+0x183/0x3d0
>> siw_qp_modify.cold.8+0x2d/0x32 [siw]
>> ? __local_bh_enable_ip+0xa5/0xf0
>> siw_accept+0x906/0x1b60 [siw]
>> ? xa_load+0x147/0x1f0
>> ? siw_connect+0x17a0/0x17a0 [siw]
>> ? lock_downgrade+0x700/0x700
>> ? siw_get_base_qp+0x1c2/0x340 [siw]
>> ? _raw_spin_unlock_irqrestore+0x39/0x40
>> iw_cm_accept+0x1f4/0x430 [iw_cm]
>> rdma_accept+0x3fa/0xb10 [rdma_cm]
>> ? check_flush_dependency+0x410/0x410
>> ? cma_rep_recv+0x570/0x570 [rdma_cm]
>> nvmet_rdma_queue_connect+0x1a62/0x2680 [nvmet_rdma]
>> ? nvmet_rdma_alloc_cmds+0xce0/0xce0 [nvmet_rdma]
>> ? lock_release+0x56e/0xcc0
>> ? lock_downgrade+0x700/0x700
>> ? lock_downgrade+0x700/0x700
>> ? __xa_alloc_cyclic+0xef/0x350
>> ? __xa_alloc+0x2d0/0x2d0
>> ? rdma_restrack_add+0xbe/0x2c0 [ib_core]
>> ? __ww_mutex_die+0x190/0x190
>> cma_cm_event_handler+0xf2/0x500 [rdma_cm]
>> iw_conn_req_handler+0x910/0xcb0 [rdma_cm]
>> ? _raw_spin_unlock_irqrestore+0x39/0x40
>> ? trace_hardirqs_on+0x1c/0x150
>> ? cma_ib_handler+0x8a0/0x8a0 [rdma_cm]
>> ? __kasan_kmalloc.constprop.7+0xc1/0xd0
>> cm_work_handler+0x121c/0x17a0 [iw_cm]
>> ? iw_cm_reject+0x190/0x190 [iw_cm]
>> ? trace_hardirqs_on+0x1c/0x150
>> process_one_work+0x8fb/0x16c0
>> ? pwq_dec_nr_in_flight+0x320/0x320
>> worker_thread+0x87/0xb40
>> ? __kthread_parkme+0xd1/0x1a0
>> ? process_one_work+0x16c0/0x16c0
>> kthread+0x35f/0x430
>> ? kthread_mod_delayed_work+0x180/0x180
>> ret_from_fork+0x22/0x30
>>
>> Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
>> Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
>> drivers/infiniband/sw/siw/siw_cm.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>> index 66764f7ef072..dff0b00cc55d 100644
>> +++ b/drivers/infiniband/sw/siw/siw_cm.c
>> @@ -1571,7 +1571,8 @@ int siw_accept(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
>> qp->tx_ctx.gso_seg_limit = 0;
>> }
>> if (params->ord > sdev->attrs.max_ord ||
>> - params->ird > sdev->attrs.max_ird) {
>> + params->ird > sdev->attrs.max_ird ||
>> + !params->ord || !params->ird) {
>> siw_dbg_cep(
>
>Are you sure this is the right place for this? Why not higher up? It
>looks like the other iwarp drivers have the same problem
>
>Jason
>
1) Good question. Do we want to allow applications to zero-size
rdma READ capabilities? Maybe we want, if it is recognized as a
security feature?
2) In any case, siw currently does not correctly handle the case
of zero sized ORD/IRD. If we want to go with 1), some fixes to siw
are to be done. If we do not want 1), Kamal's patch is half of the
story. It handles the response side only. Initiator would have to
be fixed as well.
I'd propose allowing 1). I'd fix siw accordingly. Opinions?
Thanks!
Bernard.
