| Message ID | 20200802152145.4387-1-baijiaju@tsinghua.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl() | expand |
On Sun, 2020-08-02 at 23:21 +0800, Jia-Ju Bai wrote: > Because "fs" is mapped to DMA, its data can be modified at anytime by > malicious or malfunctioning hardware. In this case, the check > "if (fsc->command >= cmdcnt)" can be passed, and then "fsc->command" > can be modified by hardware to cause buffer overflow. This threat model seems to be completely bogus. If the device were malicious it would have given the mailbox incorrect values a priori ... it wouldn't give the correct value then update it. For most systems we do assume correct operation of the device but if there's a worry about incorrect operation, the usual approach is to guard the device with an IOMMU which, again, would make this sort of fix unnecessary because the IOMMU will have removed access to the buffer after the command completed. James
On 2020/8/2 23:47, James Bottomley wrote: > On Sun, 2020-08-02 at 23:21 +0800, Jia-Ju Bai wrote: >> Because "fs" is mapped to DMA, its data can be modified at anytime by >> malicious or malfunctioning hardware. In this case, the check >> "if (fsc->command >= cmdcnt)" can be passed, and then "fsc->command" >> can be modified by hardware to cause buffer overflow. > This threat model seems to be completely bogus. If the device were > malicious it would have given the mailbox incorrect values a priori ... > it wouldn't give the correct value then update it. For most systems we > do assume correct operation of the device but if there's a worry about > incorrect operation, the usual approach is to guard the device with an > IOMMU which, again, would make this sort of fix unnecessary because the > IOMMU will have removed access to the buffer after the command > completed. Thanks for the reply :) In my opinion, IOMMU is used to prevent the hardware from accessing arbitrary memory addresses, but it cannot prevent the hardware from writing a bad value into a valid memory address. For this reason, I think that the hardware can normally access "fsc->command" and modify it into arbitrary value at any time, because IOMMU considers the address of "fsc->command" is valid for the hardware. Best wishes, Jia-Ju Bai
On Mon, 2020-08-03 at 11:07 +0800, Jia-Ju Bai wrote: > > On 2020/8/2 23:47, James Bottomley wrote: > > On Sun, 2020-08-02 at 23:21 +0800, Jia-Ju Bai wrote: > > > Because "fs" is mapped to DMA, its data can be modified at > > > anytime by malicious or malfunctioning hardware. In this case, > > > the check "if (fsc->command >= cmdcnt)" can be passed, and then > > > "fsc->command" can be modified by hardware to cause buffer > > > overflow. > > > > This threat model seems to be completely bogus. If the device were > > malicious it would have given the mailbox incorrect values a priori > > ... it wouldn't give the correct value then update it. For most > > systems we do assume correct operation of the device but if there's > > a worry about incorrect operation, the usual approach is to guard > > the device with an IOMMU which, again, would make this sort of fix > > unnecessary because the IOMMU will have removed access to the > > buffer after the command completed. > > Thanks for the reply :) > > In my opinion, IOMMU is used to prevent the hardware from accessing > arbitrary memory addresses, but it cannot prevent the hardware from > writing a bad value into a valid memory address. I think that's what I said above. It would give us a bad a priori value which copying can't help with. > For this reason, I think that the hardware can normally access > "fsc->command" and modify it into arbitrary value at any time, > because IOMMU considers the address of "fsc->command" is valid for > the hardware. Not if we suspected the device. I think esas2r does keep the buffer mapped, but if we suspected the device we'd only map it for the reply then unmap it. The point I'm making is we have hardware tools at our disposal to corral suspect devices if need be, but they're really only used in exceptional VM circumstances. Under ordinary circumstances we simply trust the device. So if you had evidence that esas2r were prone to faults, we'd usually force the manufacturer to fix the firmware and as a last resort we might consider corralling it with an iommu we wouldn't just copy some values. If you want an example of defensive coding, we had to add a load of checks to TPM devices to cope with the bus interposer situation. That's one case where we no longer trust the device to return correct information. However, to do the same for any SCSI device we'd need a convincing rationale for why. James
diff --git a/drivers/scsi/esas2r/esas2r_flash.c b/drivers/scsi/esas2r/esas2r_flash.c index b02ac389e6c6..ca5ff0d4c7d0 100644 --- a/drivers/scsi/esas2r/esas2r_flash.c +++ b/drivers/scsi/esas2r/esas2r_flash.c @@ -851,6 +851,7 @@ bool esas2r_process_fs_ioctl(struct esas2r_adapter *a, struct esas2r_ioctlfs_command *fsc = &fs->command; u8 func = 0; u32 datalen; + u8 command = fsc->command; fs->status = ATTO_STS_FAILED; fs->driver_error = RS_PENDING; @@ -860,18 +861,18 @@ bool esas2r_process_fs_ioctl(struct esas2r_adapter *a, return false; } - if (fsc->command >= cmdcnt) { + if (command >= cmdcnt) { fs->status = ATTO_STS_INV_FUNC; return false; } - func = cmd_to_fls_func[fsc->command]; + func = cmd_to_fls_func[command]; if (func == 0xFF) { fs->status = ATTO_STS_INV_FUNC; return false; } - if (fsc->command != ESAS2R_FS_CMD_CANCEL) { + if (command != ESAS2R_FS_CMD_CANCEL) { if ((a->pcid->device != ATTO_DID_MV_88RC9580 || fs->adap_type != ESAS2R_FS_AT_ESASRAID2) && (a->pcid->device != ATTO_DID_MV_88RC9580TS
In esas2r_read_fs(): struct esas2r_ioctl_fs *fs = (struct esas2r_ioctl_fs *)a->fs_api_buffer; Because "a->fs_api_buffer" is mapped to coherent DMA (allocated in esas2r_write_fs()), "fs" is also mapped to DMA. Then esas2r_read_fs() calls esas2r_process_fs_ioctl() with "fs". In esas2r_process_fs_ioctl(): fsc = &fs->command; ... if (fsc->command >= cmdcnt) { fs->status = ATTO_STS_INV_FUNC; return false; } func = cmd_to_fls_func[fsc->command]; if (func == 0xFF) { fs->status = ATTO_STS_INV_FUNC; return false; } Because "fs" is mapped to DMA, its data can be modified at anytime by malicious or malfunctioning hardware. In this case, the check "if (fsc->command >= cmdcnt)" can be passed, and then "fsc->command" can be modified by hardware to cause buffer overflow. To fix this problem, "fsc->command" is assigned to a local variable, and then this local variable is used to replace "fsc->command". Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn> --- drivers/scsi/esas2r/esas2r_flash.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)