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

[sashiko-bot@kernel.org]

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