linux-kernel - Re: [PATCH] iommu/dma: Respect SWIOTLB force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 2 May 2024 09:02:23 -0700
From: "T.J. Mercier" <tjmercier@...gle.com>
To: Robin Murphy <robin.murphy@....com>
Cc: Christoph Hellwig <hch@...radead.org>, Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Catalin Marinas <catalin.marinas@....com>, 
	isaacmanjarres@...gle.com, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/dma: Respect SWIOTLB force_bounce

On Thu, May 2, 2024 at 5:50 AM Robin Murphy <robin.murphy@....com> wrote:
>
> On 02/05/2024 6:07 am, Christoph Hellwig wrote:
> > On Wed, May 01, 2024 at 08:13:18PM +0000, T.J. Mercier wrote:
> >> iommu_dma_map_page and iommu_dma_map_sg conditionally use SWIOTLB, but
> >> checking if force_bounce is set for the device is not part of that
> >> condition. Check if devices have requested to force SWIOTLB use as part
> >> of deciding to take the existing SWIOTLB paths.
> >
> > This fails to explain why you'd want this somewhat surprising behavior,
> > and why you consider it a bug fix.
>
> Indeed, it's rather intentional that the "swiotlb=force" argument
> doesn't affect iommu-dma, since that's primarily for weeding out drivers
> making dodgy assumptions about DMA addresses, and iommu-dma is
> inherently even better at that already.
>
> Beyond that I think this change also seems likely to interact badly with
> CC_ATTR_GUEST_MEM_ENCRYPT on x86, where we invoke the SWIOTLB_FORCE flag
> for dma-direct, but expect that an IOMMU can provide a decrypted view
> in-place, thus bouncing in that path would be unnecessarily detrimental.
>
> Thanks,
> Robin.

I encountered this while testing a change to DMA direct which makes
sure that sg_dma_mark_swiotlb is called there like it is here. (Right
now the SG_DMA_SWIOTLB flag is set only if dma_map_sgtable takes the
IOMMU path, but not if SWIOTLB is used on the direct path.) While I
agree IOMMU + force_bounce is an unusual config, I found it equally
surprising that swiotlb=force wasn't doing what is advertised, or even
giving a warning/error. Since the iommu-dma code is already set up for
conditionally bouncing through SWIOTLB, it looked straightforward to
give what's asked for in the case of swiotlb=force. If it's
intentional that SWIOTLB options don't affect IOMMU code, then should
we just warn about it here when it's ignored? The presence of a
warning like that would also be a suggestion of, "you probably don't
actually want what you're asking for with this configuration you've
specified".

Thanks,
T.J.