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

[Hui Peng <benquike@gmail.com>]

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