From: David Hildenbrand <david@redhat.com>
To: Dan Williams <dan.j.williams@intel.com>,
Bjorn Helgaas <helgaas@kernel.org>
Cc: "Greg KH" <gregkh@linuxfoundation.org>,
"Arnd Bergmann" <arnd@arndb.de>, "Ingo Molnar" <mingo@redhat.com>,
"Kees Cook" <keescook@chromium.org>,
"Matthew Wilcox" <willy@infradead.org>,
"Russell King" <linux@arm.linux.org.uk>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Linux MM" <linux-mm@kvack.org>,
"Linux PCI" <linux-pci@vger.kernel.org>,
"Daniel Vetter" <daniel.vetter@ffwll.ch>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Jason Gunthorpe" <jgg@nvidia.com>,
"Christoph Hellwig" <hch@lst.de>
Subject: Re: [PATCH v4] /dev/mem: Revoke mappings when a driver claims the region
Date: Fri, 28 May 2021 10:58:01 +0200 [thread overview]
Message-ID: <7eaf4c10-292e-18d7-e8ce-3a6b72122381@redhat.com> (raw)
In-Reply-To: <CAPcyv4j-ygPddjuZqq8PMvsN-E8rJQszc+WuUu1MBXVXiQZddg@mail.gmail.com>
On 27.05.21 23:30, Dan Williams wrote:
> On Thu, May 27, 2021 at 1:58 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> [+cc Daniel, Krzysztof, Jason, Christoph, linux-pci]
>>
>> On Thu, May 21, 2020 at 02:06:17PM -0700, Dan Williams wrote:
>>> Close the hole of holding a mapping over kernel driver takeover event of
>>> a given address range.
>>>
>>> Commit 90a545e98126 ("restrict /dev/mem to idle io memory ranges")
>>> introduced CONFIG_IO_STRICT_DEVMEM with the goal of protecting the
>>> kernel against scenarios where a /dev/mem user tramples memory that a
>>> kernel driver owns. However, this protection only prevents *new* read(),
>>> write() and mmap() requests. Established mappings prior to the driver
>>> calling request_mem_region() are left alone.
>>>
>>> Especially with persistent memory, and the core kernel metadata that is
>>> stored there, there are plentiful scenarios for a /dev/mem user to
>>> violate the expectations of the driver and cause amplified damage.
>>>
>>> Teach request_mem_region() to find and shoot down active /dev/mem
>>> mappings that it believes it has successfully claimed for the exclusive
>>> use of the driver. Effectively a driver call to request_mem_region()
>>> becomes a hole-punch on the /dev/mem device.
>>
>> This idea of hole-punching /dev/mem has since been extended to PCI
>> BARs via [1].
>>
>> Correct me if I'm wrong: I think this means that if a user process has
>> mmapped a PCI BAR via sysfs, and a kernel driver subsequently requests
>> that region via pci_request_region() or similar, we punch holes in the
>> the user process mmap. The driver might be happy, but my guess is the
>> user starts seeing segmentation violations for no obvious reason and
>> is not happy.
>>
>> Apart from the user process issue, the implementation of [1] is
>> problematic for PCI because the mmappable sysfs attributes now depend
>> on iomem_init_inode(), an fs_initcall, which means they can't be
>> static attributes, which ultimately leads to races in creating them.
>
> See the comments in iomem_get_mapping(), and revoke_iomem():
>
> /*
> * Check that the initialization has completed. Losing the race
> * is ok because it means drivers are claiming resources before
> * the fs_initcall level of init and prevent iomem_get_mapping users
> * from establishing mappings.
> */
>
> ...the observation being that it is ok for the revocation inode to
> come on later in the boot process because userspace won't be able to
> use the fs yet. So any missed calls to revoke_iomem() would fall back
> to userspace just seeing the resource busy in the first instance. I.e.
> through the normal devmem_is_allowed() exclusion.
>
>>
>> So I'm raising the question of whether this hole-punch is the right
>> strategy.
>>
>> - Prior to revoke_iomem(), __request_region() was very
>> self-contained and really only depended on the resource tree. Now
>> it depends on a lot of higher-level MM machinery to shoot down
>> mappings of other tasks. This adds quite a bit of complexity and
>> some new ordering constraints.
>>
>> - Punching holes in the address space of an existing process seems
>> unfriendly. Maybe the driver's __request_region() should fail
>> instead, since the driver should be prepared to handle failure
>> there anyway.
>
> It's prepared to handle failure, but in this case it is dealing with a
> root user of 2 minds.
>
>>
>> - [2] suggests that the hole punch protects drivers from /dev/mem
>> writers, especially with persistent memory. I'm not really
>> convinced. The hole punch does nothing to prevent a user process
>> from mmapping and corrupting something before the driver loads.
>
> The motivation for this was a case that was swapping between /dev/mem
> access and /dev/pmem0 access and they forgot to stop using /dev/mem
> when they switched to /dev/pmem0. If root wants to use /dev/mem it can
> use it, if root wants to stop the driver from loading it can set
> mopdrobe policy or manually unbind, and if root asks the kernel to
> load the driver while it is actively using /dev/mem something has to
> give. Given root has other options to stop a driver the decision to
> revoke userspace access when root messes up and causes a collision
> seems prudent to me.
>
Is there a real use case for mapping pmem via /dev/mem or could we just
prohibit the access to these areas completely?
What's the use case for "swapping between /dev/mem access and /dev/pmem0
access" ?
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2021-05-28 8:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-21 21:06 [PATCH v4] /dev/mem: Revoke mappings when a driver claims the region Dan Williams
2020-05-22 3:01 ` Kees Cook
2022-04-06 19:45 ` Kees Cook
2022-04-07 18:47 ` Dan Williams
2022-04-07 23:43 ` Dan Williams
2022-04-08 3:35 ` Kees Cook
2022-04-08 6:51 ` Dan Williams
2022-04-07 23:46 ` Kees Cook
2021-05-27 20:58 ` Bjorn Helgaas
2021-05-27 21:30 ` Dan Williams
2021-05-28 8:58 ` David Hildenbrand [this message]
2021-05-28 16:42 ` Dan Williams
2021-05-28 16:51 ` David Hildenbrand
2021-06-03 3:39 ` Bjorn Helgaas
2021-06-03 4:15 ` Dan Williams
2021-06-03 18:11 ` Bjorn Helgaas
2021-06-03 18:28 ` Dan Williams
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=7eaf4c10-292e-18d7-e8ce-3a6b72122381@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=dan.j.williams@intel.com \
--cc=daniel.vetter@ffwll.ch \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=helgaas@kernel.org \
--cc=jgg@nvidia.com \
--cc=keescook@chromium.org \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=mingo@redhat.com \
--cc=willy@infradead.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).