Messages in this thread | | | Date | Wed, 6 Sep 2023 17:18:51 +0100 | Subject | Re: [PATCH 0/2] KVM: x86/mmu: .change_pte() optimization in TDP MMU | From | Robin Murphy <> |
| |
On 2023-09-06 15:44, Sean Christopherson wrote: > On Wed, Sep 06, 2023, Robin Murphy wrote: >> On 2023-09-05 19:59, Sean Christopherson wrote: >>> And if the driver *doesn't* initialize the data, then the copy is at best pointless, >>> and possibly even worse than leaking stale swiotlb data. >> >> Other than the overhead, done right it can't be any worse than if SWIOTLB >> were not involved at all. > > Yep. > >>> Looking at commit ddbd89deb7d3 ("swiotlb: fix info leak with DMA_FROM_DEVICE"), >>> IIUC the data leak was observed with a synthetic test "driver" that was developed >>> to verify a real data leak fixed by commit a45b599ad808 ("scsi: sg: allocate with >>> __GFP_ZERO in sg_build_indirect()"). Which basically proves my point: copying >>> from the source only adds value absent a bug in the owning driver. >> >> Huh? IIUC the bug there was that the SCSI layer failed to sanitise >> partially-written buffers. That bug was fixed, and the scrutiny therein >> happened to reveal that SWIOTLB *also* had a lower-level problem with >> partial writes, in that it was corrupting DMA-mapped memory which was not >> updated by the device. Partial DMA writes are not in themselves indicative >> of a bug, they may well be a valid and expected behaviour. > > The problem is that the comment only talks about leaking data to userspace, and > doesn't say anything about data corruption or the "swiotlb needs to match hardware" > justification that Linus pointed out. I buy both of those arguments for copying > data from the original page, but the "may prevent leaking swiotlb content" is IMO > completely nonsensical, because if preventing leakage is the only goal, then > explicitly initializing the memory is better in every way. > > If no one objects, I'll put together a patch to rewrite the comment in terms of > mimicking hardware and not corrupting the caller's data.
Sounds good to me. I guess the trouble is that as soon as a CVE is involved it can then get hard to look past it, or want to risk appearing to downplay it :)
>>> IMO, rather than copying from the original memory, swiotlb_tbl_map_single() should >>> simply zero the original page(s) when establishing the mapping. That would harden >>> all usage of swiotlb and avoid the read-before-write behavior that is problematic >>> for KVM. >> >> Depends on one's exact definition of "harden"... Corrupting memory with >> zeros is less bad than corrupting memory with someone else's data if you >> look at it from an information security point of view, but from a >> not-corrupting-memory point of view it's definitely still corrupting memory >> :/ >> >> Taking a step back, is there not an argument that if people care about >> general KVM performance then they should maybe stop emulating obsolete PC >> hardware from 30 years ago, and at least emulate obsolete PC hardware from >> 20 years ago that supports 64-bit DMA? > > Heh, I don't think there's an argument per se, people most definitely shouldn't > be emulating old hardware if they care about performance. I already told Yan as > much. > >> Even non-virtualised, SWIOTLB is pretty horrible for I/O performance by its >> very nature - avoiding it if at all possible should always be preferred. > > Yeah. The main reason I didn't just sweep this under the rug is the confidential > VM use case, where SWIOTLB is used to bounce data from guest private memory into > shread buffers. There's also a good argument that anyone that cares about I/O > performance in confidential VMs should put in the effort to enlighten their device > drivers to use shared memory directly, but practically speaking that's easier said > than done.
Indeed a bunch of work has gone into SWIOTLB recently trying to make it a bit more efficient for such cases where it can't be avoided, so it is definitely still interesting to learn about impacts at other levels like this. Maybe there's a bit of a get-out for confidential VMs though, since presumably there's not much point COW-ing encrypted private memory, so perhaps KVM might end up wanting to optimise that out and thus happen to end up less sensitive to unavoidable SWIOTLB behaviour anyway?
Cheers, Robin.
| |