[PATCH 1/2] scsi: define minimum CDB size

[PATCH 1/2] scsi: define minimum CDB size

[Dmitry Antipov]

According to SPC3, fixed length CDB can't be shorter
than 6 bytes, so define the convenient SCSI_MIN_CDB_SIZE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 include/scsi/scsi_proto.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index 9c7d31aa6bae..c7d2c38be4b2 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -187,6 +187,7 @@
  *	SCSI command lengths
  */
 
+#define SCSI_MIN_CDB_SIZE 6
 #define SCSI_MAX_VARLEN_CDB_SIZE 260
 
 /* defined in T10 SCSI Primary Commands-2 (SPC2) */
-- 
2.56.0

[PATCH 2/2] scsi: add basic CDB check to bsg_sg_io()

[Dmitry Antipov]

Local fuzzing of 6.12.113 has reported the following crash:

[ T6325] KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
[ T6325] CPU: 1 UID: 0 PID: 6325 Comm: syz.0.17 Tainted: G W 6.12.113 #2
...
[  123.544088][ T6325] RIP: 0010:ufs_bsg_request+0x244/0x11f0
...
[  123.550658][ T6325] Call Trace:
[  123.550918][ T6325]  <TASK>
[  123.551154][ T6325]  ? __pfx_ufs_bsg_request+0x10/0x10
[  123.551599][ T6325]  bsg_queue_rq+0x4e2/0x780
[  123.551991][ T6325]  ? __pfx_bsg_queue_rq+0x10/0x10
[  123.552526][ T6325]  ? lockdep_unlock+0x5c/0xd0
[  123.553098][ T6325]  blk_mq_dispatch_rq_list+0x499/0x1990
[  123.553819][ T6325]  ? do_raw_spin_lock+0x12b/0x2f0
[  123.554458][ T6325]  ? __pfx_blk_mq_dispatch_rq_list+0x10/0x10
[  123.555332][ T6325]  ? do_raw_spin_unlock+0xf5/0x200
[  123.556247][ T6325]  __blk_mq_sched_dispatch_requests+0x355/0x1580
[  123.557644][ T6325]  ? __lock_acquire+0x761/0x2e30
[  123.559226][ T6325]  ? do_raw_spin_lock+0x12b/0x2f0
[  123.562128][ T6325]  ? __pfx___blk_mq_sched_dispatch_requests+0x10/0x10
[  123.565938][ T6325]  ? lock_acquire+0x115/0x350
[  123.568870][ T6325]  ? lock_acquire+0x115/0x350
[  123.571884][ T6325]  blk_mq_sched_dispatch_requests+0xd7/0x180
[  123.574304][ T6325]  ? blk_mq_run_hw_queue+0x317/0x4e0
[  123.575617][ T6325]  blk_mq_run_hw_queue+0x340/0x4e0
[  123.576586][ T6325]  blk_execute_rq+0x1e0/0x4c0
[  123.577254][ T6325]  ? import_ubuf+0xfb/0x1d0
[  123.577847][ T6325]  ? blk_rq_map_user+0x11b/0x1a0
[  123.578440][ T6325]  ? __pfx_blk_execute_rq+0x10/0x10
[  123.579013][ T6325]  ? _copy_from_user+0x94/0xb0
[  123.579509][ T6325]  bsg_transport_sg_io_fn+0x5ff/0xef0
[  123.580093][ T6325]  bsg_ioctl+0x7ec/0xa10
[  123.580603][ T6325]  ? __pfx_bsg_transport_sg_io_fn+0x10/0x10
[  123.581248][ T6325]  ? __pfx_bsg_ioctl+0x10/0x10
[  123.581695][ T6325]  ? __fget_files+0x2a/0x420
[  123.582057][ T6325]  ? bpf_lsm_file_ioctl+0x9/0x20
[  123.582437][ T6325]  ? __pfx_bsg_ioctl+0x10/0x10
[  123.582789][ T6325]  __se_sys_ioctl+0xfc/0x170
[  123.583123][ T6325]  do_syscall_64+0x166/0x520
[  123.583470][ T6325]  ? trace_irq_disable+0x3b/0x140
[  123.583904][ T6325]  ? entry_SYSCALL_64_after_hwframe+0x77/0x7f
[  123.584412][ T6325]  entry_SYSCALL_64_after_hwframe+0x77/0x7f

This is caused by ioctl(..., SG_IO, {struct sg_io_v4 hdr = {
.subprotocol = BSG_SUB_PROTOCOL_SCSI_TRANSPORT, .request = NULL,
.request_len = 0 }}). So if 'hdr' is not a task management
function, perform basic 'request' and 'request_len' check
assuming that there should be a valid fixed length CDB at least.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 block/bsg-lib.c         | 2 --
 block/bsg.c             | 9 +++++++++
 include/linux/bsg-lib.h | 2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 536cee4430a7..e7ae60f73870 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -16,8 +16,6 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/sg.h>
 
-#define uptr64(val) ((void __user *)(uintptr_t)(val))
-
 struct bsg_set {
 	struct blk_mq_tag_set	tag_set;
 	struct bsg_device	*bd;
diff --git a/block/bsg.c b/block/bsg.c
index 82aaf3cee582..80f367ae7d24 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -11,6 +11,7 @@
 #include <linux/percpu.h>
 #include <linux/idr.h>
 #include <linux/bsg.h>
+#include <linux/bsg-lib.h>
 #include <linux/slab.h>
 #include <linux/io_uring/cmd.h>
 
@@ -66,6 +67,14 @@ static int bsg_sg_io(struct bsg_device *bd, bool open_for_write,
 		return -EFAULT;
 	if (hdr.guard != 'Q')
 		return -EINVAL;
+
+	if (hdr.subprotocol != BSG_SUB_PROTOCOL_SCSI_TMF) {
+		if (hdr.request_len < SCSI_MIN_CDB_SIZE)
+			return -EINVAL;
+		if (!access_ok(uptr64(hdr.request), hdr.request_len))
+			return -EFAULT;
+	}
+
 	ret = bd->sg_io_fn(bd->queue, &hdr, open_for_write,
 			   bsg_timeout(bd, &hdr));
 	if (!ret && copy_to_user(uarg, &hdr, sizeof(hdr)))
diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h
index a697246feeac..fac7c89516ca 100644
--- a/include/linux/bsg-lib.h
+++ b/include/linux/bsg-lib.h
@@ -17,6 +17,8 @@ struct device;
 struct scatterlist;
 struct request_queue;
 
+#define uptr64(val) ((void __user *)(uintptr_t)(val))
+
 typedef int (bsg_job_fn) (struct bsg_job *);
 typedef enum blk_eh_timer_return (bsg_timeout_fn)(struct request *);
 
-- 
2.56.0

Re: [PATCH 2/2] scsi: add basic CDB check to bsg_sg_io()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] block: bsg: incomplete request_len check in bsg_sg_io() allows OOB reads
- [High] block: bsg: SCSI_MIN_CDB_SIZE check in bsg_sg_io() breaks SAS SMP and FC BSG
- [Medium] block: bsg: misplaced CDB and access_ok() checks in bsg_sg_io()
--

commit f1d167c5abeebaea771b50853f4c7fc693ccc640
Author: Dmitry Antipov <dmantipov@yandex.ru>

scsi: add basic CDB check to bsg_sg_io()

This commit moves the uptr64() macro from block/bsg-lib.c to
include/linux/bsg-lib.h and adds a check in bsg_sg_io() requiring
hdr.request_len to be at least SCSI_MIN_CDB_SIZE and hdr.request to pass
access_ok() when hdr.subprotocol is not BSG_SUB_PROTOCOL_SCSI_TMF.

> diff --git a/block/bsg.c b/block/bsg.c
> index 82aaf3cee5825..80f367ae7d240 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -11,6 +11,7 @@
>  #include <linux/percpu.h>
>  #include <linux/idr.h>
>  #include <linux/bsg.h>
> +#include <linux/bsg-lib.h>
>  #include <linux/slab.h>
>  #include <linux/io_uring/cmd.h>
>  
> @@ -66,6 +67,14 @@ static int bsg_sg_io(struct bsg_device *bd, bool open_for_write,
>  		return -EFAULT;
>  	if (hdr.guard != 'Q')
>  		return -EINVAL;
> +
> +	if (hdr.subprotocol != BSG_SUB_PROTOCOL_SCSI_TMF) {
> +		if (hdr.request_len < SCSI_MIN_CDB_SIZE)
> +			return -EINVAL;

[Severity: High]
Does checking hdr.request_len < SCSI_MIN_CDB_SIZE (6 bytes) in bsg_sg_io()
prevent out-of-bounds reads in ufs_bsg_request() and other bsg-lib transport
handlers?

For BSG_SUB_PROTOCOL_SCSI_TRANSPORT queues, bsg_transport_sg_io_fn()
allocates job->request with the exact user-supplied length:

block/bsg-lib.c:bsg_transport_sg_io_fn() {
	...
	job->request_len = hdr->request_len;
	job->request = memdup_user(uptr64(hdr->request), hdr->request_len);
	...
}

Unlike BSG_SUB_PROTOCOL_SCSI_CMD, BSG_SUB_PROTOCOL_SCSI_TRANSPORT requests do
not carry a SCSI CDB in hdr.request, but instead pass transport-specific
structures. In drivers/ufs/core/ufs_bsg.c, ufs_bsg_request() casts
job->request to struct ufs_bsg_request (36 bytes) and
ufs_bsg_exec_advanced_rpmb_req() casts job->request to struct
ufs_rpmb_request without checking job->request_len:

drivers/ufs/core/ufs_bsg.c:ufs_bsg_request() {
	struct ufs_bsg_request *bsg_request = job->request;
	...
	msgcode = bsg_request->msgcode;
	switch (msgcode) {
	case UPIU_TRANSACTION_QUERY_REQ:
		desc_op = bsg_request->upiu_req.qr.opcode;
	...
}

drivers/ufs/core/ufs_bsg.c:ufs_bsg_exec_advanced_rpmb_req() {
	struct ufs_rpmb_request *rpmb_request = job->request;
	...
	if (rpmb_request->ehs_req.length != 2 ||
	    rpmb_request->ehs_req.ehs_type != 1)
		return -EINVAL;
	...
}

Similarly, in drivers/scsi/mpi3mr/mpi3mr_app.c, mpi3mr_bsg_request() and its
helpers mpi3mr_bsg_process_drv_cmds() and mpi3mr_bsg_process_mpt_cmds()
dereference struct mpi3mr_bsg_packet from job->request without validating
job->request_len.

If userspace passes hdr.request_len = 6 with
hdr.subprotocol = BSG_SUB_PROTOCOL_SCSI_TRANSPORT, won't it bypass this
SCSI_MIN_CDB_SIZE check in bsg_sg_io(), allocate a 6-byte buffer in
bsg_transport_sg_io_fn(), and still trigger a slab-out-of-bounds read in
ufs_bsg_request(), ufs_bsg_exec_advanced_rpmb_req(), or
mpi3mr_bsg_request()?

[Severity: High]
Will requiring hdr.request_len >= SCSI_MIN_CDB_SIZE in bsg_sg_io() also cause
regressions for existing BSG_SUB_PROTOCOL_SCSI_TRANSPORT users such as SAS
SMP passthrough and FC BSG?

In drivers/scsi/scsi_transport_sas.c, sas_smp_dispatch() and the SAS
transport smp_handler callbacks transfer the SMP request and response frames
entirely through job->request_payload (hdr.dout_xferp / hdr.dout_xfer_len)
and job->reply_payload (hdr.din_xferp / hdr.din_xfer_len), leaving
hdr.request = 0 and hdr.request_len = 0:

drivers/scsi/scsi_transport_sas.c:sas_smp_dispatch() {
	...
	if (!job->reply_payload.payload_len) {
		dev_warn(job->dev, "space for a smp response is missing\n");
		bsg_job_done(job, -EINVAL, 0);
		return 0;
	}

	to_sas_internal(shost->transportt)->f->smp_handler(job, shost, rphy);
	return 0;
}

Additionally, in drivers/scsi/scsi_transport_fc.c, fc_bsg_rport_dispatch()
accepts FC_BSG_RPT_ELS commands whose request length is
sizeof(uint32_t) + sizeof(struct fc_bsg_rport_els) (5 bytes), and both
fc_bsg_host_dispatch() / fc_bsg_rport_dispatch() and
iscsi_bsg_host_dispatch() in drivers/scsi/scsi_transport_iscsi.c explicitly
check for job->request_len < sizeof(uint32_t) to return -ENOMSG:

drivers/scsi/scsi_transport_fc.c:fc_bsg_rport_dispatch() {
	int cmdlen = sizeof(uint32_t);	/* start with length of msgcode */
	int ret;

	/* check if we really have all the request data needed */
	if (job->request_len < cmdlen) {
		ret = -ENOMSG;
		goto fail_rport_msg;
	}

	/* Validate the rport command */
	switch (bsg_request->msgcode) {
	case FC_BSG_RPT_ELS:
		cmdlen += sizeof(struct fc_bsg_rport_els);
	...
}

Won't rejecting hdr.request_len < 6 with -EINVAL in bsg_sg_io() break SAS SMP
passthrough requests with hdr.request_len == 0, reject valid 5-byte
FC_BSG_RPT_ELS requests, and change the -ENOMSG return code expected by FC
and iSCSI BSG callers?

> +		if (!access_ok(uptr64(hdr.request), hdr.request_len))
> +			return -EFAULT;
> +	}

[ ... ]

> diff --git a/include/linux/bsg-lib.h b/include/linux/bsg-lib.h
> index a697246feeaca..fac7c89516caf 100644
> --- a/include/linux/bsg-lib.h
> +++ b/include/linux/bsg-lib.h
> @@ -17,6 +17,8 @@ struct device;
>  struct scatterlist;
>  struct request_queue;
>  
> +#define uptr64(val) ((void __user *)(uintptr_t)(val))

[Severity: Medium]
Is the access_ok() check in bsg_sg_io() and exposing uptr64() in
include/linux/bsg-lib.h needed here?

In the crash described by the commit message (hdr.request = NULL,
hdr.request_len = 0), ufs_bsg_request() dereferenced ZERO_SIZE_PTR (0x10)
because memdup_user(NULL, 0) in bsg_transport_sg_io_fn() returns
ZERO_SIZE_PTR when the length is 0. Calling access_ok(NULL, len) does not
reject a NULL pointer because __access_ok() treats address 0 as within the
user address range.

When hdr.request_len > 0, both bd->sg_io_fn() implementations already copy
hdr.request from userspace and return -EFAULT on invalid pointers:
scsi_bsg_sg_io_fn() in drivers/scsi/scsi_bsg.c uses copy_from_user(), and
bsg_transport_sg_io_fn() in block/bsg-lib.c uses memdup_user(). Running
access_ok() earlier in bsg_sg_io() causes requests with invalid kernel
addresses in hdr.request to return -EFAULT before bd->sg_io_fn() checks
hdr->protocol and hdr->subprotocol (-EINVAL), CAP_SYS_RAWIO (-EPERM in
bsg_transport_sg_io_fn()), bidirectional support (-EOPNOTSUPP in
scsi_bsg_sg_io_fn()), or scmd->cmd_len > sizeof(scmd->cmnd) (-EINVAL in
scsi_bsg_sg_io_fn()), while also exposing the unprefixed uptr64() macro in
include/linux/bsg-lib.h.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029092624.579887-1-dmantipov@yandex.ru?part=2