On Tue, Nov 03, 2026 at 02:48:22PM +0000, [email protected] wrote: > 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()?
This whole codepath as currently implemented is structurally fscked. So it's not something fixable in this series apart from the glaringly obvious. But for the record: - The ATTONode entry thing is created as a side effect of reading another file. So esas2r_info() registers a character device and creates /proc/scsi/esas2r/ATTONode the first time it runs. That pins a global pointer to whichever host triggered it. So the control node only exists after someone looked at the host info. So it is bound to the first host and not to the adapter it operates on. The lifeteime is then tied to the module and not to the host. The entry is removed in esas2r_exit() on module exit. But the directory it lives in belongs to SCSI. That removes it when the last host of the template goes away. So unbind or hotunplug of the last adapter removes a non-empty directory... the dir is unlinked and the ATTONode leaks and stays reachable through a cached dentry. And the global host pointer dangles. So every ioctl on the node dereferences it. It's like non-empty directory removal basically. Fun. - No privilege check anywhere. The node is created with mode 0 so 0444 with procfs the the driver never calls capable(). Hence, any local user can read and write firmware and NVRAM and issue VDA commands. - The ioctl protocol itself (where to begin): - Plain-number commands - one struct copied in whole - unbounded lengths taken straight from userspace (well, that's fixed here) Sorry, I'm not going to redesign esas2r. And from what I can tell that ATTO stuff is legacy anyway so good riddance as far as I'm concerned.
