| Message ID | 20230510085527.57953-1-lrh2000@pku.edu.cn |
|---|---|
| Headers | show |
| Series | Fix type confusion in page_table_check | expand |
On Wed, May 10, 2023 at 04:55:24PM +0800, Ruihan Li wrote: > The current implementation of usbdev_mmap uses usb_alloc_coherent to > allocate memory pages that will later be mapped into the user space. > Meanwhile, usb_alloc_coherent employs three different methods to > allocate memory, as outlined below: > * If hcd->localmem_pool is non-null, it uses gen_pool_dma_alloc to > allocate memory. > * If DMA is not available, it uses kmalloc to allocate memory. > * Otherwise, it uses dma_alloc_coherent. > > However, it should be noted that gen_pool_dma_alloc does not guarantee > that the resulting memory will be page-aligned. Furthermore, trying to > map slab pages (i.e., memory allocated by kmalloc) into the user space > is not resonable and can lead to problems, such as a type confusion bug > when PAGE_TABLE_CHECK=y [1]. > > To address these issues, this patch introduces hcd_alloc_coherent_pages, > which addresses the above two problems. Specifically, > hcd_alloc_coherent_pages uses gen_pool_dma_alloc_align instead of > gen_pool_dma_alloc to ensure that the memory is page-aligned. To replace > kmalloc, hcd_alloc_coherent_pages directly allocates pages by calling > __get_free_pages. > > Reported-by: syzbot+fcf1a817ceb50935ce99@syzkaller.appspotmail.comm > Closes: https://lore.kernel.org/lkml/000000000000258e5e05fae79fc1@google.com/ [1] > Cc: stable@vger.kernel.org > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > --- I'm never quite sure about when it makes sense to complain about stylistic issues. Nevertheless, I'm going to do so here... > drivers/usb/core/buffer.c | 41 +++++++++++++++++++++++++++++++++++++++ > drivers/usb/core/devio.c | 9 +++++---- > include/linux/usb/hcd.h | 5 +++++ > 3 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c > index fbb087b72..6010ef9f5 100644 > --- a/drivers/usb/core/buffer.c > +++ b/drivers/usb/core/buffer.c > @@ -172,3 +172,44 @@ void hcd_buffer_free( > } > dma_free_coherent(hcd->self.sysdev, size, addr, dma); > } > + > +void *hcd_buffer_alloc_pages(struct usb_hcd *hcd, size_t size, > + gfp_t mem_flags, dma_addr_t *dma) > +{ > + if (size == 0) > + return NULL; > + > + if (hcd->localmem_pool) > + return gen_pool_dma_alloc_align(hcd->localmem_pool, > + size, dma, PAGE_SIZE); C isn't Lisp. Expressions in C are not based entirely around parentheses, and it's not necessary to align our code based on the parenthesized sub-expressions to avoid hopelessly confusing the reader. The style used in this file (and many other places in the USB core) is to indent continuation lines by two tab stops. The same comment applies to all the other continuation lines you added or changed in this patch and in patch 2/4. Alan Stern
Hi Alan, On Wed, May 10, 2023 at 10:38:48AM -0400, Alan Stern wrote: > On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote: > > When hcd->localmem_pool is non-null, it is used to allocate DMA memory. > > In this case, the dma address will be properly returned (in dma_handle), > > and dma_mmap_coherent should be used to map this memory into the user > > space. However, the current implementation uses pfn_remap_range, which > > is supposed to map normal pages (instead of DMA pages). > > > > Instead of repeating the logic in the memory allocation function, this > > patch introduces a more robust solution. To address the previous issue, > > this patch checks the type of allocated memory by testing whether > > dma_handle is properly set. If dma_handle is properly returned, it means > > some DMA pages are allocated and dma_mmap_coherent should be used to map > > them. Otherwise, normal pages are allocated and pfn_remap_range should > > be called. This ensures that the correct mmap functions are used > > consistently, independently with logic details that determine which type > > of memory gets allocated. > > > > Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch") > > Cc: stable@vger.kernel.org > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > > --- > > drivers/usb/core/devio.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > index b4cf9e860..5067030b7 100644 > > --- a/drivers/usb/core/devio.c > > +++ b/drivers/usb/core/devio.c > > @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > size_t size = vma->vm_end - vma->vm_start; > > void *mem; > > unsigned long flags; > > - dma_addr_t dma_handle; > > + dma_addr_t dma_handle = DMA_MAPPING_ERROR; > > int ret; > > > > ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory)); > > @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > usbm->vma_use_count = 1; > > INIT_LIST_HEAD(&usbm->memlist); > > > > - if (hcd->localmem_pool || !hcd_uses_dma(hcd)) { > > + /* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates > > + * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check > > + * whether we are in such cases, and then use remap_pfn_range (or > > + * dma_mmap_coherent) to map normal (or DMA) pages into the user > > + * space, respectively. > > + */ > > Another stylistic issue. For multi-line comments, the format we use is: > > /* > * Blah, blah, blah > * Blah, blah, blah > */ > > Alan Stern Yeah, I am pretty sure it is another style difference with the net subsystem. Again, in the next version, I'll follow the coding style that you have pointed out. Thanks, Ruihan Li
On 10.05.23 17:41, Ruihan Li wrote: > Hi Alan, > > On Wed, May 10, 2023 at 10:38:48AM -0400, Alan Stern wrote: >> On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote: >>> When hcd->localmem_pool is non-null, it is used to allocate DMA memory. >>> In this case, the dma address will be properly returned (in dma_handle), >>> and dma_mmap_coherent should be used to map this memory into the user >>> space. However, the current implementation uses pfn_remap_range, which >>> is supposed to map normal pages (instead of DMA pages). >>> >>> Instead of repeating the logic in the memory allocation function, this >>> patch introduces a more robust solution. To address the previous issue, >>> this patch checks the type of allocated memory by testing whether >>> dma_handle is properly set. If dma_handle is properly returned, it means >>> some DMA pages are allocated and dma_mmap_coherent should be used to map >>> them. Otherwise, normal pages are allocated and pfn_remap_range should >>> be called. This ensures that the correct mmap functions are used >>> consistently, independently with logic details that determine which type >>> of memory gets allocated. >>> >>> Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> >>> --- >>> drivers/usb/core/devio.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c >>> index b4cf9e860..5067030b7 100644 >>> --- a/drivers/usb/core/devio.c >>> +++ b/drivers/usb/core/devio.c >>> @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) >>> size_t size = vma->vm_end - vma->vm_start; >>> void *mem; >>> unsigned long flags; >>> - dma_addr_t dma_handle; >>> + dma_addr_t dma_handle = DMA_MAPPING_ERROR; >>> int ret; >>> >>> ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory)); >>> @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) >>> usbm->vma_use_count = 1; >>> INIT_LIST_HEAD(&usbm->memlist); >>> >>> - if (hcd->localmem_pool || !hcd_uses_dma(hcd)) { >>> + /* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates >>> + * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check >>> + * whether we are in such cases, and then use remap_pfn_range (or >>> + * dma_mmap_coherent) to map normal (or DMA) pages into the user >>> + * space, respectively. >>> + */ >> >> Another stylistic issue. For multi-line comments, the format we use is: >> >> /* >> * Blah, blah, blah >> * Blah, blah, blah >> */ >> >> Alan Stern > > Yeah, I am pretty sure it is another style difference with the net > subsystem. Again, in the next version, I'll follow the coding style that > you have pointed out. Documentation/process/coding-style.rst spells out that net/ and drivers/net/ are "special". Regarding breaking long lines, it's just an inconsistent, undocumented mess IIRC ...
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
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(-)