Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] scsi: esas2r: double fetch of data_length in handle_hba_ioctl() --
commit a2397b86d116881b0f288e71fe8885d678578a5f Author: Christian Brauner <[email protected]> scsi: esas2r: bound the lengths of the HBA, SMP and firmware ioctls This commit bounds the lengths of the HBA, SMP, and firmware ioctls in esas2r against the remaining room in struct atto_express_ioctl (or sizeof(struct atto_ioctl) for read_hw()) to prevent out-of-bounds DMA buffer reads and writes. > diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c > b/drivers/scsi/esas2r/esas2r_ioctl.c > index c93d9265e1008..80ae5b9d45db1 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; > bi.ioctl = ioctl_hba; > bi.length = sizeof(struct atto_ioctl) + ioctl_hba->data_length; [Severity: High] As reported by Sashiko on RFC [1], in drivers/scsi/esas2r/esas2r_main.c, read_hw() and write_hw() access the shared a->local_atto_ioctl buffer (allocated as sizeof(struct atto_ioctl)) without any driver lock: 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); ... } Because kernfs only serializes sysfs bin_attribute operations per open file description, concurrent read_hw() and write_hw() calls on two separate open file descriptors for the sysfs hw attribute can race on a->local_atto_ioctl. In handle_hba_ioctl(), ioctl_hba->data_length is read once for the bounds check against room - sizeof(struct atto_ioctl) and read a second time when computing bi.length. If a concurrent write_hw() overwrites a->local_atto_ioctl->data_length from 0 to a non-zero value between those two reads, wouldn't handle_buffered_ioctl() still read and write bi.length bytes past the end of a->local_atto_ioctl? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=41
