Re: [PATCH 5.15.y] nvmet-tcp: fix use-before-check of sg in bounds validation

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

Hi Cengiz,

On 05/04/26 02:53, Cengiz Can wrote:
> The stable backport of commit 52a0a9854934 ("nvmet-tcp: add bounds
> checks in nvmet_tcp_build_pdu_iovec") placed the bounds checks after
> the iov_len calculation:
> 
>      while (length) {
>          u32 iov_len = min_t(u32, length, sg->length - sg_offset);
> 
>          if (!sg_remaining) {    /* too late: sg already dereferenced */
> 
> In mainline, the checks come first because C99 allows mid-block variable
> declarations. The stable backport moved the declaration to the top of the
> loop to satisfy C89 declaration rules, but this ended up placing the
> sg->length dereference before the sg_remaining and sg->length guards.
> 
> If sg_next() returns NULL at the end of the scatterlist, the next
> iteration dereferences a NULL pointer in the iov_len calculation before
> the sg_remaining check can prevent it.
> 
> Fix this by moving the iov_len declaration to function scope and
> keeping the assignment after the bounds checks, matching the ordering
> in mainline.
> 

Nice catch.

> Fixes: 42afe8ed8ad2 ("nvmet-tcp: add bounds checks in nvmet_tcp_build_pdu_iovec")
> Cc: stable@vger.kernel.org
> Cc: YunJe Shin <ioerts@kookmin.ac.kr>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: linux-nvme@lists.infradead.org
> Signed-off-by: Cengiz Can <cengiz.can@canonical.com>
> ---
>   drivers/nvme/target/tcp.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 8f7984c53f3f..c6cc1dfef92c 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -312,7 +312,7 @@ static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
>   {
>   	struct bio_vec *iov = cmd->iov;
>   	struct scatterlist *sg;
> -	u32 length, offset, sg_offset;
> +	u32 length, offset, sg_offset, iov_len;
>   	unsigned int sg_remaining;
>   	int nr_pages;
>   
> @@ -329,8 +329,6 @@ static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
>   	sg_remaining = cmd->req.sg_cnt - cmd->sg_idx;
>   
>   	while (length) {
> -		u32 iov_len = min_t(u32, length, sg->length - sg_offset);
> -
>   		if (!sg_remaining) {
>   			nvmet_tcp_fatal_error(cmd->queue);
>   			return;
> @@ -340,6 +338,8 @@ static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
>   			return;
>   		}
>   
> +		iov_len = min_t(u32, length, sg->length - sg_offset);
> +

Nit: Shouldn't we just be moving u32 iov_len = min_t(u32, length, 
sg->length - sg_offset); line here ?

(only a nit, but might benefit future backports, so asked)

Thanks,
Harshit

>   		iov->bv_page = sg_page(sg);
>   		iov->bv_len = iov_len;
>   		iov->bv_offset = sg->offset + sg_offset;