From: Halil Pasic <[email protected]>
commit aa6f8dcbab473f3a3c7454b74caa46d36cdc5d13 upstream.
Unfortunately, we ended up merging an old version of the patch "fix info
leak with DMA_FROM_DEVICE" instead of merging the latest one. Christoph
(the swiotlb maintainer), he asked me to create an incremental fix
(after I have pointed this out the mix up, and asked him for guidance).
So here we go.
The main differences between what we got and what was agreed are:
* swiotlb_sync_single_for_device is also required to do an extra bounce
* We decided not to introduce DMA_ATTR_OVERWRITE until we have exploiters
* The implantation of DMA_ATTR_OVERWRITE is flawed: DMA_ATTR_OVERWRITE
must take precedence over DMA_ATTR_SKIP_CPU_SYNC
Thus this patch removes DMA_ATTR_OVERWRITE, and makes
swiotlb_sync_single_for_device() bounce unconditionally (that is, also
when dir == DMA_TO_DEVICE) in order do avoid synchronising back stale
data from the swiotlb buffer.
Let me note, that if the size used with dma_sync_* API is less than the
size used with dma_[un]map_*, under certain circumstances we may still
end up with swiotlb not being transparent. In that sense, this is no
perfect fix either.
To get this bullet proof, we would have to bounce the entire
mapping/bounce buffer. For that we would have to figure out the starting
address, and the size of the mapping in
swiotlb_sync_single_for_device(). While this does seem possible, there
seems to be no firm consensus on how things are supposed to work.
Signed-off-by: Halil Pasic <[email protected]>
Fixes: ddbd89deb7d3 ("swiotlb: fix info leak with DMA_FROM_DEVICE")
Cc: [email protected]
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
Documentation/core-api/dma-attributes.rst | 8 --------
include/linux/dma-mapping.h | 8 --------
kernel/dma/swiotlb.c | 25 ++++++++++++++++---------
3 files changed, 16 insertions(+), 25 deletions(-)
--- a/Documentation/core-api/dma-attributes.rst
+++ b/Documentation/core-api/dma-attributes.rst
@@ -130,11 +130,3 @@ accesses to DMA buffers in both privileg
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).
-
-DMA_ATTR_OVERWRITE
-------------------
-
-This is a hint to the DMA-mapping subsystem that the device is expected to
-overwrite the entire mapped size, thus the caller does not require any of the
-previous buffer contents to be preserved. This allows bounce-buffering
-implementations to optimise DMA_FROM_DEVICE transfers.
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -62,14 +62,6 @@
#define DMA_ATTR_PRIVILEGED (1UL << 9)
/*
- * This is a hint to the DMA-mapping subsystem that the device is expected
- * to overwrite the entire mapped size, thus the caller does not require any
- * of the previous buffer contents to be preserved. This allows
- * bounce-buffering implementations to optimise DMA_FROM_DEVICE transfers.
- */
-#define DMA_ATTR_OVERWRITE (1UL << 10)
-
-/*
* A dma_addr_t can hold any valid DMA or bus address for the platform. It can
* be given to a device to use as a DMA source or target. It is specific to a
* given device and there may be a translation between the CPU physical address
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -597,10 +597,14 @@ phys_addr_t swiotlb_tbl_map_single(struc
io_tlb_orig_addr[index + i] = slot_addr(orig_addr, i);
tlb_addr = slot_addr(io_tlb_start, index) + offset;
- if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
- (!(attrs & DMA_ATTR_OVERWRITE) || dir == DMA_TO_DEVICE ||
- dir == DMA_BIDIRECTIONAL))
- swiotlb_bounce(orig_addr, tlb_addr, mapping_size, DMA_TO_DEVICE);
+ /*
+ * When dir == DMA_FROM_DEVICE we could omit the copy from the orig
+ * to the tlb buffer, if we knew for sure the device will
+ * overwirte the entire current content. But we don't. Thus
+ * unconditional bounce may prevent leaking swiotlb content (i.e.
+ * kernel memory) to user-space.
+ */
+ swiotlb_bounce(orig_addr, tlb_addr, mapping_size, DMA_TO_DEVICE);
return tlb_addr;
}
@@ -680,11 +684,14 @@ void swiotlb_tbl_sync_single(struct devi
BUG_ON(dir != DMA_TO_DEVICE);
break;
case SYNC_FOR_DEVICE:
- if (likely(dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL))
- swiotlb_bounce(orig_addr, tlb_addr,
- size, DMA_TO_DEVICE);
- else
- BUG_ON(dir != DMA_FROM_DEVICE);
+ /*
+ * Unconditional bounce is necessary to avoid corruption on
+ * sync_*_for_cpu or dma_ummap_* when the device didn't
+ * overwrite the whole lengt of the bounce buffer.
+ */
+ swiotlb_bounce(orig_addr, tlb_addr,
+ size, DMA_TO_DEVICE);
+ BUG_ON(!valid_dma_direction(dir));
break;
default:
BUG();
On Fri, Mar 25, 2022 at 8:09 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> From: Halil Pasic <[email protected]>
>
> commit aa6f8dcbab473f3a3c7454b74caa46d36cdc5d13 upstream.
This one causes a regression on at least some wireless drivers
(ath9k). There's some active discussion about it, maybe it gets
reverted, maybe there are other options.
There's an ath9k patch that fixes the problem, so if this patch goes
into the stable tree the ath9k fix will follow.
But it might be a good idea to simply hold off on this patch a bit,
because that ath9k patch hasn't actually landed yet, there's some
discussion about it all, and it's not clear that other drivers might
not have the same issue.
So I'm not NAK'ing this patch from stable, but I also don't think it's
timing-critical, and it might be a good idea to delay it for a week or
two to both wait for the ath9k patch and to see if something else
comes up.
Linus
Den 2022-03-25 kl. 19:08, skrev Linus Torvalds:
> On Fri, Mar 25, 2022 at 8:09 AM Greg Kroah-Hartman
> <[email protected]> wrote:
>>
>> From: Halil Pasic <[email protected]>
>>
>> commit aa6f8dcbab473f3a3c7454b74caa46d36cdc5d13 upstream.
>
> This one causes a regression on at least some wireless drivers
> (ath9k). There's some active discussion about it, maybe it gets
> reverted, maybe there are other options.
>
> There's an ath9k patch that fixes the problem, so if this patch goes
> into the stable tree the ath9k fix will follow.
>
> But it might be a good idea to simply hold off on this patch a bit,
> because that ath9k patch hasn't actually landed yet, there's some
> discussion about it all, and it's not clear that other drivers might
> not have the same issue.
>
> So I'm not NAK'ing this patch from stable, but I also don't think it's
> timing-critical, and it might be a good idea to delay it for a week or
> two to both wait for the ath9k patch and to see if something else
> comes up.
>
> Linus
>
As it has already landed in 5.15.29 and 5.16.15 it should IMHO
preferably be reverted in those trees too to get them back to working
state...
--
Thomas
On Fri, Mar 25, 2022 at 10:08:27AM -0700, Linus Torvalds wrote:
> On Fri, Mar 25, 2022 at 8:09 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > From: Halil Pasic <[email protected]>
> >
> > commit aa6f8dcbab473f3a3c7454b74caa46d36cdc5d13 upstream.
>
> This one causes a regression on at least some wireless drivers
> (ath9k). There's some active discussion about it, maybe it gets
> reverted, maybe there are other options.
>
> There's an ath9k patch that fixes the problem, so if this patch goes
> into the stable tree the ath9k fix will follow.
>
> But it might be a good idea to simply hold off on this patch a bit,
> because that ath9k patch hasn't actually landed yet, there's some
> discussion about it all, and it's not clear that other drivers might
> not have the same issue.
>
> So I'm not NAK'ing this patch from stable, but I also don't think it's
> timing-critical, and it might be a good idea to delay it for a week or
> two to both wait for the ath9k patch and to see if something else
> comes up.
Yes, I've been watching that thread. This change is already in 5.15 and
5.16 kernels, and does solve one known security issue, so it's a tough
call. I'll drop them from 5.10 and 5.4 for now and save them to see how
it plays out...
thanks,
greg k-h
On Sat, Mar 26, 2022 at 3:18 AM Greg Kroah-Hartman
<[email protected]> wrote:"
>
> Yes, I've been watching that thread. This change is already in 5.15 and
> 5.16 kernels, and does solve one known security issue, so it's a tough
> call.
If you're following that thread, you'll have seen that I've reverted
it, and I actually think the security argument was bogus - the whole
commit was due to a misunderstanding of the actual direction of the
data transfer.
But hey, maybe I'm wrong. The only truly uncontested fact is that it
broke the ath9k driver.
Linus
On Sun, Mar 27, 2022 at 2:30 AM Greg Kroah-Hartman
<[email protected]> 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
On Sun, Mar 27, 2022 at 11:02:12AM -0700, Linus Torvalds wrote:
> On Sun, Mar 27, 2022 at 2:30 AM Greg Kroah-Hartman
> <[email protected]> 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.
Ok, I have just queued that one up now for the older kernels, and the
revert for 5.15 and newer, thanks.
greg k-h
On Sat, Mar 26, 2022 at 11:41:17AM -0700, Linus Torvalds wrote:
> On Sat, Mar 26, 2022 at 3:18 AM Greg Kroah-Hartman
> <[email protected]> wrote:"
> >
> > Yes, I've been watching that thread. This change is already in 5.15 and
> > 5.16 kernels, and does solve one known security issue, so it's a tough
> > call.
>
> If you're following that thread, you'll have seen that I've reverted
> it, and I actually think the security argument was bogus - the whole
> commit was due to a misunderstanding of the actual direction of the
> data transfer.
I see that now, thanks.
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?
I'm going to drop both from the 5.4 and 5.10 stable queues now, and add
your revert, but I think your tree also needs the original swiotlb fix
commit reverted to get back to a "known good" state.
thanks,
greg k-h