On Wed, 23 Mar 2022 20:54:08 +0000
Robin Murphy <robin.murphy@xxxxxxx> wrote:
> On 2022-03-23 19:16, Linus Torvalds wrote:
> > On Wed, Mar 23, 2022 at 12:06 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> >>
> >> On 2022-03-23 17:27, Linus Torvalds wrote:
> >>>
> >>> I'm assuming that the ath9k issue is that it gives DMA mapping a big
> >>> enough area to handle any possible packet size, and just expects -
> >>> quite reasonably - smaller packets to only fill the part they need.
> >>>
> >>> Which that "info leak" patch obviously breaks entirely.
> >>
> >> Except that's the exact case which the new patch is addressing
> >
> > Not "addressing". Breaking.
> >
> > Which is why it will almost certainly get reverted.
> >
> > Not doing DMA to the whole area seems to be quite the sane thing to do
> > for things like network packets, and overwriting the part that didn't
> > get DMA'd with zeroes seems to be exactly the wrong thing here.
> >
> > So the SG_IO - and other random untrusted block command sources - data
> > leak will almost certainly have to be addressed differently. Possibly
> > by simply allocating the area with GFP_ZERO to begin with.
>
> Er, the point of the block layer case is that whole area *is* zeroed to
> begin with, and a latent memory corruption problem in SWIOTLB itself
> replaces those zeros with random other kernel data unexpectedly. Let me
> try illustrating some sequences for clarity...
>
> Expected behaviour/without SWIOTLB:
> Memory
> ---------------------------------------------------
> start 12345678
> dma_map(DMA_FROM_DEVICE) no-op
> device writes partial data 12ABC678 <- ABC
> dma_unmap(DMA_FROM_DEVICE) 12ABC678
>
>
> SWIOTLB previously:
> Memory Bounce buffer
> ---------------------------------------------------
> start 12345678 xxxxxxxx
> dma_map(DMA_FROM_DEVICE) no-op
> device writes partial data 12345678 xxABCxxx <- ABC
> dma_unmap(DMA_FROM_DEVICE) xxABCxxx <- xxABCxxx
>
>
> SWIOTLB Now:
> Memory Bounce buffer
> ---------------------------------------------------
> start 12345678 xxxxxxxx
> dma_map(DMA_FROM_DEVICE) 12345678 -> 12345678
> device writes partial data 12345678 12ABC678 <- ABC
> dma_unmap(DMA_FROM_DEVICE) 12ABC678 <- 12ABC678
>
>
> Now, sure we can prevent any actual information leakage by initialising
> the bounce buffer slot with zeros, but then we're just corrupting the
> not-written-to parts of the mapping with zeros instead of anyone else's
> old data. That's still fundamentally not OK. The only thing SWIOTLB can
> do to be correct is treat DMA_FROM_DEVICE as a read-modify-write of the
> entire mapping, because it has no way to know how much of it is actually
> going to be modified.
>
Very nice explanation! Thanks!
> I'll admit I still never quite grasped the reason for also adding the
> override to swiotlb_sync_single_for_device() in aa6f8dcbab47, but I
> think by that point we were increasingly tired and confused and starting
> to second-guess ourselves (well, I was, at least).
I raised the question, do we need to do the same for
swiotlb_sync_single_for_device(). Did that based on my understanding of the
DMA API documentation. I had the following scenario in mind
SWIOTLB without the snyc_single:
Memory Bounce buffer Owner
--------------------------------------------------------------------------
start 12345678 xxxxxxxx C
dma_map(DMA_FROM_DEVICE) 12345678 -> 12345678 C->D
device writes partial data 12345678 12ABC678 <- ABC D
sync_for_cpu(DMA_FROM_DEVICE) 12ABC678 <- 12ABC678 D->C
cpu modifies buffer 66666666 12ABC678 C
sync_for_device(DMA_FROM_DEVICE) 66666666 12ABC678 C->D
device writes partial data 66666666 1EFGC678 <-EFG D
dma_unmap(DMA_FROM_DEVICE) 1EFGC678 <- 1EFGC678 D->C
Legend: in Owner column C stands for cpu and D for device.
Without swiotlb, I believe we should have arrived at 6EFG6666. To get the
same result, IMHO, we need to do a sync in sync_for_device().
And aa6f8dcbab47 is an imperfect solution to that (because of size).
> I don't think it's
> wrong per se, but as I said I do think it can bite anyone who's been
> doing dma_sync_*() wrong but getting away with it until now.
I fully agree.
> If
> ddbd89deb7d3 alone turns out to work OK then I'd be inclined to try a
> partial revert of just that one hunk.
>
I'm not against being pragmatic and doing the partial revert. But as
explained above, I do believe for correctness of swiotlb we ultimately
do need that change. So if the revert is the short term solution,
what should be our mid-term road-map?
Regards,
Halil
> Thanks,
> Robin.
