Recently, syzbot reported [1] ("kernel BUG in page_table_check_clear").
The root cause is that usbdev_mmap calls remap_pfn_range on kmalloc'ed
memory, which leads to type confusion between struct page and slab in
page_table_check. This series of patches fixes the usb side by avoiding
mapping slab pages into userspace, and fixes the mm side by enforcing
that all user-accessible pages are not slab pages. A more detailed
analysis and some discussion of how to fix the problem can also be found
in [1].

 [1] https://lore.kernel.org/lkml/20230507135844.1231056-1-lrh2000@pku.edu.cn/T/

Cc: Pasha Tatashin <pasha.tatashin@soleen.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Ruihan Li (4):
  usb: usbfs: Enforce page requirements for mmap
  usb: usbfs: Use consistent mmap functions
  mm: page_table_check: Make it dependent on !DEVMEM
  mm: page_table_check: Ensure user pages are not slab pages

 Documentation/mm/page_table_check.rst | 18 ++++++++++++
 drivers/usb/core/buffer.c             | 41 +++++++++++++++++++++++++++
 drivers/usb/core/devio.c              | 15 +++++++---
 include/linux/page-flags.h            |  6 ++++
 include/linux/usb/hcd.h               |  5 ++++
 mm/Kconfig.debug                      |  2 +-
 mm/page_table_check.c                 |  6 ++++
 7 files changed, 88 insertions(+), 5 deletions(-)

-- 
2.40.1

On Wed, May 10, 2023 at 04:55:23PM +0800, Ruihan Li wrote:
> Recently, syzbot reported [1] ("kernel BUG in page_table_check_clear").
> The root cause is that usbdev_mmap calls remap_pfn_range on kmalloc'ed
> memory, which leads to type confusion between struct page and slab in
> page_table_check. This series of patches fixes the usb side by avoiding
> mapping slab pages into userspace, and fixes the mm side by enforcing
> that all user-accessible pages are not slab pages. A more detailed
> analysis and some discussion of how to fix the problem can also be found
> in [1].
> 
>  [1] https://lore.kernel.org/lkml/20230507135844.1231056-1-lrh2000@pku.edu.cn/T/

Can you see if you can implement Christoph's proposed change instead:
	https://lore.kernel.org/r/ZFuZVDcU81WmqEvJ@infradead.org

As it might not actually be as bad as you think to require this type of
churn.

thanks,

greg k-h

On Thu, May 11, 2023 at 07:51:58AM +0900, Greg Kroah-Hartman wrote:
> On Wed, May 10, 2023 at 04:55:23PM +0800, Ruihan Li wrote:
> > Recently, syzbot reported [1] ("kernel BUG in page_table_check_clear").
> > The root cause is that usbdev_mmap calls remap_pfn_range on kmalloc'ed
> > memory, which leads to type confusion between struct page and slab in
> > page_table_check. This series of patches fixes the usb side by avoiding
> > mapping slab pages into userspace, and fixes the mm side by enforcing
> > that all user-accessible pages are not slab pages. A more detailed
> > analysis and some discussion of how to fix the problem can also be found
> > in [1].
> > 
> >  [1] https://lore.kernel.org/lkml/20230507135844.1231056-1-lrh2000@pku.edu.cn/T/
> 
> Can you see if you can implement Christoph's proposed change instead:
> 	https://lore.kernel.org/r/ZFuZVDcU81WmqEvJ@infradead.org
> 
> As it might not actually be as bad as you think to require this type of
> churn.
> 
> thanks,
> 
> greg k-h

Sorry, but no.

Christoph's patch perfectly fixes _one_ problem: kmalloc'ed memory
cannot be mapped to user space. However, as I detailed in the commit
message, this series of patches fixes _three_ problems.

I have to say that the original code is quite buggy. In the
gen_pool_dma_alloc path, there is no guarantee of page alignment. 

void *hcd_buffer_alloc(...)
{
	// ...
	if (hcd->localmem_pool)
		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
	// ...
}

When localmem_pool gets initialized, the alignment bits are set to 4
(instead of PAGE_SHIFT).

int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
			    dma_addr_t dma, size_t size)
{
	// ...
	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 4,
						  dev_to_node(hcd->self.sysdev),
						  dev_name(hcd->self.sysdev));
	// ...
}

It is introduced by commit ff2437befd8f ("usb: host: Fix excessive
alignment restriction for local memory allocations") [1].

 [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ff2437befd8fe52046e0db949347b5bcfab6b097

If you don't believe me, please see my test results. In the following
qemu setup,

	qemu-system-sh4 -M r2d -kernel arch/sh/boot/zImage \
		-usb -device usb-storage,drive=d0 \
		-drive file=test.img,if=none,id=d0,format=raw \
		-append "console=tty0 console=ttySC1,115200 rootwait root=/dev/sda init=/bin/sh" \
		-serial null -serial stdio

together with the following patch,

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index e501a03d6..d7069c986 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -265,6 +265,10 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	INIT_LIST_HEAD(&usbm->memlist);
 
 	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
+		printk("usb_alloc_coherent returns %px\n", usbm->mem);
+		printk("so the mapping starts at %lx\n",
+				virt_to_phys(usbm->mem) >> PAGE_SHIFT);
+
 		if (remap_pfn_range(vma, vma->vm_start,
 				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
 				    size, vma->vm_page_prot) < 0) {

it quickly leads to the following output when I manage to map a page
from /dev/bus/usb/001/002,

	usb_alloc_coherent returns b07c06c0
	so the mapping starts at 307c0

What's more, in this case, remap_pfn_range should _not_ be used, since
we are going to map a DMA page. However, as you can see, usbdev_mmap
actually goes to the wrong path.

If you say I shouldn't fix all these bugs in _this_ patch series, that's
fine. However, I do think that these bugs should be fixed, perhaps in
another patch series.

Thanks,
Ruihan Li

On Thu, May 11, 2023 at 09:44:55PM +0800, Ruihan Li wrote:
> Christoph's patch perfectly fixes _one_ problem: kmalloc'ed memory
> cannot be mapped to user space. However, as I detailed in the commit
> message, this series of patches fixes _three_ problems.

FYI, I agree with you.  My simple patch was sent before reading
your new series, and is a strict subset of it.

> I have to say that the original code is quite buggy. In the
> gen_pool_dma_alloc path, there is no guarantee of page alignment. 

I also find this whole interface very problematic to start with,
but that's a separate discussion for later.

On Thu, May 11, 2023 at 08:32:01AM -0700, Christoph Hellwig wrote:
> On Thu, May 11, 2023 at 09:44:55PM +0800, Ruihan Li wrote:
> > Christoph's patch perfectly fixes _one_ problem: kmalloc'ed memory
> > cannot be mapped to user space. However, as I detailed in the commit
> > message, this series of patches fixes _three_ problems.
> 
> FYI, I agree with you.  My simple patch was sent before reading
> your new series, and is a strict subset of it.

Thank you for the clarification.

> > I have to say that the original code is quite buggy. In the
> > gen_pool_dma_alloc path, there is no guarantee of page alignment. 
> 
> I also find this whole interface very problematic to start with,
> but that's a separate discussion for later.

Yes. I don't think hybrid allocation of DMA memory and normal memory in
one function is a good thing, but currently there is no clear way to fix
this. Mixing memory allocation and page allocation is another bad thing,
and at least, my patch can (hopefully) solve the second (and much
easier) issue.

Thanks,
Ruihan Li

On Fri, May 12, 2023 at 12:19:09AM +0800, Ruihan Li wrote:
> On Thu, May 11, 2023 at 08:32:01AM -0700, Christoph Hellwig wrote:
> > On Thu, May 11, 2023 at 09:44:55PM +0800, Ruihan Li wrote:
> > > Christoph's patch perfectly fixes _one_ problem: kmalloc'ed memory
> > > cannot be mapped to user space. However, as I detailed in the commit
> > > message, this series of patches fixes _three_ problems.
> > 
> > FYI, I agree with you.  My simple patch was sent before reading
> > your new series, and is a strict subset of it.
> 
> Thank you for the clarification.
> 
> > > I have to say that the original code is quite buggy. In the
> > > gen_pool_dma_alloc path, there is no guarantee of page alignment. 
> > 
> > I also find this whole interface very problematic to start with,
> > but that's a separate discussion for later.
> 
> Yes. I don't think hybrid allocation of DMA memory and normal memory in
> one function is a good thing, but currently there is no clear way to fix
> this. Mixing memory allocation and page allocation is another bad thing,
> and at least, my patch can (hopefully) solve the second (and much
> easier) issue.

Ok, I'll take these through the usb tree if I can get an ack for the
mm-specific patches.  Or were you going to send a v2 series?

thanks,

greg k-h

On Sat, May 13, 2023 at 06:37:52PM +0900, Greg Kroah-Hartman wrote:
> 
> On Fri, May 12, 2023 at 12:19:09AM +0800, Ruihan Li wrote:
> > On Thu, May 11, 2023 at 08:32:01AM -0700, Christoph Hellwig wrote:
> > > On Thu, May 11, 2023 at 09:44:55PM +0800, Ruihan Li wrote:
> > > > Christoph's patch perfectly fixes _one_ problem: kmalloc'ed memory
> > > > cannot be mapped to user space. However, as I detailed in the commit
> > > > message, this series of patches fixes _three_ problems.
> > > 
> > > FYI, I agree with you.  My simple patch was sent before reading
> > > your new series, and is a strict subset of it.
> > 
> > Thank you for the clarification.
> > 
> > > > I have to say that the original code is quite buggy. In the
> > > > gen_pool_dma_alloc path, there is no guarantee of page alignment. 
> > > 
> > > I also find this whole interface very problematic to start with,
> > > but that's a separate discussion for later.
> > 
> > Yes. I don't think hybrid allocation of DMA memory and normal memory in
> > one function is a good thing, but currently there is no clear way to fix
> > this. Mixing memory allocation and page allocation is another bad thing,
> > and at least, my patch can (hopefully) solve the second (and much
> > easier) issue.
> 
> Ok, I'll take these through the usb tree if I can get an ack for the
> mm-specific patches.  Or were you going to send a v2 series?
> 
> thanks,
> 
> greg k-h

There are some comments from Alan and David, so I'll send a v2 series to
address them (probably tomorrow).

Thanks,
Ruihan Li