[PATCH v2 0/6] sg: fixes for 5.13/scsi-staging

[PATCH v2 0/6] sg: fixes for 5.13/scsi-staging

[Douglas Gilbert]

This patchset addresses reports sent by Colin King to the linux-scsi
list in 20210311 based on coverity reports. There were also similar
reports from Dan Carpenter the following day. Plus syzbot (KASAN)
made a double free report. These were due to a 45 part patchset:
"sg: add v4 interface" applied to 5.13/scsi-staging recently.
Patches 1, 2 and 4 address those concerns.

Colin King sent a patch titled: "[PATCH][next] scsi: sg: Fix use of
pointer sfp after it has been kfree'd" [linux-scsi 20210311] which
should be applied.
Dan Carpenter sent a patch titled: "Re: [PATCH] scsi: sg: Fix a
warning message" [linux-scsi 20210312] regarding the use of
WARN_ONCE() which should be applied.

Patches 3, 5 and 6 are due to the author's ongoing testing.

This patchset is against MKP's repository, 5.13/scsi-staging
branch.

Douglas Gilbert (6):
  sg: sg_rq_map_kern: fix uninitialized
  sg: sg_remove_sfp_usercontext: remove NULL check
  sg: sg_rq_end_io: set SG_FRQ_ISSUED
  sg: fix double free of long scsi commands
  sg: tighten handling of struct request objects
  sg: remove debugging remnants

 drivers/scsi/sg.c | 87 +++++++++++++++++++----------------------------
 1 file changed, 35 insertions(+), 52 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] sg: sg_rq_map_kern: fix uninitialized

[Douglas Gilbert]

Variable k should not be used in call to sg_mk_kern_bio().

Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 2d4bbc1a1727..7d4a0fd9ee32 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2987,7 +2987,7 @@ sg_rq_map_kern(struct sg_request *srp, struct request_queue *q, struct request *
 		return 0;
 	if (rw_ind == WRITE)
 		op_flags = REQ_SYNC | REQ_IDLE;
-	bio = sg_mk_kern_bio(num_sgat - k);
+	bio = sg_mk_kern_bio(num_sgat);
 	if (!bio)
 		return -ENOMEM;
 	bio->bi_opf = req_op(rqq) | op_flags;
-- 
2.25.1

[PATCH v2 2/6] sg: sg_remove_sfp_usercontext: remove NULL check

[Douglas Gilbert]

The NULL check on sdp is useless as it has already been
de-referenced. sg_fd object without valid parent pointer
(sdp) should never occur.

Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 7d4a0fd9ee32..77fec70b7c2f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -3918,10 +3918,8 @@ sg_remove_sfp_usercontext(struct work_struct *work)
 	       o_count, sfp);
 	kfree(sfp);
 
-	if (sdp) {
-		scsi_device_put(sdp->device);
-		kref_put(&sdp->d_ref, sg_device_destroy);
-	}
+	scsi_device_put(sdp->device);
+	kref_put(&sdp->d_ref, sg_device_destroy);
 	module_put(THIS_MODULE);
 }
 
-- 
2.25.1

[PATCH v2 3/6] sg: sg_rq_end_io: set SG_FRQ_ISSUED

[Douglas Gilbert]

The SG_FRQ_ISSUED flag should be set when the driver knows the
block layer has issued a request with blk_execute_rq_nowait().
This flag was set on the line following that nowait() call.
However with blk_poll() the request may have already invoked
the completion call-back (sg_rq_end_io()) so set this flag
there as well.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 77fec70b7c2f..b6e06e039d5b 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2624,6 +2624,7 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 			set_bit(SG_FRQ_DEACT_ORPHAN, srp->frq_bm);
 		}
 	}
+	set_bit(SG_FRQ_ISSUED, srp->frq_bm);
 	if (test_bit(SG_FRQ_COUNT_ACTIVE, srp->frq_bm)) {
 		int num = atomic_inc_return(&sfp->waiting);
 
-- 
2.25.1

[PATCH v2 4/6] sg: fix double free of long scsi commands

[Douglas Gilbert]

In the post: "[syzbot] KASAN: invalid-free in sg_finish_scsi_blk_rq"
on 20210315 to the linux-scsi list KASAN reported a double free.
Remove the offending scsi_req_free_cmd(scsi_rp) call and note
with a comment where the change in ownership of long SCSI command
(> 16 bytes long) takes place.

Reported-by: syzbot+0a0e8ecea895d38332e6@syzkaller.appspotmail.com
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index b6e06e039d5b..c49f87c97dd4 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -3097,16 +3097,14 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 		set_bit(SG_FFD_HIPRI_SEEN, sfp->ffd_bm);
 
 	if (cwrp->cmd_len > BLK_MAX_CDB)
-		scsi_rp->cmd = long_cmdp;
+		scsi_rp->cmd = long_cmdp;	/* transfer ownership */
 	if (cwrp->u_cmdp)
 		res = sg_fetch_cmnd(cwrp->filp, sfp, cwrp->u_cmdp,
 				    cwrp->cmd_len, scsi_rp->cmd);
 	else
 		res = -EPROTO;
-	if (res) {
-		kfree(long_cmdp);
-		return res;
-	}
+	if (res)
+		goto fini;
 	scsi_rp->cmd_len = cwrp->cmd_len;
 	srp->cmd_opcode = scsi_rp->cmd[0];
 	us_xfer = !(rq_flags & (SG_FLAG_NO_DXFER | SG_FLAG_MMAP_IO));
@@ -3178,7 +3176,6 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	}
 fini:
 	if (unlikely(res)) {		/* failure, free up resources */
-		scsi_req_free_cmd(scsi_rp);
 		if (likely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ,
 					     srp->frq_bm))) {
 			srp->rq = NULL;
-- 
2.25.1

[PATCH v2 5/6] sg: tighten handling of struct request objects

[Douglas Gilbert]

The struct sg_request object is related to a struct request object
in the block layer. So a struct sg_request object holds a pointer
to the other object. And since an active struct request object is
shorter lived, then that pointer is set to NULL when it becomes
inactive. Tighten the handling of that pointer with READ_ONCE()
and WRITE_ONCE(). Also put a single lock around various state
variables in the completion callback (sg_rq_end_io() ) so observers
see either pre-change or post-change and not something in between.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c49f87c97dd4..f59ddf809f21 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -218,7 +218,7 @@ struct sg_request {	/* active SCSI command or inactive request */
 	unsigned long frq_bm[1];	/* see SG_FRQ_* defines above */
 	u8 *sense_bp;		/* mempool alloc-ed sense buffer, as needed */
 	struct sg_fd *parentfp;	/* pointer to owning fd, even when on fl */
-	struct request *rq;	/* released in sg_rq_end_io(), bio kept */
+	struct request *rqq;	/* released in sg_rq_end_io(), bio kept */
 	struct bio *bio;	/* kept until this req -->SG_RS_INACTIVE */
 	struct execute_work ew_orph;	/* harvest orphan request */
 };
@@ -1014,6 +1014,7 @@ sg_execute_cmd(struct sg_fd *sfp, struct sg_request *srp)
 {
 	bool at_head, is_v4h, sync;
 	struct sg_device *sdp = sfp->parentdp;
+	struct request *rqq;
 
 	is_v4h = test_bit(SG_FRQ_IS_V4I, srp->frq_bm);
 	sync = test_bit(SG_FRQ_SYNC_INVOC, srp->frq_bm);
@@ -1037,9 +1038,10 @@ sg_execute_cmd(struct sg_fd *sfp, struct sg_request *srp)
 		atomic_inc(&sfp->submitted);
 		set_bit(SG_FRQ_COUNT_ACTIVE, srp->frq_bm);
 	}
+	rqq = READ_ONCE(srp->rqq);
 	if (srp->rq_flags & SGV4_FLAG_HIPRI)
-		srp->rq->cmd_flags |= REQ_HIPRI;
-	blk_execute_rq_nowait(sdp->disk, srp->rq, (int)at_head, sg_rq_end_io);
+		rqq->cmd_flags |= REQ_HIPRI;
+	blk_execute_rq_nowait(sdp->disk, rqq, (int)at_head, sg_rq_end_io);
 	set_bit(SG_FRQ_ISSUED, srp->frq_bm);
 }
 
@@ -1116,7 +1118,7 @@ sg_common_write(struct sg_comm_wr_t *cwrp)
 		goto err_out;
  	}
 
-	if (unlikely(test_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm) || !srp->rq)) {
+	if (unlikely(test_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm) || !srp->rqq)) {
 		res = -EIDRM;	/* this failure unexpected but observed */
 		goto err_out;
 	}
@@ -1125,7 +1127,7 @@ sg_common_write(struct sg_comm_wr_t *cwrp)
 		res = -ENODEV;
 		goto err_out;
 	}
-	srp->rq->timeout = cwrp->timeout;
+	srp->rqq->timeout = cwrp->timeout;
 	sg_execute_cmd(fp, srp);
 	return srp;
 err_out:
@@ -2299,7 +2301,7 @@ sg_sfp_blk_poll(struct sg_fd *sfp, int loop_count)
 		return -EINVAL;
 	xa_lock_irqsave(xafp, iflags);
 	xa_for_each(xafp, idx, srp) {
-		rqq = srp->rq;
+		rqq = READ_ONCE(srp->rqq);
 		if (rqq && (srp->rq_flags & SGV4_FLAG_HIPRI) &&
 		    atomic_read(&srp->rq_st) == SG_RS_INFLIGHT &&
 		    test_bit(SG_FRQ_ISSUED, srp->frq_bm)) {
@@ -2320,8 +2322,8 @@ sg_srp_blk_poll(struct sg_request *srp, int loop_count)
 {
 	if (!test_bit(SG_FRQ_ISSUED, srp->frq_bm))
 		return 0;	/* blk_execute_rq_nowait() may not have issued request */
-	return sg_srp_q_blk_poll(srp, srp->rq, srp->parentfp->parentdp->device->request_queue,
-				 loop_count);
+	return sg_srp_q_blk_poll(srp, READ_ONCE(srp->rqq),
+				 srp->parentfp->parentdp->device->request_queue, loop_count);
 }
 
 /*
@@ -2553,6 +2555,7 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 	enum sg_rq_state rqq_state = SG_RS_AWAIT_RCV;
 	int a_resid, slen;
 	u32 rq_result;
+	unsigned long iflags;
 	struct sg_request *srp = rq->end_io_data;
 	struct scsi_request *scsi_rp = scsi_req(rq);
 	struct sg_device *sdp;
@@ -2624,7 +2627,10 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 			set_bit(SG_FRQ_DEACT_ORPHAN, srp->frq_bm);
 		}
 	}
-	set_bit(SG_FRQ_ISSUED, srp->frq_bm);
+	xa_lock_irqsave(&sfp->srp_arr, iflags);
+	__set_bit(SG_FRQ_ISSUED, srp->frq_bm);
+	sg_rq_chg_state_force_ulck(srp, rqq_state);
+	WRITE_ONCE(srp->rqq, NULL);
 	if (test_bit(SG_FRQ_COUNT_ACTIVE, srp->frq_bm)) {
 		int num = atomic_inc_return(&sfp->waiting);
 
@@ -2638,12 +2644,11 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 				WRITE_ONCE(sfp->low_await_idx, srp->rq_idx);
 		}
 	}
-	sg_rq_chg_state_force(srp, rqq_state);
+	xa_unlock_irqrestore(&sfp->srp_arr, iflags);
 	/*
 	 * Free the mid-level resources apart from the bio (if any). The bio's
 	 * blk_rq_unmap_user() can be called later from user context.
 	 */
-	srp->rq = NULL;
 	scsi_req_free_cmd(scsi_rp);
 	blk_put_request(rq);
 
@@ -3092,7 +3097,7 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	}
 	/* current sg_request protected by SG_RS_BUSY state */
 	scsi_rp = scsi_req(rq);
-	srp->rq = rq;
+	WRITE_ONCE(srp->rqq, rq);
 	if (rq_flags & SGV4_FLAG_HIPRI)
 		set_bit(SG_FFD_HIPRI_SEEN, sfp->ffd_bm);
 
@@ -3178,7 +3183,7 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	if (unlikely(res)) {		/* failure, free up resources */
 		if (likely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ,
 					     srp->frq_bm))) {
-			srp->rq = NULL;
+			WRITE_ONCE(srp->rqq, NULL);
 			blk_put_request(rq);
 		}
 	} else {
@@ -3214,9 +3219,9 @@ sg_finish_scsi_blk_rq(struct sg_request *srp)
 
 	/* Expect blk_put_request(rq) already called in sg_rq_end_io() */
 	if (unlikely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm))) {
-		struct request *rq = srp->rq;
+		struct request *rq = READ_ONCE(srp->rqq);
 
-		srp->rq = NULL;
+		WRITE_ONCE(srp->rqq, NULL);
 		if (rq) {       /* blk_get_request() may have failed */
 			if (scsi_req(rq))
 				scsi_req_free_cmd(scsi_req(rq));
-- 
2.25.1

[PATCH v2 6/6] sg: remove debugging remnants

[Douglas Gilbert]

Remove check whether the sg_request object's xarray mark indicates
that it is 'free' (it shouldn't be in sg_common_write() ). This was
added as part of scattergun debugging to find a problem that was
fixed elsewhere. The associated SG_LOG message has never been seen
in testing.

Likewise the SG_FRQ_BLK_PUT_REQ bit in the sg_request::frq_bm
bitmask was part of debugging.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 42 ++++++++++++------------------------------
 1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f59ddf809f21..f36a6a262774 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -115,10 +115,9 @@ enum sg_rq_state {	/* N.B. sg_rq_state_arr assumes SG_RS_AWAIT_RCV==2 */
 #define SG_FRQ_NO_US_XFER	3	/* no user space transfer of data */
 #define SG_FRQ_DEACT_ORPHAN	6	/* not keeping orphan so de-activate */
 #define SG_FRQ_RECEIVING	7	/* guard against multiple receivers */
-#define SG_FRQ_BLK_PUT_REQ	8	/* set when blk_put_request() called */
-#define SG_FRQ_FOR_MMAP		9	/* request needs PAGE_SIZE elements */
-#define SG_FRQ_COUNT_ACTIVE	10	/* sfp->submitted + waiting active */
-#define SG_FRQ_ISSUED		11	/* blk_execute_rq_nowait() finished */
+#define SG_FRQ_FOR_MMAP		8	/* request needs PAGE_SIZE elements */
+#define SG_FRQ_COUNT_ACTIVE	9	/* sfp->submitted + waiting active */
+#define SG_FRQ_ISSUED		10	/* blk_execute_rq_nowait() finished */
 
 /* Bit positions (flags) for sg_fd::ffd_bm bitmask follow */
 #define SG_FFD_FORCE_PACKID	0	/* receive only given pack_id/tag */
@@ -1118,15 +1117,6 @@ sg_common_write(struct sg_comm_wr_t *cwrp)
 		goto err_out;
  	}
 
-	if (unlikely(test_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm) || !srp->rqq)) {
-		res = -EIDRM;	/* this failure unexpected but observed */
-		goto err_out;
-	}
-	if (xa_get_mark(&fp->srp_arr, srp->rq_idx, SG_XA_RQ_FREE)) {
-		SG_LOG(1, fp, "%s: ahhh, request erased!!!\n", __func__);
-		res = -ENODEV;
-		goto err_out;
-	}
 	srp->rqq->timeout = cwrp->timeout;
 	sg_execute_cmd(fp, srp);
 	return srp;
@@ -2562,10 +2552,6 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 	struct sg_fd *sfp;
 
 	/* Expect 0 --> 1 transition, otherwise processed elsewhere */
-	if (unlikely(test_and_set_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm))) {
-		pr_info("%s: srp=%pK already completed\n", __func__, srp);
-		return;
-	}
 	sfp = srp->parentfp;
 	sdp = sfp->parentdp;
 
@@ -3181,11 +3167,8 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	}
 fini:
 	if (unlikely(res)) {		/* failure, free up resources */
-		if (likely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ,
-					     srp->frq_bm))) {
-			WRITE_ONCE(srp->rqq, NULL);
-			blk_put_request(rq);
-		}
+		WRITE_ONCE(srp->rqq, NULL);
+		blk_put_request(rq);
 	} else {
 		srp->bio = rq->bio;
 	}
@@ -3206,6 +3189,7 @@ sg_finish_scsi_blk_rq(struct sg_request *srp)
 {
 	int ret;
 	struct sg_fd *sfp = srp->parentfp;
+	struct request *rqq = READ_ONCE(srp->rqq);
 
 	SG_LOG(4, sfp, "%s: srp=0x%pK%s\n", __func__, srp,
 	       (srp->parentfp->rsv_srp == srp) ? " rsv" : "");
@@ -3217,15 +3201,13 @@ sg_finish_scsi_blk_rq(struct sg_request *srp)
 		atomic_dec(&sfp->waiting);
 	}
 
-	/* Expect blk_put_request(rq) already called in sg_rq_end_io() */
-	if (unlikely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm))) {
-		struct request *rq = READ_ONCE(srp->rqq);
-
+	/* Expect blk_put_request(rqq) already called in sg_rq_end_io() */
+	if (unlikely(rqq)) {
 		WRITE_ONCE(srp->rqq, NULL);
-		if (rq) {       /* blk_get_request() may have failed */
-			if (scsi_req(rq))
-				scsi_req_free_cmd(scsi_req(rq));
-			blk_put_request(rq);
+		if (rqq) {       /* blk_get_request() may have failed */
+			if (scsi_req(rqq))
+				scsi_req_free_cmd(scsi_req(rqq));
+			blk_put_request(rqq);
 		}
 	}
 	if (srp->bio) {
-- 
2.25.1