[PATCH] scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl()

[PATCH] scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl()

[Jia-Ju Bai]

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(-)

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
-- 
2.17.1

Re: [PATCH] scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl()

[James Bottomley]

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

Re: [PATCH] scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl()

[Jia-Ju Bai]

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

Re: [PATCH] scsi: esas2r: fix possible buffer overflow caused by bad DMA value in esas2r_process_fs_ioctl()

[James Bottomley]

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