Re: [PATCH v3 1/2] RDMA/srpt: bound the indirect descriptor table to the IU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] RDMA/srpt: signed division truncation in srpt_get_desc_tbl() room check
--

commit a65e69a3e5c91bb2033461a63cf75aa8305efc27
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

RDMA/srpt: bound the indirect descriptor table to the IU

This commit bounds the indirect descriptor count in srpt_get_desc_tbl()
against the remaining space in recv_ioctx->byte_len to prevent out-of-bounds
reads in srpt_alloc_rw_ctxs().

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 7197d95f2216b..97758082f71d2 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1117,6 +1117,8 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
>  		struct srp_indirect_buf *idb = srpt_get_desc_buf(srp_cmd);
>  		int nbufs = be32_to_cpu(idb->table_desc.len) /
>  				sizeof(struct srp_direct_buf);
> +		int desc_off = (u8 *)idb - (u8 *)srp_cmd;
> +		int room;
>  
>  		if (nbufs >
>  		    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
> @@ -1128,6 +1130,14 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
>  			return -EINVAL;
>  		}
>  
> +		room = (recv_ioctx->byte_len - desc_off - (int)sizeof(*idb)) /
> +			(int)sizeof(struct srp_direct_buf);
> +		if (nbufs > room) {
> +			pr_err("received SRP_CMD with %d descriptors but room for %d in the IU\n",
> +			       nbufs, room);
> +			return -EINVAL;
> +		}
> +
>  		*data_len = be32_to_cpu(idb->len);
>  		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
>  				sg, sg_cnt);

[Severity: Medium]
The commit message notes:

"recv_ioctx->byte_len is signed, so a byte_len too small to cover even the
descriptor header yields a negative room and the same test refuses it."

In srpt_get_desc_tbl() in drivers/infiniband/ulp/srpt/ib_srpt.c, signed
integer division truncates toward zero. When recv_ioctx->byte_len is 1 to 15
bytes short of covering desc_off + sizeof(*idb), the numerator
(recv_ioctx->byte_len - desc_off - (int)sizeof(*idb)) is in [-15, -1], so
dividing by sizeof(struct srp_direct_buf) (16) produces room == 0 rather than
a negative value.

Also, idb->table_desc.len is read before recv_ioctx->byte_len is checked:

	int nbufs = be32_to_cpu(idb->table_desc.len) /
			sizeof(struct srp_direct_buf);

If be32_to_cpu(idb->table_desc.len) < 16, nbufs is 0. With both nbufs == 0
and room == 0, the nbufs > room check passes on the truncated request,
allowing srpt_get_desc_tbl() to read idb->len past recv_ioctx->byte_len and
call srpt_alloc_rw_ctxs() with nbufs == 0.

Can a truncated srp_cmd request bypass this bounds check when the byte
deficit is between 1 and 15 bytes and nbufs is 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022143901.2991062-1-yhlee@isslab.korea.ac.kr?part=1