| Message ID | 20220325150420.085364078@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, Mar 25, 2022 at 8:09 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > From: Halil Pasic <pasic@linux.ibm.com> > > 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
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 > <gregkh@linuxfoundation.org> wrote: > > > > From: Halil Pasic <pasic@linux.ibm.com> > > > > 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 <gregkh@linuxfoundation.org> 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 Sat, Mar 26, 2022 at 11:41:17AM -0700, Linus Torvalds wrote: > On Sat, Mar 26, 2022 at 3:18 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> 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
On Sun, Mar 27, 2022 at 2:30 AM Greg Kroah-Hartman <gregkh@linuxfoundation.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
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 > <gregkh@linuxfoundation.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. 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
--- 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();