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;
MIME-Version: 1.0
References: <000000000000258e5e05fae79fc1@google.com> <20230507135844.1231056-1-lrh2000@pku.edu.cn>
In-Reply-To: <20230507135844.1231056-1-lrh2000@pku.edu.cn>
From:   Pasha Tatashin <pasha.tatashin@soleen.com>
Date:   Mon, 8 May 2023 17:27:10 -0400
Message-ID: <CA+CK2bBe2YKYM3rUTCnZ0RF=NFUR9VqO-QYn3ygPsFJWLY1MUA@mail.gmail.com>
Subject: Re: usbdev_mmap causes type confusion in page_table_check
To:     Ruihan Li <lrh2000@pku.edu.cn>
Cc:     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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, May 7, 2023 at 9:58=E2=80=AFAM Ruihan Li <lrh2000@pku.edu.cn> wrote=
:
>
> Hi all,

Hi Ruihan,

Thank you for bisecting, and great analysis of the problem.

> Recently, syzbot reported [1] ("kernel BUG in page_table_check_clear"). A=
fter
> some bisection, I found out that when usbdev_mmap calls remap_pfn_range o=
n
> kmalloc'ed memory, it causes type confusion between struct folio and slab=
 in
> page_table_check. As I will explain below, it seems that both usb-side an=
d
> mm-side need some fixes. So I have cc'd linux-usb and linux-mm here, as w=
ell
> as their maintainers, to see whether there are any comments on the propos=
ed
> way to fix.
>
>  [1] https://lore.kernel.org/all/000000000000258e5e05fae79fc1@google.com/
>
> To handle mmap requests for /dev/bus/usb/XXX/YYY, usbdev_mmap first alloc=
ates
> memory by calling usb_alloc_coherent and then maps it into the user space
> using remap_pfn_range:
>
> static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> {
>         // ...
>         mem =3D usb_alloc_coherent(ps->dev, size, GFP_USER | __GFP_NOWARN=
,
>                         &dma_handle);
>         // ...
>         if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
>                 if (remap_pfn_range(vma, vma->vm_start,
>                                     virt_to_phys(usbm->mem) >> PAGE_SHIFT=
,
>                                     size, vma->vm_page_prot) < 0) {
>                         // ...
>                 }
>         }
>         // ...
> }
>
> Note that in this case, we consider the DMA-unavailable scenario, which, =
to be
> honest, is rare in practice. However, syzbot emulates USB devices using a
> module named dummy_hcd, and since these devices are emulated, DMA is not
> available at all.
>
> Meanwhile, usb_alloc_coherent calls hcd_buffer_alloc, which uses kmalloc =
for
> memory allocation:
>
> void *hcd_buffer_alloc(
>         struct usb_bus          *bus,
>         size_t                  size,
>         gfp_t                   mem_flags,
>         dma_addr_t              *dma
> )
> {
>         // ...
>         if (!hcd_uses_dma(hcd)) {
>                 *dma =3D ~(dma_addr_t) 0;
>                 return kmalloc(size, mem_flags);
>         }
>         // ...
> }
>
> However, during the update of the page table to map the kmalloc'd page in=
to
> the user space, page_table_check_set attempts to determine whether the pa=
ge is
> anonymous using PageAnon:
>
> static void page_table_check_set(struct mm_struct *mm, unsigned long addr=
,
>                                  unsigned long pfn, unsigned long pgcnt,
>                                  bool rw)
> {
>         // ...
>         anon =3D PageAnon(page);
>         for (i =3D 0; i < pgcnt; i++) {
>                 // ...
>                 if (anon) {
>                         BUG_ON(atomic_read(&ptc->file_map_count));
>                         BUG_ON(atomic_inc_return(&ptc->anon_map_count) > =
1 && rw);
>                 } else {
>                         BUG_ON(atomic_read(&ptc->anon_map_count));
>                         BUG_ON(atomic_inc_return(&ptc->file_map_count) < =
0);
>                 }
>                 // ...
>         }
>         // ...
> }
>
> This call to PageAnon is invalid for slab pages because slab reuses the b=
its
> in struct page/folio to store its internal states, and the anonymity bit =
only
> exists in struct page/folio. As a result, the counters are incorrectly up=
dated
> and checked in page_table_check_set and page_table_check_clear, leading t=
o the
> bug being raised.

We should change anon boolean to be:

anon =3D !PageSlab(page) && PageAnon(page);

This way, we will have a correct way to determine anon pages, and the
rest will fall into file_map_count. The file (non-anon) PTEs are OK to
be double mapped, so there won't be any problems from page_table_check
point of view even if it is a slab page.

> Intuitively, I don't think it's reasonable to call remap_pfn_range to map
> kmalloc'd pages into the user space. In the past, kmalloc'd pages might h=
ave
> had alignment issues when certain memory debugging options were enabled.
> Although this has been fixed in commit 59bb47985c1d ("mm, sl[aou]b: guara=
ntee
> natural alignment for kmalloc(power-of-two)"), it has been shown that
> performing such mapping is still buggy, as demonstrated by the type confu=
sion
> in page_table_check. Therefore, I propose adding a new function,
> hcd_buffer_alloc_pages, to guarantee the page requirements (i.e., no
> intermediate allocators, such as slab/slub). Below is a diff as a quick
> example:
>
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index fbb087b72..514bdc949 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -112,7 +112,7 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
>   * better sharing and to leverage mm/slab.c intelligence.
>   */
>
> -void *hcd_buffer_alloc(
> +void *hcd_buffer_alloc_pages(
>         struct usb_bus          *bus,
>         size_t                  size,
>         gfp_t                   mem_flags,
> @@ -126,12 +126,13 @@ void *hcd_buffer_alloc(
>                 return NULL;
>
>         if (hcd->localmem_pool)
> -               return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> +               return gen_pool_dma_alloc_align(hcd->localmem_pool, size,
> +                                               dma, PAGE_SIZE);
>
>         /* some USB hosts just use PIO */
>         if (!hcd_uses_dma(hcd)) {
>                 *dma =3D ~(dma_addr_t) 0;
> -               return kmalloc(size, mem_flags);
> +               return (void *)__get_free_pages(mem_flags, get_order(size=
));
>         }
>
>         for (i =3D 0; i < HCD_BUFFER_POOLS; i++) {
>
> (There appears to be another issue. In cases where hcd->localmem_pool is
> non-null, I suspect that remap_pfn_range should not be used. This is a DM=
A
> scenario, and the DMA handle is properly returned, so dma_mmap_coherent s=
hould
> be used instead. Am I correct?)
>
> 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 phys=
ical
> 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 m=
eans
> 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.
>
> If a fix is deemed necessary, I think taking the flag VM_PFNMAP into
> consideration is a reasonable solution, that said, in page_table_check_*,
>  * when the VM_PFNMAP flag is set, all operations and checks on
>    file_map_count and anon_map_count counters should be bypassed;
>  * when the VM_PFNMAP flag is not set, an additionally check to ensure
>    folio_test_slab evaluates to false should be performed.
>
> The implementation should be straightforward. However, I noticed that the
> page_table_check_* hooks are called in arch/* instead of mm/*, which not =
only
> limits its supported architectures (currently x86_64, arm64, and risc-v) =
but
> also makes it difficult to get the struct vm_area_struct as a parameter, =
since
> the struct vm_area_struct is not passed from mm/* to arch/* when mapping =
or
> unmapping pages. I have looked at d283d422c6c4 ("x86: mm: add x86_64 supp=
ort
> for page table check"), but I don't see a valid reason. Perhaps Pasha can
> provide some explanation about this implementation choice?

We specifically decided not to use VMA information in order to avoid
relying on MM state (except for limited "struct page" info). The
page_table_check is a separate from Linux-MM state machine that
verifies that the user accessible pages are not falsely shared.

Pasha