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 <[email protected]> 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. > 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 [ ... ] > @@ -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/[email protected]?part=28
