From: Daniel Vetter <firstname.lastname@example.org> To: Jason Gunthorpe <email@example.com> Cc: firstname.lastname@example.org, linux-samsung-soc <email@example.com>, "Jan Kara" <firstname.lastname@example.org>, "Kees Cook" <email@example.com>, firstname.lastname@example.org, "John Hubbard" <email@example.com>, LKML <firstname.lastname@example.org>, "DRI Development" <email@example.com>, "Linux MM" <firstname.lastname@example.org>, "Jérôme Glisse" <email@example.com>, "Daniel Vetter" <firstname.lastname@example.org>, "Dan Williams" <email@example.com>, "Andrew Morton" <firstname.lastname@example.org>, "Linux ARM" <email@example.com>, "open list:DMA BUFFER SHARING FRAMEWORK" <firstname.lastname@example.org> Subject: Re: [PATCH 11/13] mm: add unsafe_follow_pfn Date: Wed, 7 Oct 2020 21:38:42 +0200 [thread overview] Message-ID: <CAKMK7uHNV7di+J10_JyLY6ctdsAGNnGhF7qrFzRCWFnXsQSEwQ@mail.gmail.com> (raw) In-Reply-To: <20201007190023.GA5177@ziepe.ca> On Wed, Oct 7, 2020 at 9:00 PM Jason Gunthorpe <email@example.com> wrote: > > On Wed, Oct 07, 2020 at 08:10:34PM +0200, Daniel Vetter wrote: > > On Wed, Oct 7, 2020 at 7:36 PM Jason Gunthorpe <firstname.lastname@example.org> wrote: > > > > > > On Wed, Oct 07, 2020 at 06:44:24PM +0200, Daniel Vetter wrote: > > > > Way back it was a reasonable assumptions that iomem mappings never > > > > change the pfn range they point at. But this has changed: > > > > > > > > - gpu drivers dynamically manage their memory nowadays, invalidating > > > > ptes with unmap_mapping_range when buffers get moved > > > > > > > > - contiguous dma allocations have moved from dedicated carvetouts to > > > > cma regions. This means if we miss the unmap the pfn might contain > > > > pagecache or anon memory (well anything allocated with GFP_MOVEABLE) > > > > > > > > - even /dev/mem now invalidates mappings when the kernel requests that > > > > iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 > > > > ("/dev/mem: Revoke mappings when a driver claims the region") > > > > > > > > Accessing pfns obtained from ptes without holding all the locks is > > > > therefore no longer a good idea. > > > > > > > > Unfortunately there's some users where this is not fixable (like v4l > > > > userptr of iomem mappings) or involves a pile of work (vfio type1 > > > > iommu). For now annotate these as unsafe and splat appropriately. > > > > > > > > This patch adds an unsafe_follow_pfn, which later patches will then > > > > roll out to all appropriate places. > > > > > > > > Signed-off-by: Daniel Vetter <email@example.com> > > > > Cc: Jason Gunthorpe <firstname.lastname@example.org> > > > > Cc: Kees Cook <email@example.com> > > > > Cc: Dan Williams <firstname.lastname@example.org> > > > > Cc: Andrew Morton <email@example.com> > > > > Cc: John Hubbard <firstname.lastname@example.org> > > > > Cc: Jérôme Glisse <email@example.com> > > > > Cc: Jan Kara <firstname.lastname@example.org> > > > > Cc: Dan Williams <email@example.com> > > > > Cc: firstname.lastname@example.org > > > > Cc: email@example.com > > > > Cc: firstname.lastname@example.org > > > > Cc: email@example.com > > > > Cc: firstname.lastname@example.org > > > > include/linux/mm.h | 2 ++ > > > > mm/memory.c | 32 +++++++++++++++++++++++++++++++- > > > > mm/nommu.c | 17 +++++++++++++++++ > > > > security/Kconfig | 13 +++++++++++++ > > > > 4 files changed, 63 insertions(+), 1 deletion(-) > > > > > > Makes sense to me. > > > > > > I wonder if we could change the original follow_pfn to require the > > > ptep and then lockdep_assert_held() it against the page table lock? > > > > The safe variant with the pagetable lock is follow_pte_pmd. The only > > way to make follow_pfn safe is if you have an mmu notifier and > > corresponding retry logic. That is not covered by lockdep (it would > > splat if we annotate the retry side), so I'm not sure how you'd check > > for that? > > Right OK. > > > Checking for ptep lock doesn't work here, since the one leftover safe > > user of this (kvm) doesn't need that at all, because it has the mmu > > notifier. > > Ah, so a better name and/or function kdoc for follow_pfn is probably a > good iead in this patch as well. I did change that already to mention that you need an mmu notifier, and that follow_pte_pmd respectively unsafe_follow_pfn are the alternatives. Do you want more or something else here? Note that I left the kerneldoc for the nommu.c case unchanged, since without an mmu all bets are off anyway. > > So I think we're as good as it gets, since I really have no idea how > > to make sure follow_pfn callers do have an mmu notifier registered. > > Yah, can't be done. Most mmu notifier users should be using > hmm_range_fault anyhow, kvm is really very special here. We could pass an mmu notifier to follow_pfn and check that it has a registration for vma->vm_mm, but that feels like overkill when kvm is the only legit user for this. > > I've followed the few other CONFIG_STRICT_FOO I've seen, which are all > > explicit enables and default to "do not break uapi, damn the > > (security) bugs". Which is I think how this should be done. It is in > > the security section though, so hopefully competent distros will > > enable this all. > > I thought the strict ones were more general and less clear security > worries, not bugs like this. > > This is "allow a user triggerable use after free bug to exist in the > kernel" Since at most you get at GFP_MOVEABLE stuff I'm not sure you can use this to pull the kernel over the table. Maybe best way is if you get a gpu pagetable somehow into your pfn and then use that to access abitrary stuff, but there's still an iommu. I think leveraging this is going to be very tricky, and pretty much has to be device or driver specific somehow. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list email@example.com https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-10-07 19:38 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-07 16:44 [PATCH 00/13] follow_pfn and other iomap races Daniel Vetter 2020-10-07 16:44 ` [PATCH 01/13] drm/exynos: Stop using frame_vector helpers Daniel Vetter 2020-10-07 20:32 ` John Hubbard 2020-10-07 21:32 ` Daniel Vetter 2020-10-07 21:36 ` John Hubbard 2020-10-07 21:50 ` Daniel Vetter 2020-10-07 16:44 ` [PATCH 02/13] drm/exynos: Use FOLL_LONGTERM for g2d cmdlists Daniel Vetter 2020-10-07 20:43 ` John Hubbard 2020-10-07 16:44 ` [PATCH 03/13] misc/habana: Stop using frame_vector helpers Daniel Vetter 2020-10-07 20:38 ` John Hubbard 2020-10-07 16:44 ` [PATCH 04/13] misc/habana: Use FOLL_LONGTERM for userptr Daniel Vetter 2020-10-07 20:46 ` John Hubbard 2020-10-07 16:44 ` [PATCH 05/13] mm/frame-vector: Use FOLL_LONGTERM Daniel Vetter 2020-10-07 16:53 ` Jason Gunthorpe 2020-10-07 17:12 ` Daniel Vetter 2020-10-07 17:33 ` Jason Gunthorpe 2020-10-07 21:13 ` John Hubbard 2020-10-07 21:30 ` Daniel Vetter 2020-10-07 16:44 ` [PATCH 06/13] media: videobuf2: Move frame_vector into media subsystem Daniel Vetter 2020-10-07 22:18 ` John Hubbard 2020-10-07 16:44 ` [PATCH 07/13] mm: close race in generic_access_phys Daniel Vetter 2020-10-07 17:27 ` Jason Gunthorpe 2020-10-07 18:01 ` Daniel Vetter 2020-10-07 23:21 ` Jason Gunthorpe 2020-10-08 0:44 ` John Hubbard 2020-10-08 7:23 ` Daniel Vetter 2020-10-07 16:44 ` [PATCH 08/13] s390/pci: Remove races against pte updates Daniel Vetter 2020-10-08 16:44 ` Gerald Schaefer 2020-10-08 17:16 ` Daniel Vetter 2020-10-07 16:44 ` [PATCH 09/13] PCI: obey iomem restrictions for procfs mmap Daniel Vetter 2020-10-07 18:46 ` Bjorn Helgaas 2020-10-07 16:44 ` [PATCH 10/13] PCI: revoke mappings like devmem Daniel Vetter 2020-10-07 18:41 ` Bjorn Helgaas 2020-10-07 19:24 ` Daniel Vetter 2020-10-07 19:33 ` Dan Williams 2020-10-07 19:47 ` Daniel Vetter 2020-10-07 22:23 ` Dan Williams 2020-10-07 22:29 ` Dan Williams 2020-10-08 8:09 ` Daniel Vetter 2020-10-07 23:24 ` Jason Gunthorpe 2020-10-08 7:31 ` Daniel Vetter 2020-10-08 7:49 ` Dan Williams 2020-10-08 8:13 ` Daniel Vetter 2020-10-08 8:35 ` Dan Williams 2020-10-08 12:41 ` Jason Gunthorpe 2020-10-07 16:44 ` [PATCH 11/13] mm: add unsafe_follow_pfn Daniel Vetter 2020-10-07 17:36 ` Jason Gunthorpe 2020-10-07 18:10 ` Daniel Vetter 2020-10-07 19:00 ` Jason Gunthorpe 2020-10-07 19:38 ` Daniel Vetter [this message] 2020-10-07 16:44 ` [PATCH 12/13] media/videbuf1|2: Mark follow_pfn usage as unsafe Daniel Vetter 2020-10-07 16:44 ` [PATCH 13/13] vfio/type1: Mark follow_pfn " Daniel Vetter 2020-10-07 17:39 ` Jason Gunthorpe 2020-10-07 18:14 ` Daniel Vetter 2020-10-07 18:47 ` Jason Gunthorpe
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKMK7uHNV7di+J10_JyLY6ctdsAGNnGhF7qrFzRCWFnXsQSEwQ@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 11/13] mm: add unsafe_follow_pfn' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).