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