esas2r_ioctl_handler() copies in a struct atto_express_ioctl, a bit over
512k, and hands the union member for the command to the handlers. For
EXPRESS_IOCTL_HBA and EXPRESS_IOCTL_SMP the length of the buffered ioctl
is the size of the member plus lengths that come straight from
userspace:
bi.length = sizeof(struct atto_ioctl) + ioctl_hba->data_length;
bi.length = sizeof(struct atto_ioctl_smp)
+ le32_to_cpu(si->req_length)
+ le32_to_cpu(si->rsp_length);
handle_buffered_ioctl() copies bi.length bytes from the member into a
DMA buffer and, once the adapter is done, copies the DMA buffer back
over the member. Nothing checks that the lengths fit into the struct
that was copied in. So a large data_length makes it read past the
struct and then write the DMA buffer back over the memory behind it.
The firmware commands pass sizeof(struct atto_express_ioctl) to
esas2r_{read,write}_{fw,fs}() as the length of data.fwrw.image, which
starts 28 bytes into the struct. So they can read and write 28 bytes
past it as well.
The handler is reachable through /proc/scsi/esas2r/ATTONode. It is
created with mode 0, which procfs turns into 0444, and esas2r doesn't
check for a capability anywhere. So any local user can do this on a
machine with such an adapter.
Bound all of these by the room from the member to the end of the
struct. read_hw() passes handle_hba_ioctl() a single struct atto_ioctl
and only ever returns that, so there the data length has to be zero.
Reported-by: Chris Mason <[email protected]>
Fixes: 26780d9e12ed ("[SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA
RAID Adapter Driver")
Cc: [email protected]
Signed-off-by: Christian Brauner (Amutable) <[email protected]>
---
drivers/scsi/esas2r/esas2r.h | 2 +-
drivers/scsi/esas2r/esas2r_ioctl.c | 41 ++++++++++++++++++++++++--------------
drivers/scsi/esas2r/esas2r_main.c | 3 ++-
3 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r.h b/drivers/scsi/esas2r/esas2r.h
index a763edd05fe4..10101f50b282 100644
--- a/drivers/scsi/esas2r/esas2r.h
+++ b/drivers/scsi/esas2r/esas2r.h
@@ -967,7 +967,7 @@ int esas2r_write_params(struct esas2r_adapter *a, struct
esas2r_request *rq,
int esas2r_ioctl_handler(void *hostdata, unsigned int cmd, void __user *arg);
int esas2r_ioctl(struct scsi_device *dev, unsigned int cmd, void __user *arg);
u8 handle_hba_ioctl(struct esas2r_adapter *a,
- struct atto_ioctl *ioctl_hba);
+ struct atto_ioctl *ioctl_hba, size_t room);
enum scsi_qc_status esas2r_queuecommand(struct Scsi_Host *host,
struct scsi_cmnd *cmd);
int esas2r_show_info(struct seq_file *m, struct Scsi_Host *sh);
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c
b/drivers/scsi/esas2r/esas2r_ioctl.c
index 3f7c1d131ec3..f99318f61c93 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -311,17 +311,27 @@ static int smp_ioctl_callback(struct esas2r_adapter *a,
return true;
}
+/* The room from @member to the end of a struct atto_express_ioctl. */
+#define EXPRESS_IOCTL_ROOM(member) \
+ (sizeof(struct atto_express_ioctl) - \
+ offsetof(struct atto_express_ioctl, member))
+
static u8 handle_smp_ioctl(struct esas2r_adapter *a, struct atto_ioctl_smp *si)
{
struct esas2r_buffered_ioctl bi;
+ u64 length = (u64)sizeof(struct atto_ioctl_smp) +
+ le32_to_cpu(si->req_length) +
+ le32_to_cpu(si->rsp_length);
+
+ /* The request and the response follow @si in the same buffer. */
+ if (length > EXPRESS_IOCTL_ROOM(data.ioctl_smp))
+ return IOCTL_INVALID_PARAM;
memset(&bi, 0, sizeof(bi));
bi.a = a;
bi.ioctl = si;
- bi.length = sizeof(struct atto_ioctl_smp)
- + le32_to_cpu(si->req_length)
- + le32_to_cpu(si->rsp_length);
+ bi.length = length;
bi.offset = 0;
bi.callback = smp_ioctl_callback;
return handle_buffered_ioctl(&bi);
@@ -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;
@@ -1349,31 +1364,27 @@ int esas2r_ioctl_handler(void *hostdata, unsigned int
cmd, void __user *arg)
err = esas2r_write_fw(a,
(char *)ioctl->data.fwrw.image,
0,
- sizeof(struct
- atto_express_ioctl));
+
EXPRESS_IOCTL_ROOM(data.fwrw.image));
if (err >= 0) {
err = esas2r_read_fw(a,
(char *)ioctl->data.fwrw.
image,
0,
- sizeof(struct
-
atto_express_ioctl));
+
EXPRESS_IOCTL_ROOM(data.fwrw.image));
}
} else if (ioctl->data.fwrw.img_type == FW_IMG_FS_API) {
err = esas2r_write_fs(a,
(char *)ioctl->data.fwrw.image,
0,
- sizeof(struct
- atto_express_ioctl));
+
EXPRESS_IOCTL_ROOM(data.fwrw.image));
if (err >= 0) {
err = esas2r_read_fs(a,
(char *)ioctl->data.fwrw.
image,
0,
- sizeof(struct
-
atto_express_ioctl));
+
EXPRESS_IOCTL_ROOM(data.fwrw.image));
}
} else {
ioctl->header.return_code = IOCTL_BAD_FLASH_IMGTYPE;
@@ -1441,9 +1452,9 @@ int esas2r_ioctl_handler(void *hostdata, unsigned int
cmd, void __user *arg)
break;
case EXPRESS_IOCTL_HBA:
- ioctl->header.return_code = handle_hba_ioctl(a,
- &ioctl->data.
- ioctl_hba);
+ ioctl->header.return_code =
+ handle_hba_ioctl(a, &ioctl->data.ioctl_hba,
+ EXPRESS_IOCTL_ROOM(data.ioctl_hba));
break;
case EXPRESS_IOCTL_VDA:
diff --git a/drivers/scsi/esas2r/esas2r_main.c
b/drivers/scsi/esas2r/esas2r_main.c
index ada278c24c51..75a70ae9441b 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -178,7 +178,8 @@ static ssize_t read_hw(struct file *file, struct kobject
*kobj,
if (!a->local_atto_ioctl)
return -ENOMEM;
- if (handle_hba_ioctl(a, a->local_atto_ioctl) != IOCTL_SUCCESS)
+ if (handle_hba_ioctl(a, a->local_atto_ioctl,
+ sizeof(struct atto_ioctl)) != IOCTL_SUCCESS)
return -ENOMEM;
memcpy(buf, a->local_atto_ioctl, length);
--
2.53.0
