Re: [PATCH] IB/isert: reject PDUs declaring more data than was received

On Mon, Jul 27, 2026 at 12:31:30AM +0900, Yehyeong Lee wrote:
> On Sun, Jul 26, 2026 at 09:46:28AM +0300, Leon Romanovsky wrote:
> > 1. You should review patches generated by AI.
> > 2. There is a need to add similar check to isert_get_login_rx() too.
> 
> You are right on both counts.  My changelog said the login handler
> "already rejects PDUs shorter than ISER_HEADERS_LEN"; I confirmed that a
> length check existed there and did not confirm that it was the same
> check.  It is not - 29e7b925ae6d bounds login_req_len from below, and
> nothing bounds the length the BHS declares.
> 
> I reproduced it before replying.  An initiator that sends a 105-byte
> first login PDU while declaring 8193 in the BHS DataSegmentLength, on
> 7.2.0-rc4 with KASAN over soft-RoCE:
> 
>   BUG: KASAN: slab-out-of-bounds in kmemdup_nul+0x43/0x80
>   Read of size 8193 at addr ffff8881056a8000 by task iscsi_np/167
>    __asan_memcpy+0x23/0x60
>    kmemdup_nul+0x43/0x80
>    iscsi_target_locate_portal+0x48d/0x1180
>    iscsi_target_login_thread+0x19a9/0x3350
>   Allocated by task 167:
>    __kmalloc_cache_noprof+0x158/0x370
>    iscsi_target_login_thread+0x971/0x3350
>   The buggy address belongs to the object at ffff8881056a8000
>    which belongs to the cache kmalloc-8k of size 8192
>   allocated 8192-byte region
> 
> faddr2line puts the allocation at iscsi_login_init_conn(),
> iscsi_target_login.c:50 - login->req_buf = kzalloc(MAX_KEY_VALUE_PAIRS) -
> and the read at iscsi_target_nego.c:1136, the kmemdup_nul() in
> iscsi_target_locate_portal().  The read size tracks the declared value
> exactly; 16384 gives "Read of size 16384", and a control run with a
> correct DataSegmentLength produces no report.  It is reached before
> authentication and the length is attacker-chosen.
> 
> On why isert is the one exposed: iscsi_target_check_login_request()
> already rejects payload_length > MAX_KEY_VALUE_PAIRS, but it is only
> called from iscsit_get_login_rx() and from cxgbit.  isert does not call
> it.  iscsit over TCP is safe by construction in any case - it reads
> exactly payload_length + padding from the socket, so the declared length
> governs how much arrives rather than how much is copied out of an
> already-filled buffer.
> 
> Your check is sufficient and not merely necessary: the posted login SGE
> is ISER_RX_PAYLOAD_SIZE, so login_req_len cannot exceed
> MAX_KEY_VALUE_PAIRS and size = min(login_req_len, MAX_KEY_VALUE_PAIRS)
> is login_req_len; once dlength <= login_req_len the copy out stays
> inside what was copied in.

I have little desire to argue with chatbot, but the second check MUST to
be added.

Thanks

> 
> I will send a v2 two-patch series - the isert_recv_done() check as 1/2
> and this one as 2/2 with Suggested-by: Leon Romanovsky
> <[email protected]> - once the before/after on the login case is run.
> 
> Best regards,