Re: [PATCH] scsi: qedi: fix OOB read in login/text response completion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: qedi: unclamped dlength in qedi_process_text_resp() and login_resp()
--

commit b81ce3a14651452d3f551e4635d2337a8b305729
Author: Henry Martin <bsdhenrymartin@gmail.com>

scsi: qedi: fix OOB read in login/text response completion

This commit clamps the payload length extracted from the CQE in
qedi_process_text_resp() and qedi_process_login_resp() to
qedi_conn->gen_pdu.resp_buf_size before advancing resp_wr_ptr, preventing
an out-of-bounds read when completing the PDU to userspace.

> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
> index c8af10c326532..5e20716043aee 100644
> --- a/drivers/scsi/qedi/qedi_fw.c
> +++ b/drivers/scsi/qedi/qedi_fw.c
> @@ -115,6 +115,8 @@ static void qedi_process_text_resp(struct qedi_ctx *qedi,
>  
>  	pld_len = cqe_text_response->hdr_second_dword &
>  		  ISCSI_TEXT_RESPONSE_HDR_DATA_SEG_LEN_MASK;
> +	if (pld_len > qedi_conn->gen_pdu.resp_buf_size)
> +		pld_len = qedi_conn->gen_pdu.resp_buf_size;
>  	qedi_conn->gen_pdu.resp_wr_ptr = qedi_conn->gen_pdu.resp_buf + pld_len;
>  
>  	memset(task_ctx, '\0', sizeof(*task_ctx));
> @@ -294,6 +296,8 @@ static void qedi_process_login_resp(struct qedi_ctx *qedi,
>  	resp_hdr_ptr->status_detail = cqe_login_response->status_detail;
>  	pld_len = cqe_login_response->hdr_second_dword &
>  		  ISCSI_LOGIN_RESPONSE_HDR_DATA_SEG_LEN_MASK;
> +	if (pld_len > qedi_conn->gen_pdu.resp_buf_size)
> +		pld_len = qedi_conn->gen_pdu.resp_buf_size;
>  	qedi_conn->gen_pdu.resp_wr_ptr = qedi_conn->gen_pdu.resp_buf + pld_len;

[Severity: Medium]
Should resp_hdr_ptr->dlength also be populated from the clamped pld_len in
both qedi_process_text_resp() and qedi_process_login_resp()?

Earlier in both functions, resp_hdr_ptr->dlength is written using the raw
unclamped 24-bit CQE value before pld_len is clamped to
qedi_conn->gen_pdu.resp_buf_size:

drivers/scsi/qedi/qedi_fw.c:qedi_process_text_resp() {
	...
	hton24(resp_hdr_ptr->dlength,
	       (cqe_text_response->hdr_second_dword &
		ISCSI_TEXT_RESPONSE_HDR_DATA_SEG_LEN_MASK));
	...
	pld_len = cqe_text_response->hdr_second_dword &
		  ISCSI_TEXT_RESPONSE_HDR_DATA_SEG_LEN_MASK;
	if (pld_len > qedi_conn->gen_pdu.resp_buf_size)
		pld_len = qedi_conn->gen_pdu.resp_buf_size;
	qedi_conn->gen_pdu.resp_wr_ptr = qedi_conn->gen_pdu.resp_buf + pld_len;

and similarly in qedi_process_login_resp():

drivers/scsi/qedi/qedi_fw.c:qedi_process_login_resp() {
	...
	hton24(resp_hdr_ptr->dlength,
	       (cqe_login_response->hdr_second_dword &
		ISCSI_LOGIN_RESPONSE_HDR_DATA_SEG_LEN_MASK));
	...
	pld_len = cqe_login_response->hdr_second_dword &
		  ISCSI_LOGIN_RESPONSE_HDR_DATA_SEG_LEN_MASK;
	if (pld_len > qedi_conn->gen_pdu.resp_buf_size)
		pld_len = qedi_conn->gen_pdu.resp_buf_size;
	qedi_conn->gen_pdu.resp_wr_ptr = qedi_conn->gen_pdu.resp_buf + pld_len;

When __iscsi_complete_pdu() calls iscsi_recv_pdu(), the netlink message is
allocated and populated with only data_size (the clamped pld_len) bytes of
payload following the header:

drivers/scsi/scsi_transport_iscsi.c:iscsi_recv_pdu() {
	int len = nlmsg_total_size(sizeof(*ev) + sizeof(struct iscsi_hdr) +
				   data_size);
	...
	pdu = (char*)ev + sizeof(*ev);
	memcpy(pdu, hdr, sizeof(struct iscsi_hdr));
	memcpy(pdu + sizeof(struct iscsi_hdr), data, data_size);

If the CQE data segment length exceeds resp_buf_size, won't this leave
resp_hdr_ptr->dlength advertising the larger unclamped length in the
embedded struct iscsi_hdr while only resp_buf_size bytes of payload are
actually present in the netlink message sent to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027150752.951217-1-bsdhenrymartin@gmail.com?part=1