linux-kernel - Re: [PATCH 5.10 11/38] swiotlb: rework "fix info leak with DMA_FROM

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]

Message-ID: <CAHk-=wgzSHQqz33i0DfmFyEG43eeyDkkUO=a3jY0eH2h_1AwgA@mail.gmail.com>
Date:   Sun, 27 Mar 2022 11:02:12 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 5.10 11/38] swiotlb: rework "fix info leak with DMA_FROM_DEVICE"

On Sun, Mar 27, 2022 at 2:30 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> But why did you just revert that commit, and not the previous one (i.e.
> the one that this one "fixes")?  Shouldn't ddbd89deb7d3 ("swiotlb: fix
> info leak with DMA_FROM_DEVICE") also be dropped?

The previous one wasn't obviously broken, and while it's a bit ugly,
it doesn't have the fundamental issues that the "fix" commit had.

And it does fix the whole "bounce buffer contents are undefined, and
can get copied back later" at the bounce buffer allocation (well,
"mapping") stage.

Which could cause wasted CPU cycles and isn't great, but should fix
the stale content thing for at least the common "map DMA, do DMA,
unmap" situation.

What commit aa6f8dcbab47 tried to fix was the "do multiple DMA
sequences using one single mapping" case, but that's also what then
broke ath9k because it really does want to do exactly that, but it
very much needs to do it using the same buffer with no "let's reset
it".

So I think you're fine to drop ddbd89deb7d3 too, but that commit
doesn't seem *wrong* per se.

I do think we need some model for "clear the bounce buffer of stale
data", and I do think that commit ddbd89deb7d3 probably isn't the
final word, but we don't actually _have_ the final word on this all,
so stable dropping it all is sane.

But as mentioned, commit ddbd89deb7d3 can actually fix some cases.

In particular, I do think it fixes the SG_IO data leak case that
triggered the whole issue. It was just then the "let's expand on this
fix" that was a disaster.

                  Linus