| Message ID | 20200325161249.55095-35-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
On Wed, Mar 25, 2020 at 05:12:45PM +0100, glider@google.com wrote: > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index a8560052a915f..63dc1a594964a 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -367,6 +367,7 @@ dma_addr_t dma_direct_map_page(struct device *dev, struct page *page, > &dma_addr, size, *dev->dma_mask, dev->bus_dma_limit); > return DMA_MAPPING_ERROR; > } > + kmsan_handle_dma(page_address(page) + offset, size, dir); This needs to go into dma_map_page so that it also covers IOMMUs. dma_map_sg_atttrs will also need similar treatment. Also the page doesn't have to be mapped into kernel address space, you probably want to pass the page to kmsan_handle_dma and throw in a highmem check there.
On Wed, Mar 25, 2020 at 5:19 PM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Mar 25, 2020 at 05:12:45PM +0100, glider@google.com wrote: > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > > index a8560052a915f..63dc1a594964a 100644 > > --- a/kernel/dma/direct.c > > +++ b/kernel/dma/direct.c > > @@ -367,6 +367,7 @@ dma_addr_t dma_direct_map_page(struct device *dev, struct page *page, > > &dma_addr, size, *dev->dma_mask, dev->bus_dma_limit); > > return DMA_MAPPING_ERROR; > > } > > + kmsan_handle_dma(page_address(page) + offset, size, dir); > > This needs to go into dma_map_page so that it also covers IOMMUs. > dma_map_sg_atttrs will also need similar treatment. Thanks, will be done in v6! > Also the page > doesn't have to be mapped into kernel address space, you probably > want to pass the page to kmsan_handle_dma and throw in a highmem > check there. Do you mean comparing the address to TASK_SIZE, or is there a more portable way to check that?
On Fri, Mar 27, 2020 at 06:03:32PM +0100, Alexander Potapenko wrote: > > Also the page > > doesn't have to be mapped into kernel address space, you probably > > want to pass the page to kmsan_handle_dma and throw in a highmem > > check there. > > Do you mean comparing the address to TASK_SIZE, or is there a more > portable way to check that? !PageHighMem(page) implies the page has a kernel direct mapping.
On Fri, Mar 27, 2020 at 6:06 PM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Mar 27, 2020 at 06:03:32PM +0100, Alexander Potapenko wrote: > > > Also the page > > > doesn't have to be mapped into kernel address space, you probably > > > want to pass the page to kmsan_handle_dma and throw in a highmem > > > check there. > > > > Do you mean comparing the address to TASK_SIZE, or is there a more > > portable way to check that? > > !PageHighMem(page) implies the page has a kernel direct mapping. I tried adding this check and started seeing false positives because the virtio_ring driver actually uses highmem pages for DMA, and data from those pages is later copied to the kernel. Guess it's easier to just allow handling highmem pages? What problems do you anticipate?
On Fri, Mar 27, 2020 at 07:46:08PM +0100, Alexander Potapenko wrote: > > > Do you mean comparing the address to TASK_SIZE, or is there a more > > > portable way to check that? > > > > !PageHighMem(page) implies the page has a kernel direct mapping. > > I tried adding this check and started seeing false positives because > the virtio_ring driver actually uses highmem pages for DMA, and data > from those pages is later copied to the kernel. > Guess it's easier to just allow handling highmem pages? What problems > do you anticipate? For PageHighMem(page), page_address(page) is not actually valid, so І'm not sure how your code in this patch even worked at all. Note that all drivers (well, except for a few buggy legacy ones with workarounds) can DMA from/to highmem.
On Sat, Mar 28, 2020 at 9:52 AM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Mar 27, 2020 at 07:46:08PM +0100, Alexander Potapenko wrote: > > > > Do you mean comparing the address to TASK_SIZE, or is there a more > > > > portable way to check that? > > > > > > !PageHighMem(page) implies the page has a kernel direct mapping. > > > > I tried adding this check and started seeing false positives because > > the virtio_ring driver actually uses highmem pages for DMA, and data > > from those pages is later copied to the kernel. > > Guess it's easier to just allow handling highmem pages? What problems > > do you anticipate? > > For PageHighMem(page), page_address(page) is not actually valid, so І'm > not sure how your code in this patch even worked at all. Note that > all drivers (well, except for a few buggy legacy ones with workarounds) > can DMA from/to highmem. Hm, skipping PageHighMem pages works now, I must've been doing something wrong. Thanks, will add that check to v6. I found a bunch of other places in mm/kmsan/kmsan_shadow.c where we're using page_address(), but metadata pages are not supposed to reside in high memory.
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index a8560052a915f..63dc1a594964a 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -367,6 +367,7 @@ dma_addr_t dma_direct_map_page(struct device *dev, struct page *page, &dma_addr, size, *dev->dma_mask, dev->bus_dma_limit); return DMA_MAPPING_ERROR; } + kmsan_handle_dma(page_address(page) + offset, size, dir); if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) arch_sync_dma_for_device(phys, size, dir);
KMSAN doesn't know about DMA memory writes performed by devices. We unpoison such memory when it's mapped to avoid false positive reports. Signed-off-by: Alexander Potapenko <glider@google.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Vegard Nossum <vegard.nossum@oracle.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Marco Elver <elver@google.com> Cc: Andrey Konovalov <andreyknvl@google.com> Cc: linux-mm@kvack.org --- Change-Id: Ib1019ed531fea69f88b5cdec3d1e27403f2f3d64 --- kernel/dma/direct.c | 1 + 1 file changed, 1 insertion(+)