[PATCH] scsi: sg: clean up request on error paths in sg_read()

[PATCH] scsi: sg: clean up request on error paths in sg_read()

[Hui Peng]

In sg_read(), once sg_get_rq_mark() locates a completed Sg_request and
transitions srp->done from 1 to 2, an allocation failure in kzalloc() or
a user-copy fault (-EFAULT) in copy_to_user() / sg_read_oxfer() returns
or jumps to free_old_hdr without calling sg_finish_rem_req(srp) and
sg_remove_request(sfp, srp).

Because srp->done is already 2, subsequent sg_read() calls skip the
request, permanently leaking srp->bio and keeping sfp->res_in_use = 1
(blocking SG_SET_RESERVED_SIZE with -EBUSY and wedging non-SG_FLAG_NO_DXFER
writes with -EDOM) until the file descriptor is closed.

Always call sg_finish_rem_req(srp) and sg_remove_request(sfp, srp) under
the free_old_hdr cleanup label, matching sg_new_read().

Fixes: cb59e8408381 ("[PATCH] sg.c: update")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -480,8 +480,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 
 	hp = &srp->header;
 	old_hdr = kzalloc(SZ_SG_HEADER, GFP_KERNEL);
-	if (!old_hdr)
-		return -ENOMEM;
+	if (!old_hdr) {
+		retval = -ENOMEM;
+		goto free_old_hdr;
+	}
 
 	old_hdr->reply_len = (int) hp->timeout;
 	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
@@ -543,10 +545,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 		}
 	} else
 		count = (old_hdr->result == 0) ? 0 : -EIO;
-	sg_finish_rem_req(srp);
-	sg_remove_request(sfp, srp);
 	retval = count;
 free_old_hdr:
+	sg_finish_rem_req(srp);
+	sg_remove_request(sfp, srp);
 	kfree(old_hdr);
 	return retval;
 }
-- 
2.43.0

Re: [PATCH] scsi: sg: clean up request on error paths in sg_read()

[John Garry]

On 9/19/26 21:48, Hui Peng wrote:
> In sg_read(), once sg_get_rq_mark() locates a completed Sg_request and
> transitions srp->done from 1 to 2, an allocation failure in kzalloc() or
> a user-copy fault (-EFAULT) in copy_to_user() / sg_read_oxfer() returns
> or jumps to free_old_hdr without calling sg_finish_rem_req(srp) and
> sg_remove_request(sfp, srp).
> 
> Because srp->done is already 2, subsequent sg_read() calls skip the
> request, permanently leaking srp->bio and keeping sfp->res_in_use = 1
> (blocking SG_SET_RESERVED_SIZE with -EBUSY and wedging non-SG_FLAG_NO_DXFER
> writes with -EDOM) until the file descriptor is closed.
> 
> Always call sg_finish_rem_req(srp) and sg_remove_request(sfp, srp) under
> the free_old_hdr cleanup label, matching sg_new_read().
> 
> Fixes: cb59e8408381 ("[PATCH] sg.c: update")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>

How was this tested?

> ---
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -480,8 +480,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
>   
>   	hp = &srp->header;
>   	old_hdr = kzalloc(SZ_SG_HEADER, GFP_KERNEL);
> -	if (!old_hdr)
> -		return -ENOMEM;
> +	if (!old_hdr) {
> +		retval = -ENOMEM;
> +		goto free_old_hdr;
> +	}
>   
>   	old_hdr->reply_len = (int) hp->timeout;
>   	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
> @@ -543,10 +545,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
>   		}
>   	} else
>   		count = (old_hdr->result == 0) ? 0 : -EIO;
> -	sg_finish_rem_req(srp);
> -	sg_remove_request(sfp, srp);
>   	retval = count;
>   free_old_hdr:

This is no longer a fully appropriate label name

> +	sg_finish_rem_req(srp);
> +	sg_remove_request(sfp, srp);
>   	kfree(old_hdr);
>   	return retval;
>   }

[PATCH v2] scsi: sg: finish request on error in sg_read()

[Hui Peng]

In sg_read(), for the legacy sg_header interface, a completed request
(srp) is dequeued from the ready list via sg_get_rq_mark(). However, if
kzalloc() fails, or if copy_to_user() or sg_read_oxfer() fails with
-EFAULT, sg_read() returns immediately without calling
sg_finish_rem_req(srp) and sg_remove_request(sfp, srp).

Because sg_get_rq_mark() clears the request from the ready list, this
completed request is left orphaned on sfp->rq_list in an active state,
leaking its kernel buffers and pages until the file descriptor is
closed. Fix this by jumping to finish_req and calling
sg_finish_rem_req() and sg_remove_request() before returning on error.

This was tested in QEMU (7.3.0-rc3) using scsi_debug (/dev/sg0):
1. Queued a legacy struct sg_header INQUIRY command via write().
2. Waited for completion via poll().
3. Invoked read() with an unmapped userspace buffer (NULL, count = 64),
   returning -EFAULT.
4. Queried the request state via the SG_GET_REQUEST_TABLE ioctl.
Before this fix, req_tbl[0].req_state remained 2 (active/orphaned on
sfp->rq_list) after the failed read(); with this fix, req_tbl[0].req_state
is 0 (properly finished and removed).

The C reproducer (PoC) can be provided upon request.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Suggested-by: John Garry <john.garry@linux.dev>
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Rename error cleanup label from `free_old_hdr:` to `finish_req:`
  since it now finishes and removes `srp` in addition to freeing `old_hdr`
  (John Garry).
- Also handle `kzalloc(SZ_SG_HEADER, GFP_KERNEL)` failure by setting
  `retval = -ENOMEM` and jumping to `finish_req`, as `srp` has already
  been dequeued by `sg_get_rq_mark()` at that point.
- Moved testing explanation into the commit message body.
- Added note that C reproducer (PoC) can be provided upon request.

 drivers/scsi/sg.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 5408f002e6c0..5d635349e06e 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -480,8 +480,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 
 	hp = &srp->header;
 	old_hdr = kzalloc(SZ_SG_HEADER, GFP_KERNEL);
-	if (!old_hdr)
-		return -ENOMEM;
+	if (!old_hdr) {
+		retval = -ENOMEM;
+		goto finish_req;
+	}
 
 	old_hdr->reply_len = (int) hp->timeout;
 	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
@@ -543,10 +545,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 		}
 	} else
 		count = (old_hdr->result == 0) ? 0 : -EIO;
-	sg_finish_rem_req(srp);
-	sg_remove_request(sfp, srp);
 	retval = count;
-free_old_hdr:
+finish_req:
+	sg_finish_rem_req(srp);
+	sg_remove_request(sfp, srp);
 	kfree(old_hdr);
 	return retval;
 }
-- 
2.53.0.797.g7842e34a66-goog