On Sun, May 7, 2023 at 9:58 AM Ruihan Li <lrh2000@xxxxxxxxxx> 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"). After
> some bisection, I found out that when usbdev_mmap calls remap_pfn_range on
> 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 and
> mm-side need some fixes. So I have cc'd linux-usb and linux-mm here, as well
> as their maintainers, to see whether there are any comments on the proposed
> way to fix.
>
> [1] https://lore.kernel.org/all/000000000000258e5e05fae79fc1@xxxxxxxxxx/
>
> To handle mmap requests for /dev/bus/usb/XXX/YYY, usbdev_mmap first allocates
> 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 = 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 = ~(dma_addr_t) 0;
> return kmalloc(size, mem_flags);
> }
> // ...
> }
>
> However, during the update of the page table to map the kmalloc'd page into
> the user space, page_table_check_set attempts to determine whether the page 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 = PageAnon(page);
> for (i = 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 bits
> 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 updated
> and checked in page_table_check_set and page_table_check_clear, leading to the
> bug being raised.
We should change anon boolean to be:
anon = !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 have
> had alignment issues when certain memory debugging options were enabled.
> Although this has been fixed in commit 59bb47985c1d ("mm, sl[aou]b: guarantee
> natural alignment for kmalloc(power-of-two)"), it has been shown that
> performing such mapping is still buggy, as demonstrated by the type confusion
> 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 = ~(dma_addr_t) 0;
> - return kmalloc(size, mem_flags);
> + return (void *)__get_free_pages(mem_flags, get_order(size));
> }
>
> for (i = 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 DMA
> scenario, and the DMA handle is properly returned, so dma_mmap_coherent should
> 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 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.
>
> 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 support
> 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
