Re: [bug report] mm: huge_memory: enable debugfs to split huge pages to any order

[Zi Yan <ziy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 4473 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]