On 7 Mar 2024, at 8:49, Dan Carpenter wrote:
> Hello Zi Yan,
>
> Commit fc4d182316bd ("mm: huge_memory: enable debugfs to split huge
> pages to any order") from Feb 26, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> mm/huge_memory.c:2898 __split_huge_page()
> error: undefined (user controlled) shift '1 << new_order'
>
> mm/huge_memory.c
> 2889 static void __split_huge_page(struct page *page, struct list_head *list,
> 2890 pgoff_t end, unsigned int new_order)
> 2891 {
> 2892 struct folio *folio = page_folio(page);
> 2893 struct page *head = &folio->page;
> 2894 struct lruvec *lruvec;
> 2895 struct address_space *swap_cache = NULL;
> 2896 unsigned long offset = 0;
> 2897 int i, nr_dropped = 0;
> --> 2898 unsigned int new_nr = 1 << new_order;
> ^^^^^^^^^
> The new_order variable comes from the user via debugfs.
>
> 2899 int order = folio_order(folio);
> 2900 unsigned int nr = 1 << order;
> 2901
> 2902 /* complete memcg works before add pages to LRU */
> 2903 split_page_memcg(head, order, new_order);
> 2904
> 2905 if (folio_test_anon(folio) && folio_test_swapcache(folio)) {
> 2906 offset = swp_offset(folio->swap);
> 2907 swap_cache = swap_address_space(folio->swap);
>
> Here is the debugfs code in split_huge_pages_write()
>
> mm/huge_memory.c
> 3628
> 3629 ret = sscanf(input_buf, "%d,0x%lx,0x%lx,%d", &pid, &vaddr_start, &vaddr_end, &new_order);
> ^^^^^^^^^^
> We just read new_order
>
> 3630 if (ret == 1 && pid == 1) {
> 3631 split_huge_pages_all();
> 3632 ret = strlen(input_buf);
> 3633 goto out;
> 3634 } else if (ret != 3 && ret != 4) {
> 3635 ret = -EINVAL;
> 3636 goto out;
> 3637 }
> 3638
> 3639 ret = split_huge_pages_pid(pid, vaddr_start, vaddr_end, new_order);
> ^^^^^^^^^
> And pass it directly with no bounds checking. Debugfs code is root
> only... We used to take a view that if root does something stupid then
> they get what they deserve. But these days syzbot is fuzz testing stuff
> even when it's root only and complaining about shift wraps or other
> undefined behavior. So I feel like it might be easiest to silence this
> undefined behavior warning now instead of waiting for the syzbot reports
> to come back to bite us in a couple years.
Sure. Thank you for reporting this.
Can you check if the patch below fixes the issue? I checked the inputs from
debugfs and also inside split_huge_page_to_list_to_order().
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a81a09236c16..4d21e57a7d07 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3052,6 +3052,9 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+ if (new_order >= folio_order(folio))
+ return -EINVAL;
+
/* Cannot split anonymous THP to order-1 */
if (new_order == 1 && folio_test_anon(folio)) {
VM_WARN_ONCE(1, "Cannot split to order-1 folio");
@@ -3484,6 +3487,9 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
goto next;
total++;
+
+ if (new_order >= folio_order(folio))
+ goto next;
/*
* For folios with private, split_huge_page_to_list_to_order()
* will try to drop it before split and then check if the folio
@@ -3550,6 +3556,9 @@ static int split_huge_pages_in_file(const char *file_path, pgoff_t off_start,
total++;
nr_pages = folio_nr_pages(folio);
+ if (new_order >= folio_order(folio))
+ goto next;
+
if (!folio_trylock(folio))
goto next;
--
Best Regards,
Yan, Zi
Attachment:
signature.asc
Description: OpenPGP digital signature
