usbdev_mmap causes type confusion in page_table_check

Commit Message

Ruihan Li May 7, 2023, 1:58 p.m. UTC

Hi all,

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/[e-mail]/

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.

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:


(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
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?

Thanks,
Ruihan Li

Comments

Pasha Tatashin May 8, 2023, 9:27 p.m. UTC | #1

On Sun, May 7, 2023 at 9:58 AM Ruihan Li <[e-mail]> 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/[e-mail]/
>
> 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

Matthew Wilcox (Oracle) May 8, 2023, 9:36 p.m. UTC | #2

On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> > 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);

No.  Slab pages are not elegible for mapping into userspace.  That's
all.  There should be a BUG() for that.  And I do mean BUG(), not
"return error to user".  Something has gone horribly wrong, and it's
time to crash.

David Hildenbrand May 8, 2023, 9:37 p.m. UTC | #3

On 08.05.23 23:27, Pasha Tatashin wrote:
> On Sun, May 7, 2023 at 9:58 AM Ruihan Li <[e-mail]> 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/[e-mail]/
>>
>> 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.


I'm surprised that we allow mapping slab pages to user space. I somehow 
remember that this is a big no-no.

Do we really want to allow that? Are there many such users or is this 
the only one?

If we do support it, we might want to update some PageAnon() checks in 
mm/gup.c too to exclude slab pages ...

Pasha Tatashin May 8, 2023, 9:48 p.m. UTC | #4

On Mon, May 8, 2023 at 2:36 PM Matthew Wilcox <[e-mail]> wrote:
>
> On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> > > 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);
>
> No.  Slab pages are not elegible for mapping into userspace.  That's

Sure, I can add BUG_ON(PageSlab(page)); to page_table_check_set.

> all.  There should be a BUG() for that.  And I do mean BUG(), not
> "return error to user".  Something has gone horribly wrong, and it's
> time to crash.

 It is just too easy to make slab available via remap_pfn_range(), but
I do not think we want to add BUG() into the remap function, otherwise
we will break devices such as /dev/mem.

Pasha

Matthew Wilcox (Oracle) May 8, 2023, 9:52 p.m. UTC | #5

On Mon, May 08, 2023 at 02:48:59PM -0700, Pasha Tatashin wrote:
> On Mon, May 8, 2023 at 2:36 PM Matthew Wilcox <[e-mail]> wrote:
> >
> > On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> > > > 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);
> >
> > No.  Slab pages are not elegible for mapping into userspace.  That's
> 
> Sure, I can add BUG_ON(PageSlab(page)); to page_table_check_set.
> 
> > all.  There should be a BUG() for that.  And I do mean BUG(), not
> > "return error to user".  Something has gone horribly wrong, and it's
> > time to crash.
> 
>  It is just too easy to make slab available via remap_pfn_range(), but
> I do not think we want to add BUG() into the remap function, otherwise
> we will break devices such as /dev/mem.

Slab pages can't be mmaped.  Really, no matter what interface you're
using.  page->_mapcount is necessarily incremented by mapping to
userspace, and slab uses that space for its own purposes (and has
for decades).  It's similar for page tables and other allocations that
use PageType.

Pasha Tatashin May 8, 2023, 9:55 p.m. UTC | #6

On Mon, May 8, 2023 at 2:52 PM Matthew Wilcox <[e-mail]> wrote:
>
> On Mon, May 08, 2023 at 02:48:59PM -0700, Pasha Tatashin wrote:
> > On Mon, May 8, 2023 at 2:36 PM Matthew Wilcox <[e-mail]> wrote:
> > >
> > > On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> > > > > 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);
> > >
> > > No.  Slab pages are not elegible for mapping into userspace.  That's
> >
> > Sure, I can add BUG_ON(PageSlab(page)); to page_table_check_set.
> >
> > > all.  There should be a BUG() for that.  And I do mean BUG(), not
> > > "return error to user".  Something has gone horribly wrong, and it's
> > > time to crash.
> >
> >  It is just too easy to make slab available via remap_pfn_range(), but
> > I do not think we want to add BUG() into the remap function, otherwise
> > we will break devices such as /dev/mem.
>
> Slab pages can't be mmaped.  Really, no matter what interface you're
> using.  page->_mapcount is necessarily incremented by mapping to
> userspace, and slab uses that space for its own purposes (and has
> for decades).  It's similar for page tables and other allocations that
> use PageType.

Mapping random memory in /dev/mem can cause mapping slab pages in to
userspace, the page->_mapcount is not incremented (and other fields
are not accessed) in that case, as we are using VM_PFNMAP type VMA,
which does not access "struct page".

Pasha

David Hildenbrand May 8, 2023, 10:46 p.m. UTC | #7

On 08.05.23 23:55, Pasha Tatashin wrote:
> On Mon, May 8, 2023 at 2:52 PM Matthew Wilcox <[e-mail]> wrote:
>>
>> On Mon, May 08, 2023 at 02:48:59PM -0700, Pasha Tatashin wrote:
>>> On Mon, May 8, 2023 at 2:36 PM Matthew Wilcox <[e-mail]> wrote:
>>>>
>>>> On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
>>>>>> 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);
>>>>
>>>> No.  Slab pages are not elegible for mapping into userspace.  That's
>>>
>>> Sure, I can add BUG_ON(PageSlab(page)); to page_table_check_set.
>>>
>>>> all.  There should be a BUG() for that.  And I do mean BUG(), not
>>>> "return error to user".  Something has gone horribly wrong, and it's
>>>> time to crash.
>>>
>>>   It is just too easy to make slab available via remap_pfn_range(), but
>>> I do not think we want to add BUG() into the remap function, otherwise
>>> we will break devices such as /dev/mem.
>>
>> Slab pages can't be mmaped.  Really, no matter what interface you're
>> using.  page->_mapcount is necessarily incremented by mapping to
>> userspace, and slab uses that space for its own purposes (and has
>> for decades).  It's similar for page tables and other allocations that
>> use PageType.
> 
> Mapping random memory in /dev/mem can cause mapping slab pages in to
> userspace, the page->_mapcount is not incremented (and other fields
> are not accessed) in that case, as we are using VM_PFNMAP type VMA,
> which does not access "struct page".

We should be using vm_normal_page() to identify if we should be looking 
at the struct page or not, no?

Pasha Tatashin May 8, 2023, 11:17 p.m. UTC | #8

On Mon, May 8, 2023 at 3:46 PM David Hildenbrand <[e-mail]> wrote:
>
> On 08.05.23 23:55, Pasha Tatashin wrote:
> > On Mon, May 8, 2023 at 2:52 PM Matthew Wilcox <[e-mail]> wrote:
> >>
> >> On Mon, May 08, 2023 at 02:48:59PM -0700, Pasha Tatashin wrote:
> >>> On Mon, May 8, 2023 at 2:36 PM Matthew Wilcox <[e-mail]> wrote:
> >>>>
> >>>> On Mon, May 08, 2023 at 05:27:10PM -0400, Pasha Tatashin wrote:
> >>>>>> 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);
> >>>>
> >>>> No.  Slab pages are not elegible for mapping into userspace.  That's
> >>>
> >>> Sure, I can add BUG_ON(PageSlab(page)); to page_table_check_set.
> >>>
> >>>> all.  There should be a BUG() for that.  And I do mean BUG(), not
> >>>> "return error to user".  Something has gone horribly wrong, and it's
> >>>> time to crash.
> >>>
> >>>   It is just too easy to make slab available via remap_pfn_range(), but
> >>> I do not think we want to add BUG() into the remap function, otherwise
> >>> we will break devices such as /dev/mem.
> >>
> >> Slab pages can't be mmaped.  Really, no matter what interface you're
> >> using.  page->_mapcount is necessarily incremented by mapping to
> >> userspace, and slab uses that space for its own purposes (and has
> >> for decades).  It's similar for page tables and other allocations that
> >> use PageType.
> >
> > Mapping random memory in /dev/mem can cause mapping slab pages in to
> > userspace, the page->_mapcount is not incremented (and other fields
> > are not accessed) in that case, as we are using VM_PFNMAP type VMA,
> > which does not access "struct page".
>
> We should be using vm_normal_page() to identify if we should be looking
> at the struct page or not, no?

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
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.

Pasha

>
> --
> Thanks,
>
> David / dhildenb
>

Pasha Tatashin May 8, 2023, 11:21 p.m. UTC | #9

> 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
> 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.

Pasha

David Hildenbrand May 8, 2023, 11:37 p.m. UTC | #10

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?

Pasha Tatashin May 9, 2023, 12:07 a.m. UTC | #11

On Mon, May 8, 2023 at 4:37 PM David Hildenbrand <[e-mail]> 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

Ruihan Li May 9, 2023, 1:18 a.m. UTC | #12

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 <[e-mail]> 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 <[e-mail]> 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

Christoph Hellwig May 9, 2023, 1:25 p.m. UTC | #13

On Sun, May 07, 2023 at 09:58:44PM +0800, Ruihan Li wrote:
> 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,

usb_alloc_coherent and up in the DMA coherent allocator (usually
anyway), and you absolutely must never do a virt_to_phys or virt_to_page
on that return value.  This code is a buggy as f**k.

Greg KH May 9, 2023, 2:01 p.m. UTC | #14

On Tue, May 09, 2023 at 06:25:42AM -0700, Christoph Hellwig wrote:
> On Sun, May 07, 2023 at 09:58:44PM +0800, Ruihan Li wrote:
> > 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,
> 
> usb_alloc_coherent and up in the DMA coherent allocator (usually
> anyway), and you absolutely must never do a virt_to_phys or virt_to_page
> on that return value.  This code is a buggy as f**k.

Odd, you gave it a reviewed-by: in commit a0e710a7def4 ("USB: usbfs: fix
mmap dma mismatch") back in 2020 when it was merged as you said that was
the way to fix this up.  :)

Do you have a better way to do it now that is more correct?  Did some
DMA changes happen that missed this codepath getting fixed up?

thanks,

gre gk-h

Christoph Hellwig May 10, 2023, 1:17 p.m. UTC | #15

On Tue, May 09, 2023 at 04:01:02PM +0200, Greg KH wrote:
> > > 	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,
> > 
> > usb_alloc_coherent and up in the DMA coherent allocator (usually
> > anyway), and you absolutely must never do a virt_to_phys or virt_to_page
> > on that return value.  This code is a buggy as f**k.
> 
> Odd, you gave it a reviewed-by: in commit a0e710a7def4 ("USB: usbfs: fix
> mmap dma mismatch") back in 2020 when it was merged as you said that was
> the way to fix this up.  :)
> 
> Do you have a better way to do it now that is more correct?  Did some
> DMA changes happen that missed this codepath getting fixed up?

Sorry, I should not have shouted as quickly.  The code is clearly
guarded by the same conditional that makes it not use the DMA API,
so from the DMA API POV this is actually correct, just ugly.

The fix for the actual remap_file_ranges thing is probably something
like this:

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index fbb087b728dc98..be56eba2558814 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -131,7 +131,7 @@ void *hcd_buffer_alloc(
 	/* 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(get_order(size), mem_flags);
 	}
 
 	for (i = 0; i < HCD_BUFFER_POOLS; i++) {
@@ -160,7 +160,7 @@ void hcd_buffer_free(
 	}
 
 	if (!hcd_uses_dma(hcd)) {
-		kfree(addr);
+		free_pages((unsigned long)addr, get_order(size));
 		return;
 	}

Patch

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++) {