* Re: [PATCH 2/2] scsi: add basic CDB check to bsg_sg_io()
2026-10-29 9:26 ` [PATCH 2/2] scsi: add basic CDB check to bsg_sg_io() Dmitry Antipov
@ 2026-10-29 9:48 ` sashiko-bot
0 siblings, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-29 9:48 UTC (permalink / raw)
To: Dmitry Antipov; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 3+ messages in thread