Message-ID: <a501219c-f75a-4467-fefe-bd571e84f99e@redhat.com>
Date:   Fri, 28 Apr 2023 17:33:17 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jason Gunthorpe <jgg@...pe.ca>, Jens Axboe <axboe@...nel.dk>,
        Matthew Wilcox <willy@...radead.org>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Leon Romanovsky <leon@...nel.org>,
        Christian Benvenuti <benve@...co.com>,
        Nelson Escobar <neescoba@...co.com>,
        Bernard Metzler <bmt@...ich.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Bjorn Topel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Christian Brauner <brauner@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        Oleg Nesterov <oleg@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        John Hubbard <jhubbard@...dia.com>, Jan Kara <jack@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Pavel Begunkov <asml.silence@...il.com>,
        Mika Penttila <mpenttil@...hat.com>,
        David Howells <dhowells@...hat.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings
 by default

On 28.04.23 17:24, Lorenzo Stoakes wrote:
> On Fri, Apr 28, 2023 at 05:13:07PM +0200, David Hildenbrand wrote:
>> [...]
>>
>>>> This change has the potential to break existing setups. Simple example:
>>>> libvirt domains configured for file-backed VM memory that also has a vfio
>>>> device configured. It can easily be configured by users (evolving VM
>>>> configuration, copy-paste etc.). And it works from a VM perspective, because
>>>> the guest memory is essentially stale once the VM is shutdown and the pages
>>>> were unpinned. At least we're not concerned about stale data on disk.
>>>>
>>>> With your changes, such VMs would no longer start, breaking existing user
>>>> setups with a kernel update.
>>>
>>> Which vfio vm_ops are we talking about? vfio_pci_mmap_ops for example
>>> doesn't specify page_mkwrite or pfn_mkwrite. Unless you mean some arbitrary
>>> file system in the guest?
>>
>> Sorry, you define a VM to have its memory backed by VM memory and, at the
>> same time, define a vfio-pci device for your VM, which will end up long-term
>> pinning the VM memory.
> 

"memory backed by file memory", I guess you figured that out :)

> Ah ack. Jason seemed concerned that this was already a broken case, I guess
> that's one for you two to hash out...
> 
>>
>>>
>>> I may well be missing context on this so forgive me if I'm being a little
>>> dumb here, but it'd be good to get a specific example.
>>
>> I was giving to little details ;)
>>
>> [...]
>>
>>>>
>>>> I know, Jason und John will disagree, but I don't think we want to be very
>>>> careful with changing the default.
>>>>
>>>> Sure, we could warn, or convert individual users using a flag (io_uring).
>>>> But maybe we should invest more energy on a fix?
>>>
>>> This is proactively blocking a cleanup (eliminating vmas) that I believe
>>> will be useful in moving things forward. I am not against an opt-in option
>>> (I have been responding to community feedback in adapting my approach),
>>> which is the way I implemented it all the way back then :)
>>
>> There are alternatives: just use a flag as Jason initially suggested and use
>> that in io_uring code. Then, you can also bail out on the GUP-fast path as
>> "cannot support it right now, never do GUP-fast".
> 
> I already implemented the alternatives (look back through revisions to see :)
> and there were objections for various reasons.
> 
> Personally my preference is to provide a FOLL_SAFE_FILE_WRITE flag or such and
> replace the FOLL_LONGTERM check with this (that'll automatically get rejected
> for GUP-fast so the GUP-fast conundrum wouldn't be a thing either).
> 
> GUP-fast is a problem as you say,, but it feels like a fundamental issue with
> GUP-fast as a whole since you don't get to look at a VMA since you can't take
> the mmap_lock. You could just look at the folio->mapping once you've walked the
> page tables and say 'I'm out' if FOLL_WRITE and it's non-anon if that's what
> you're suggesting?

See my other reply, kind-of yes. Like we do with gup_must_unshare(). I'm 
only concerned about how to keep GUP-fast working on hugetlb and shmem.

> 
> I'm not against that change but this being incremental, I think that would be a
> further increment.

If we want to fix a security issue, as Jason said, incremental is IMHO 
the wrong approach.

It's often too tempting to ignore the hard part and fix the easy part, 
making the hard part an increment for the future that nobody will really 
implement ... because it's hard.
[...]

>>>
>>> But given we know this is both entirely broken and a potential security
>>> issue, and FOLL_LONGTERM is about as egregious as you can get (user
>>> explicitly saying they'll hold write access indefinitely) I feel it is an
>>> important improvement and makes clear that this is not an acceptable usage.
>>>
>>> I see Jason has said more on this also :)
>>>
>>>>
>>>>
>>>>
>>>>
>>>>> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
>>>>> Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
>>>>> ---
>>>>>     include/linux/mm.h |  1 +
>>>>>     mm/gup.c           | 41 ++++++++++++++++++++++++++++++++++++++++-
>>>>>     mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
>>>>>     3 files changed, 68 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>> index 37554b08bb28..f7da02fc89c6 100644
>>>>> --- a/include/linux/mm.h
>>>>> +++ b/include/linux/mm.h
>>>>> @@ -2433,6 +2433,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
>>>>>     #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
>>>>>     					    MM_CP_UFFD_WP_RESOLVE)
>>>>>
>>>>> +bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
>>>>>     int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
>>>>>     static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
>>>>>     {
>>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>>> index 1f72a717232b..d36a5db9feb1 100644
>>>>> --- a/mm/gup.c
>>>>> +++ b/mm/gup.c
>>>>> @@ -959,16 +959,51 @@ static int faultin_page(struct vm_area_struct *vma,
>>>>>     	return 0;
>>>>>     }
>>>>>
>>>>> +/*
>>>>> + * Writing to file-backed mappings which require folio dirty tracking using GUP
>>>>> + * is a fundamentally broken operation, as kernel write access to GUP mappings
>>>>> + * do not adhere to the semantics expected by a file system.
>>>>> + *
>>>>> + * Consider the following scenario:-
>>>>> + *
>>>>> + * 1. A folio is written to via GUP which write-faults the memory, notifying
>>>>> + *    the file system and dirtying the folio.
>>>>> + * 2. Later, writeback is triggered, resulting in the folio being cleaned and
>>>>> + *    the PTE being marked read-only.
>>>>> + * 3. The GUP caller writes to the folio, as it is mapped read/write via the
>>>>> + *    direct mapping.
>>>>> + * 4. The GUP caller, now done with the page, unpins it and sets it dirty
>>>>> + *    (though it does not have to).
>>>>> + *
>>>>> + * This results in both data being written to a folio without writenotify, and
>>>>> + * the folio being dirtied unexpectedly (if the caller decides to do so).
>>>>> + */
>>>>> +static bool writeable_file_mapping_allowed(struct vm_area_struct *vma,
>>>>> +					   unsigned long gup_flags)
>>>>> +{
>>>>> +	/* If we aren't pinning then no problematic write can occur. */
>>>>> +	if (!(gup_flags & (FOLL_GET | FOLL_PIN)))
>>>>> +		return true;
>>>>
>>>> FOLL_LONGTERM only applies to FOLL_PIN. This check can be dropped.
>>>
>>> I understand that of course (well maybe not of course, but I mean I do, I
>>> have oodles of diagrams referencing this int he book :) This is intended to
>>> document the fact that the check isn't relevant if we don't pin at all,
>>> e.g. reading this you see:-
>>>
>>> - (implicit) if not writing or anon we're good
>>> - if not pin we're good
>>> - ok we are only currently checking one especially egregious case
>>> - finally, perform the dirty tracking check.
>>>
>>> So this is intentional.
>>>
>>>>
>>>>> +
>>>>> +	/* We limit this check to the most egregious case - a long term pin. */
>>>>> +	if (!(gup_flags & FOLL_LONGTERM))
>>>>> +		return true;
>>>>> +
>>>>> +	/* If the VMA requires dirty tracking then GUP will be problematic. */
>>>>> +	return vma_needs_dirty_tracking(vma);
>>>>> +}
>>>>> +
>>>>>     static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>>>>>     {
>>>>>     	vm_flags_t vm_flags = vma->vm_flags;
>>>>>     	int write = (gup_flags & FOLL_WRITE);
>>>>>     	int foreign = (gup_flags & FOLL_REMOTE);
>>>>> +	bool vma_anon = vma_is_anonymous(vma);
>>>>>
>>>>>     	if (vm_flags & (VM_IO | VM_PFNMAP))
>>>>>     		return -EFAULT;
>>>>>
>>>>> -	if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma))
>>>>> +	if ((gup_flags & FOLL_ANON) && !vma_anon)
>>>>>     		return -EFAULT;
>>>>>
>>>>>     	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
>>>>> @@ -978,6 +1013,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>>>>>     		return -EFAULT;
>>>>>
>>>>>     	if (write) {
>>>>> +		if (!vma_anon &&
>>>>> +		    !writeable_file_mapping_allowed(vma, gup_flags))
>>>>> +			return -EFAULT;
>>>>> +
>>>>>     		if (!(vm_flags & VM_WRITE)) {
>>>>>     			if (!(gup_flags & FOLL_FORCE))
>>>>>     				return -EFAULT;
>>>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>>>> index 536bbb8fa0ae..7b6344d1832a 100644
>>>>> --- a/mm/mmap.c
>>>>
>>>>
>>>> I'm probably missing something, why don't we have to handle GUP-fast (having
>>>> said that, it's hard to handle ;) )? The sequence you describe above should
>>>> apply to GUP-fast as well, no?
>>>>
>>>> 1) Pin writable mapped page using GUP-fast
>>>> 2) Trigger writeback
>>>> 3) Write to page via pin
>>>> 4) Unpin and set dirty
>>>
>>> You're right, and this is an excellent point. I worry about other GUP use
>>> cases too, but we're a bit out of luck there because we don't get to check
>>> the VMA _at all_ (which opens yet another Pandora's box about how safe it
>>> is to do unlocked pinning :)
>>>
>>> But again, this comes down to the fact we're trying to make things
>>> _incrementally__ better rather than throwing our hands up and saying one
>>> day my ship will come in...
>>
>> That's not how security fixes are supposed to work IMHO, sorry.
> 
> Sure, but I don't think the 'let things continue to be terribly broken for X
> more years' is also a great approach.

Not at all, people (including me) were simply not aware that it is that 
much of an (security) issue because I never saw any real bug reports (or 
CVE numbers) and only herd John talk about possible fixes a year ago :)

So I'm saying we either try to block it completely or finally look into 
fixing it for good. I'm not a friend of anything in between.

You don't gain a lot of security by locking the front door but knowingly 
leaving the back door unlocked.

> 
> Personally I come at this from the 'I just want my vmas patch series' unblocked
> perspective :) and feel there's a functional aspect here too.

I know, it always gets messy when touching such sensible topics :P

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists