Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: esas2r: TOCTOU race on data_length in handle_hba_ioctl() via read_hw()
--
commit 8af21917bc01e9991b2ab872e7a4c38c91a6e388
Author: Christian Brauner [off-list ref]
scsi: esas2r: bound the lengths of the HBA, SMP and firmware ioctls
This commit adds bounds checks on user-supplied lengths for the HBA, SMP,
and firmware ioctls in esas2r to prevent out-of-bounds reads and writes past
the end of struct atto_express_ioctl and a->local_atto_ioctl.
quoted hunk ↗ jump to hunk
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c
index 3f7c1d131ec34..f99318f61c934 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1221,11 +1231,16 @@ static void hba_ioctl_done_callback(struct esas2r_adapter *a,
}
}
+/* @room is the size of the buffer that starts with @ioctl_hba. */
u8 handle_hba_ioctl(struct esas2r_adapter *a,
- struct atto_ioctl *ioctl_hba)
+ struct atto_ioctl *ioctl_hba, size_t room)
{
struct esas2r_buffered_ioctl bi;
+ /* The data follows @ioctl_hba in the same buffer. */
+ if (ioctl_hba->data_length > room - sizeof(struct atto_ioctl))
+ return IOCTL_INVALID_PARAM;
+
memset(&bi, 0, sizeof(bi));
bi.a = a;
[Severity: High]
Can a concurrent write_hw() bypass this bounds check when handle_hba_ioctl()
is called from read_hw()?
In drivers/scsi/esas2r/esas2r_main.c, read_hw() and write_hw() both operate
on the shared a->local_atto_ioctl buffer without holding a driver lock (and
kernfs only serializes per open file description via of->mutex, not across
separate file descriptors):
drivers/scsi/esas2r/esas2r_main.c:read_hw() {
...
if (handle_hba_ioctl(a, a->local_atto_ioctl,
sizeof(struct atto_ioctl)) != IOCTL_SUCCESS)
return -ENOMEM;
...
}
drivers/scsi/esas2r/esas2r_main.c:write_hw() {
...
memset(a->local_atto_ioctl, 0, sizeof(struct atto_ioctl));
memcpy(a->local_atto_ioctl, buf, length);
...
}
Inside handle_hba_ioctl(), ioctl_hba->data_length is read twice without
synchronization:
drivers/scsi/esas2r/esas2r_ioctl.c:handle_hba_ioctl() {
if (ioctl_hba->data_length > room - sizeof(struct atto_ioctl))
return IOCTL_INVALID_PARAM;
memset(&bi, 0, sizeof(bi));
bi.a = a;
bi.ioctl = ioctl_hba;
bi.length = sizeof(struct atto_ioctl) + ioctl_hba->data_length;
...
return handle_buffered_ioctl(&bi);
}
If write_hw() overwrites a->local_atto_ioctl->data_length with a non-zero
value between the bounds check and the assignment to bi.length, wouldn't
handle_buffered_ioctl() still read and write bi.length bytes past the end of
a->local_atto_ioctl?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-work-procfs-bugfixes-v1-0-59422ed3e465@kernel.org?part=28