Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 9 May 2023 09:18:14 +0800
From:   Ruihan Li <lrh2000@pku.edu.cn>
To:     Pasha Tatashin <pasha.tatashin@soleen.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        syzbot+fcf1a817ceb50935ce99@syzkaller.appspotmail.com,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        Ruihan Li <lrh2000@pku.edu.cn>
Subject: Re: usbdev_mmap causes type confusion in page_table_check
Message-ID: <cyexilogx5xvdne6f2uoxbvir7i5mqykpzbbg27x4l2qnlxksg@f5xqssrqanzq>
References: <CA+CK2bBe2YKYM3rUTCnZ0RF=NFUR9VqO-QYn3ygPsFJWLY1MUA@mail.gmail.com>
 <ZFlrbDft1QfMyIDc@casper.infradead.org>
 <CA+CK2bDVjovwB9v-Zv4Fn7EUfp5FV2XK36iJKYKY7pYNOFfOGA@mail.gmail.com>
 <ZFlvJEfs1ufh1UUD@casper.infradead.org>
 <CA+CK2bDC-FVv1tZg9MDn-N735Ak3OAtdZPf+LEYM-JHsO90YcQ@mail.gmail.com>
 <fa1dac7a-406e-30ea-6aba-ded2e0e871fa@redhat.com>
 <CA+CK2bAHbHHwLUoGJkz8n6mrM5dy7oMojeNksdVOMYn+qFYngA@mail.gmail.com>
 <CA+CK2bD=bv1vPGZaNcxDy-uUFj2ZAKkdPmAtJaweXhgTe91oEw@mail.gmail.com>
 <366ab078-1101-421c-691d-34f5efe006b5@redhat.com>
 <CA+CK2bBwYGd2_CVQ4Nr87CGEjp7BqVbBeoP++yHB7Cr9_peBOQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+CK2bBwYGd2_CVQ4Nr87CGEjp7BqVbBeoP++yHB7Cr9_peBOQ@mail.gmail.com>
Precedence: bulk

Thanks for the discussion!

On Mon, May 08, 2023 at 05:07:26PM -0700, Pasha Tatashin wrote:
> On Mon, May 8, 2023 at 4:37 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 09.05.23 01:21, Pasha Tatashin wrote:
> > >> For normal Kernel-MM operations, vm_normal_page() should be used to
> > >> get "struct page" based on vma+addr+pte combination, but
> > >> page_table_check does not use vma for its operation in order to
> > >> strengthen the verification of no invalid page sharing. But, even
> >
> > I'm not sure if that's the right approach for this case here, though.
> >
> > >> vm_normal_page() can cause access to the "struct page" for VM_PFNMAP
> > >> if pfn_valid(pfn) is true. So, vm_normal_page() can return a struct
> > >> page for a user mapped slab page.
> > >
> > > Only for !ARCH_HAS_PTE_SPECIAL case, otherwise NULL is returned.
> >
> > That would violate VM_PFNMAP semantics, though. I remember that there
> > was a trick to it.
> >
> > Assuming we map /dev/mem, what stops a page we mapped and determined to
> > be !anon to be freed and reused, such that we suddenly have an anon page
> > mappped?
> >
> > In that case, we really don't want to look at the "struct page" ever, no?
> 
> Good point. page_table_check just does not work well /dev/mem. I am
> thinking of adding BUG_ON(PageSlab(page); and also "depends on
> !DEVMEM" for the PAGE_TABLE_CHECK config option.
> 
> Pasha

Agreed. I have previously thought about using something like
	anon = !PageSlab(page) && PageAnon(page);
or
	BUG(PageSlab(page))
in page_table_check_*. But this just does not work well with /dev/mem,
and cannot be made to work unless something like VM_PFNMAP is involved.
The reason is that both PageSlab(page) and PageAnon(page) can evaluate
to different values when mapping and unmapping the same page, since the
page number can be totally arbitrary.

But if we do not want /dev/mem to work with PAGE_TABLE_CHECK, this is
what I doubted and Pasha confirmed earlier,

On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> On Sun, May 7, 2023 at 9:58 AM Ruihan Li <lrh2000@pku.edu.cn> wrote:
> > This does not actually fix the type confusion bug in page_table_check_*. It
> > should be noted that by leveraging /dev/mem, users can map arbitrary physical
> > memory regions into the user space, which is also achieved through
> > remap_pfn_range. I'm not 100% certain whether a fix is necessary, as one may
> > argue that using page table checks (a kernel hardening technique, which means
> > security is important) and /dev/mem (clearly insecure and potentially
> 
> Yes, /dev/mem device is a security problem, and would not work with a
> hardern kernel.
> 
> > exploitable) together is completely unreasonable.

we should enforce this explicitly (and perhaps document it explicitly as
well), and the exact way to do this is to make the PAGE_TABLE_CHECK
config option dependent on !DEVMEM.

This also implies that we must fix hcd_buffer_alloc on the usb side as
well. I am also not sure if there are other uses of remap_pfn_range on
slab pages (as David asked earlier) besides usbfs and /dev/mem. Maybe
the calls to remap_pfn_range (and other functions that can map the
userspace memory?) need to be manually checked one by one. But later I
can first try this approach at least, and see if anything goes wrong
(though unlikely).

Thanks,
Ruihan Li