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/[email protected]/T/
Cc: Pasha Tatashin <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
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
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: [email protected]
Signed-off-by: Ruihan Li <[email protected]>
---
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.
+ */
+ if (dma_handle == DMA_MAPPING_ERROR) {
if (remap_pfn_range(vma, vma->vm_start,
virt_to_phys(usbm->mem) >> PAGE_SHIFT,
size, vma->vm_page_prot) < 0) {
--
2.40.1
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: [email protected]
Closes: https://lore.kernel.org/lkml/[email protected]/ [1]
Cc: [email protected]
Signed-off-by: Ruihan Li <[email protected]>
---
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);
+
+ /* some USB hosts just use PIO */
+ if (!hcd_uses_dma(hcd)) {
+ *dma = DMA_MAPPING_ERROR;
+ return (void *)__get_free_pages(mem_flags,
+ get_order(size));
+ }
+
+ return dma_alloc_coherent(hcd->self.sysdev,
+ size, dma, mem_flags);
+}
+
+void hcd_buffer_free_pages(struct usb_hcd *hcd, size_t size,
+ void *addr, dma_addr_t dma)
+{
+ if (!addr)
+ return;
+
+ if (hcd->localmem_pool) {
+ gen_pool_free(hcd->localmem_pool,
+ (unsigned long)addr, size);
+ return;
+ }
+
+ if (!hcd_uses_dma(hcd)) {
+ free_pages((unsigned long)addr, get_order(size));
+ return;
+ }
+
+ dma_free_coherent(hcd->self.sysdev, size, addr, dma);
+}
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index e501a03d6..b4cf9e860 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -186,6 +186,7 @@ static int connected(struct usb_dev_state *ps)
static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
{
struct usb_dev_state *ps = usbm->ps;
+ struct usb_hcd *hcd = bus_to_hcd(ps->dev->bus);
unsigned long flags;
spin_lock_irqsave(&ps->lock, flags);
@@ -194,8 +195,8 @@ static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
list_del(&usbm->memlist);
spin_unlock_irqrestore(&ps->lock, flags);
- usb_free_coherent(ps->dev, usbm->size, usbm->mem,
- usbm->dma_handle);
+ hcd_buffer_free_pages(hcd, usbm->size, usbm->mem,
+ usbm->dma_handle);
usbfs_decrease_memory_usage(
usbm->size + sizeof(struct usb_memory));
kfree(usbm);
@@ -247,8 +248,8 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
goto error_decrease_mem;
}
- mem = usb_alloc_coherent(ps->dev, size, GFP_USER | __GFP_NOWARN,
- &dma_handle);
+ mem = hcd_buffer_alloc_pages(hcd, size, GFP_USER | __GFP_NOWARN,
+ &dma_handle);
if (!mem) {
ret = -ENOMEM;
goto error_free_usbm;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 094c77eaf..79f89109e 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -501,6 +501,11 @@ void *hcd_buffer_alloc(struct usb_bus *bus, size_t size,
void hcd_buffer_free(struct usb_bus *bus, size_t size,
void *addr, dma_addr_t dma);
+void *hcd_buffer_alloc_pages(struct usb_hcd *hcd, size_t size,
+ gfp_t mem_flags, dma_addr_t *dma);
+void hcd_buffer_free_pages(struct usb_hcd *hcd, size_t size,
+ void *addr, dma_addr_t dma);
+
/* generic bus glue, needed for host controllers that don't use PCI */
extern irqreturn_t usb_hcd_irq(int irq, void *__hcd);
--
2.40.1
The special device /dev/mem enables users to map arbitrary physical
memory regions into the user space, which can conflict with the double
mapping detection logic used by the page table check. For instance,
pages may change their properties (e.g., from anonymous pages to named
pages) while they are still being mapped in the user space via /dev/mem,
leading to "corruption" detected by the page table check.
To address this issue, the PAGE_TABLE_CHECK config option is now
dependent on !DEVMM. This ensures that the page table check cannot be
enabled when /dev/mem is used. It should be noted that /dev/mem itself
is a significant security issue, and its conflict with a hardening
technique is understandable.
Cc: <[email protected]> # 5.17
Signed-off-by: Ruihan Li <[email protected]>
---
Documentation/mm/page_table_check.rst | 18 ++++++++++++++++++
mm/Kconfig.debug | 2 +-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
index cfd8f4117..b04f29230 100644
--- a/Documentation/mm/page_table_check.rst
+++ b/Documentation/mm/page_table_check.rst
@@ -52,3 +52,21 @@ Build kernel with:
Optionally, build kernel with PAGE_TABLE_CHECK_ENFORCED in order to have page
table support without extra kernel parameter.
+
+Implementation notes
+====================
+
+We specifically decided not to use VMA information in order to avoid relying on
+MM states (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.
+
+As a result, special devices that violate the model cannot live with
+PAGE_TABLE_CHECK. Currently, /dev/mem is the only known example. Given it
+allows users to map arbitrary physical memory regions into the userspace, any
+pages may change their properties (e.g., from anonymous pages to named pages)
+while they are still being mapped in the userspace via /dev/mem, leading to
+"corruption" detected by the page table check. Therefore, the PAGE_TABLE_CHECK
+config option is now dependent on !DEVMEM. It's worth noting that /dev/mem
+itself is a significant security issue, and its conflict with a hardening
+technique is understandable.
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index a925415b4..37f3d5b20 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -97,7 +97,7 @@ config PAGE_OWNER
config PAGE_TABLE_CHECK
bool "Check for invalid mappings in user page tables"
- depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK
+ depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK && !DEVMEM
select PAGE_EXTENSION
help
Check that anonymous page is not being mapped twice with read write
--
2.40.1
The current uses of PageAnon in page table check functions can lead to
type confusion bugs between struct page and slab [1], if slab pages are
accidentally mapped into the user space. This is because slab reuses the
bits in struct page to store its internal states, which renders PageAnon
ineffective on slab pages.
Since slab pages are not expected to be mapped into user spaces, this
patch adds BUG_ON(PageSlab(page)) checks to ensure that slab pages are
not inadvertently mapped. Otherwise, there must be some bugs in the
kernel.
Reported-by: [email protected]
Closes: https://lore.kernel.org/lkml/[email protected]/ [1]
Fixes: df4e817b7108 ("mm: page table check")
Cc: <[email protected]> # 5.17
Signed-off-by: Ruihan Li <[email protected]>
---
include/linux/page-flags.h | 6 ++++++
mm/page_table_check.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 1c68d67b8..7475a5399 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -617,6 +617,12 @@ PAGEFLAG_FALSE(VmemmapSelfHosted, vmemmap_self_hosted)
* Please note that, confusingly, "page_mapping" refers to the inode
* address_space which maps the page from disk; whereas "page_mapped"
* refers to user virtual address space into which the page is mapped.
+ *
+ * For slab pages, since slab reuses the bits in struct page to store its
+ * internal states, the page->mapping does not exist as such, nor do these
+ * flags below. So in order to avoid testing non-existent bits, please
+ * make sure that PageSlab(page) actually evaluates to false before calling
+ * the following functions (e.g., PageAnon). See slab.h.
*/
#define PAGE_MAPPING_ANON 0x1
#define PAGE_MAPPING_MOVABLE 0x2
diff --git a/mm/page_table_check.c b/mm/page_table_check.c
index 25d8610c0..f2baf97d5 100644
--- a/mm/page_table_check.c
+++ b/mm/page_table_check.c
@@ -71,6 +71,8 @@ static void page_table_check_clear(struct mm_struct *mm, unsigned long addr,
page = pfn_to_page(pfn);
page_ext = page_ext_get(page);
+
+ BUG_ON(PageSlab(page));
anon = PageAnon(page);
for (i = 0; i < pgcnt; i++) {
@@ -107,6 +109,8 @@ static void page_table_check_set(struct mm_struct *mm, unsigned long addr,
page = pfn_to_page(pfn);
page_ext = page_ext_get(page);
+
+ BUG_ON(PageSlab(page));
anon = PageAnon(page);
for (i = 0; i < pgcnt; i++) {
@@ -133,6 +137,8 @@ void __page_table_check_zero(struct page *page, unsigned int order)
struct page_ext *page_ext;
unsigned long i;
+ BUG_ON(PageSlab(page));
+
page_ext = page_ext_get(page);
BUG_ON(!page_ext);
for (i = 0; i < (1ul << order); i++) {
--
2.40.1
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: [email protected]
> Signed-off-by: Ruihan Li <[email protected]>
> ---
> 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
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: [email protected]
> Closes: https://lore.kernel.org/lkml/[email protected]/ [1]
> Cc: [email protected]
> Signed-off-by: Ruihan Li <[email protected]>
> ---
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: [email protected]
> > Signed-off-by: Ruihan Li <[email protected]>
> > ---
> > 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
Hi Alan,
On Wed, May 10, 2023 at 10:37:45AM -0400, Alan Stern wrote:
> 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: [email protected]
> > Closes: https://lore.kernel.org/lkml/[email protected]/ [1]
> > Cc: [email protected]
> > Signed-off-by: Ruihan Li <[email protected]>
> > ---
>
> 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
I'm just a bit shocked to find out that different subsystems might
prefer different styles of coding. In the net subsystem, checkpatch.pl
will complain that:
CHECK: Alignment should match open parenthesis
Nevertheless, 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: [email protected]
>>> Signed-off-by: Ruihan Li <[email protected]>
>>> ---
>>> 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 ...
--
Thanks,
David / dhildenb
On 10.05.23 10:55, Ruihan Li wrote:
> The special device /dev/mem enables users to map arbitrary physical
> memory regions into the user space, which can conflict with the double
> mapping detection logic used by the page table check. For instance,
> pages may change their properties (e.g., from anonymous pages to named
> pages) while they are still being mapped in the user space via /dev/mem,
> leading to "corruption" detected by the page table check.
>
> To address this issue, the PAGE_TABLE_CHECK config option is now
> dependent on !DEVMM. This ensures that the page table check cannot be
> enabled when /dev/mem is used. It should be noted that /dev/mem itself
> is a significant security issue, and its conflict with a hardening
> technique is understandable.
>
> Cc: <[email protected]> # 5.17
> Signed-off-by: Ruihan Li <[email protected]>
> ---
> Documentation/mm/page_table_check.rst | 18 ++++++++++++++++++
> mm/Kconfig.debug | 2 +-
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
> index cfd8f4117..b04f29230 100644
> --- a/Documentation/mm/page_table_check.rst
> +++ b/Documentation/mm/page_table_check.rst
> @@ -52,3 +52,21 @@ Build kernel with:
>
> Optionally, build kernel with PAGE_TABLE_CHECK_ENFORCED in order to have page
> table support without extra kernel parameter.
> +
> +Implementation notes
> +====================
> +
> +We specifically decided not to use VMA information in order to avoid relying on
> +MM states (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.
> +
> +As a result, special devices that violate the model cannot live with
> +PAGE_TABLE_CHECK. Currently, /dev/mem is the only known example. Given it
> +allows users to map arbitrary physical memory regions into the userspace, any
> +pages may change their properties (e.g., from anonymous pages to named pages)
> +while they are still being mapped in the userspace via /dev/mem, leading to
> +"corruption" detected by the page table check. Therefore, the PAGE_TABLE_CHECK
> +config option is now dependent on !DEVMEM. It's worth noting that /dev/mem
> +itself is a significant security issue, and its conflict with a hardening
> +technique is understandable.
> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> index a925415b4..37f3d5b20 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,7 +97,7 @@ config PAGE_OWNER
>
> config PAGE_TABLE_CHECK
> bool "Check for invalid mappings in user page tables"
> - depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK
> + depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK && !DEVMEM
> select PAGE_EXTENSION
> help
> Check that anonymous page is not being mapped twice with read write
That might disable it in a lot of environments I'm afraid. I wonder if
we could allow it for STRICT_DEVMEM. Hm ...
--
Thanks,
David / dhildenb
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/[email protected]/T/
Can you see if you can implement Christoph's proposed change instead:
https://lore.kernel.org/r/[email protected]
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/[email protected]/T/
>
> Can you see if you can implement Christoph's proposed change instead:
> https://lore.kernel.org/r/[email protected]
>
> 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 Wed, May 10, 2023 at 06:40:31PM +0200, David Hildenbrand wrote:
> On 10.05.23 10:55, Ruihan Li wrote:
> > The special device /dev/mem enables users to map arbitrary physical
> > memory regions into the user space, which can conflict with the double
> > mapping detection logic used by the page table check. For instance,
> > pages may change their properties (e.g., from anonymous pages to named
> > pages) while they are still being mapped in the user space via /dev/mem,
> > leading to "corruption" detected by the page table check.
> >
> > To address this issue, the PAGE_TABLE_CHECK config option is now
> > dependent on !DEVMM. This ensures that the page table check cannot be
> > enabled when /dev/mem is used. It should be noted that /dev/mem itself
> > is a significant security issue, and its conflict with a hardening
> > technique is understandable.
> >
> > Cc: <[email protected]> # 5.17
> > Signed-off-by: Ruihan Li <[email protected]>
> > ---
> > Documentation/mm/page_table_check.rst | 18 ++++++++++++++++++
> > mm/Kconfig.debug | 2 +-
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
> > index cfd8f4117..b04f29230 100644
> > --- a/Documentation/mm/page_table_check.rst
> > +++ b/Documentation/mm/page_table_check.rst
> > @@ -52,3 +52,21 @@ Build kernel with:
> > Optionally, build kernel with PAGE_TABLE_CHECK_ENFORCED in order to have page
> > table support without extra kernel parameter.
> > +
> > +Implementation notes
> > +====================
> > +
> > +We specifically decided not to use VMA information in order to avoid relying on
> > +MM states (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.
> > +
> > +As a result, special devices that violate the model cannot live with
> > +PAGE_TABLE_CHECK. Currently, /dev/mem is the only known example. Given it
> > +allows users to map arbitrary physical memory regions into the userspace, any
> > +pages may change their properties (e.g., from anonymous pages to named pages)
> > +while they are still being mapped in the userspace via /dev/mem, leading to
> > +"corruption" detected by the page table check. Therefore, the PAGE_TABLE_CHECK
> > +config option is now dependent on !DEVMEM. It's worth noting that /dev/mem
> > +itself is a significant security issue, and its conflict with a hardening
> > +technique is understandable.
> > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> > index a925415b4..37f3d5b20 100644
> > --- a/mm/Kconfig.debug
> > +++ b/mm/Kconfig.debug
> > @@ -97,7 +97,7 @@ config PAGE_OWNER
> > config PAGE_TABLE_CHECK
> > bool "Check for invalid mappings in user page tables"
> > - depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK
> > + depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK && !DEVMEM
> > select PAGE_EXTENSION
> > help
> > Check that anonymous page is not being mapped twice with read write
>
> That might disable it in a lot of environments I'm afraid. I wonder if we
> could allow it for STRICT_DEVMEM. Hm ...
> --
> Thanks,
>
> David / dhildenb
That sounds pretty reasonable. However, I'm not quite sure if PageAnon
makes sense of (and is guaranteed to work well with) I/O memory pages,
which should be the only pages allowed to be accessed via /dev/mem under
STRICT_DEVMEM.
A quick test has shown that PageAnon (by accident or design?) results in
"false" for I/O memory pages. Meanwhile, the logic used in the page
table check allows named (i.e., non-anonymous) pages to be shared
arbitrarily (i.e. in both read-only and read-write modes) between
processes. So it looks that everything works fine. But is it a
coincidence?
Thanks,
Ruihan Li
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