[PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Tong Tiangen]

We triggered the following error logs in syzkaller test:

  BUG: Bad page state in process syz.7.38  pfn:1eff3
  page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1eff3
  flags: 0x3fffff00004004(referenced|reserved|node=0|zone=1|lastcpupid=0x1fffff)
  raw: 003fffff00004004 ffffe6c6c07bfcc8 ffffe6c6c07bfcc8 0000000000000000
  raw: 0000000000000000 0000000000000000 00000000fffffffe 0000000000000000
  page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
  Call Trace:
   <TASK>
   dump_stack_lvl+0x32/0x50
   bad_page+0x69/0xf0
   free_unref_page_prepare+0x401/0x500
   free_unref_page+0x6d/0x1b0
   uprobe_write_opcode+0x460/0x8e0
   install_breakpoint.part.0+0x51/0x80
   register_for_each_vma+0x1d9/0x2b0
   __uprobe_register+0x245/0x300
   bpf_uprobe_multi_link_attach+0x29b/0x4f0
   link_create+0x1e2/0x280
   __sys_bpf+0x75f/0xac0
   __x64_sys_bpf+0x1a/0x30
   do_syscall_64+0x56/0x100
   entry_SYSCALL_64_after_hwframe+0x78/0xe2

   BUG: Bad rss-counter state mm:00000000452453e0 type:MM_FILEPAGES val:-1

The following syzkaller test case can be used to reproduce:

  r2 = creat(&(0x7f0000000000)='./file0\x00', 0x8)
  write$nbd(r2, &(0x7f0000000580)=ANY=[], 0x10)
  r4 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x42, 0x0)
  mmap$IORING_OFF_SQ_RING(&(0x7f0000ffd000/0x3000)=nil, 0x3000, 0x0, 0x12, r4, 0x0)
  r5 = userfaultfd(0x80801)
  ioctl$UFFDIO_API(r5, 0xc018aa3f, &(0x7f0000000040)={0xaa, 0x20})
  r6 = userfaultfd(0x80801)
  ioctl$UFFDIO_API(r6, 0xc018aa3f, &(0x7f0000000140))
  ioctl$UFFDIO_REGISTER(r6, 0xc020aa00, &(0x7f0000000100)={{&(0x7f0000ffc000/0x4000)=nil, 0x4000}, 0x2})
  ioctl$UFFDIO_ZEROPAGE(r5, 0xc020aa04, &(0x7f0000000000)={{&(0x7f0000ffd000/0x1000)=nil, 0x1000}})
  r7 = bpf$PROG_LOAD(0x5, &(0x7f0000000140)={0x2, 0x3, &(0x7f0000000200)=ANY=[@ANYBLOB="1800000000120000000000000000000095"], &(0x7f0000000000)='GPL\x00', 0x7, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, @fallback=0x30, 0xffffffffffffffff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x10, 0x0, @void, @value}, 0x94)
  bpf$BPF_LINK_CREATE_XDP(0x1c, &(0x7f0000000040)={r7, 0x0, 0x30, 0x1e, @val=@uprobe_multi={&(0x7f0000000080)='./file0\x00', &(0x7f0000000100)=[0x2], 0x0, 0x0, 0x1}}, 0x40)

The cause is that zero pfn is set to the pte without increasing the rss
count in mfill_atomic_pte_zeropage() and the refcount of zero folio does
not increase accordingly. Then, the operation on the same pfn is performed
in uprobe_write_opcode()->__replace_page() to unconditional decrease the
rss count and old_folio's refcount.

Therefore, two bugs are introduced:
1. The rss count is incorrect, when process exit, the check_mm() report
   error "Bad rss-count".
2. The reserved folio (zero folio) is freed when folio->refcount is zero,
   then free_pages_prepare->free_page_is_bad() report error
   "Bad page state".

Considering that uprobe hit the zero folio is a very rare scene, just
reject zero old folio immediately after get_user_page_vma_remote().

Fixes: 7396fa818d62 ("uprobes/core: Make background page replacement logic account for rss_stat counters")
Fixes: 2b1444983508 ("uprobes, mm, x86: Add the ability to install and remove uprobes breakpoints")
Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
v2: Modified according to the comments of David and Oleg. 
---
 kernel/events/uprobes.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 46ddf3a2334d..daa46868faf3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -506,6 +506,11 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	if (ret <= 0)
 		goto put_old;
 
+	if (is_zero_page(old_page)) {
+		ret = -EINVAL;
+		goto put_old;
+	}
+
 	if (WARN(!is_register && PageCompound(old_page),
 		 "uprobe unregister should never work on compound page\n")) {
 		ret = -EINVAL;
-- 
2.25.1

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[David Hildenbrand]

On 21.02.25 02:50, Tong Tiangen wrote:
> We triggered the following error logs in syzkaller test:
> 
>    BUG: Bad page state in process syz.7.38  pfn:1eff3
>    page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1eff3
>    flags: 0x3fffff00004004(referenced|reserved|node=0|zone=1|lastcpupid=0x1fffff)
>    raw: 003fffff00004004 ffffe6c6c07bfcc8 ffffe6c6c07bfcc8 0000000000000000
>    raw: 0000000000000000 0000000000000000 00000000fffffffe 0000000000000000
>    page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
>    Call Trace:
>     <TASK>
>     dump_stack_lvl+0x32/0x50
>     bad_page+0x69/0xf0
>     free_unref_page_prepare+0x401/0x500
>     free_unref_page+0x6d/0x1b0
>     uprobe_write_opcode+0x460/0x8e0
>     install_breakpoint.part.0+0x51/0x80
>     register_for_each_vma+0x1d9/0x2b0
>     __uprobe_register+0x245/0x300
>     bpf_uprobe_multi_link_attach+0x29b/0x4f0
>     link_create+0x1e2/0x280
>     __sys_bpf+0x75f/0xac0
>     __x64_sys_bpf+0x1a/0x30
>     do_syscall_64+0x56/0x100
>     entry_SYSCALL_64_after_hwframe+0x78/0xe2
> 
>     BUG: Bad rss-counter state mm:00000000452453e0 type:MM_FILEPAGES val:-1
> 
> The following syzkaller test case can be used to reproduce:
> 
>    r2 = creat(&(0x7f0000000000)='./file0\x00', 0x8)
>    write$nbd(r2, &(0x7f0000000580)=ANY=[], 0x10)
>    r4 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x42, 0x0)
>    mmap$IORING_OFF_SQ_RING(&(0x7f0000ffd000/0x3000)=nil, 0x3000, 0x0, 0x12, r4, 0x0)
>    r5 = userfaultfd(0x80801)
>    ioctl$UFFDIO_API(r5, 0xc018aa3f, &(0x7f0000000040)={0xaa, 0x20})
>    r6 = userfaultfd(0x80801)
>    ioctl$UFFDIO_API(r6, 0xc018aa3f, &(0x7f0000000140))
>    ioctl$UFFDIO_REGISTER(r6, 0xc020aa00, &(0x7f0000000100)={{&(0x7f0000ffc000/0x4000)=nil, 0x4000}, 0x2})
>    ioctl$UFFDIO_ZEROPAGE(r5, 0xc020aa04, &(0x7f0000000000)={{&(0x7f0000ffd000/0x1000)=nil, 0x1000}})
>    r7 = bpf$PROG_LOAD(0x5, &(0x7f0000000140)={0x2, 0x3, &(0x7f0000000200)=ANY=[@ANYBLOB="1800000000120000000000000000000095"], &(0x7f0000000000)='GPL\x00', 0x7, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, @fallback=0x30, 0xffffffffffffffff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x10, 0x0, @void, @value}, 0x94)
>    bpf$BPF_LINK_CREATE_XDP(0x1c, &(0x7f0000000040)={r7, 0x0, 0x30, 0x1e, @val=@uprobe_multi={&(0x7f0000000080)='./file0\x00', &(0x7f0000000100)=[0x2], 0x0, 0x0, 0x1}}, 0x40)
> 
> The cause is that zero pfn is set to the pte without increasing the rss
> count in mfill_atomic_pte_zeropage() and the refcount of zero folio does
> not increase accordingly. Then, the operation on the same pfn is performed
> in uprobe_write_opcode()->__replace_page() to unconditional decrease the
> rss count and old_folio's refcount.
> 
> Therefore, two bugs are introduced:
> 1. The rss count is incorrect, when process exit, the check_mm() report
>     error "Bad rss-count".
> 2. The reserved folio (zero folio) is freed when folio->refcount is zero,
>     then free_pages_prepare->free_page_is_bad() report error
>     "Bad page state".

Well, there is more, like triggering the

	VM_WARN_ON_FOLIO(is_zero_folio(folio), folio);

in __folio_rmap_sanity_checks() I assume.

So maybe just call the patch

	"uprobes: reject the share zeropage in uprobe_write_opcode)()"

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Tong Tiangen]

在 2025/2/21 16:12, David Hildenbrand 写道:
> On 21.02.25 02:50, Tong Tiangen wrote:
>> We triggered the following error logs in syzkaller test:
>>
>>    BUG: Bad page state in process syz.7.38  pfn:1eff3
>>    page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 
>> pfn:0x1eff3
>>    flags: 
>> 0x3fffff00004004(referenced|reserved|node=0|zone=1|lastcpupid=0x1fffff)
>>    raw: 003fffff00004004 ffffe6c6c07bfcc8 ffffe6c6c07bfcc8 
>> 0000000000000000
>>    raw: 0000000000000000 0000000000000000 00000000fffffffe 
>> 0000000000000000
>>    page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
>>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
>> 1.13.0-1ubuntu1.1 04/01/2014
>>    Call Trace:
>>     <TASK>
>>     dump_stack_lvl+0x32/0x50
>>     bad_page+0x69/0xf0
>>     free_unref_page_prepare+0x401/0x500
>>     free_unref_page+0x6d/0x1b0
>>     uprobe_write_opcode+0x460/0x8e0
>>     install_breakpoint.part.0+0x51/0x80
>>     register_for_each_vma+0x1d9/0x2b0
>>     __uprobe_register+0x245/0x300
>>     bpf_uprobe_multi_link_attach+0x29b/0x4f0
>>     link_create+0x1e2/0x280
>>     __sys_bpf+0x75f/0xac0
>>     __x64_sys_bpf+0x1a/0x30
>>     do_syscall_64+0x56/0x100
>>     entry_SYSCALL_64_after_hwframe+0x78/0xe2
>>
>>     BUG: Bad rss-counter state mm:00000000452453e0 type:MM_FILEPAGES 
>> val:-1
>>
>> The following syzkaller test case can be used to reproduce:
>>
>>    r2 = creat(&(0x7f0000000000)='./file0\x00', 0x8)
>>    write$nbd(r2, &(0x7f0000000580)=ANY=[], 0x10)
>>    r4 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 
>> 0x42, 0x0)
>>    mmap$IORING_OFF_SQ_RING(&(0x7f0000ffd000/0x3000)=nil, 0x3000, 0x0, 
>> 0x12, r4, 0x0)
>>    r5 = userfaultfd(0x80801)
>>    ioctl$UFFDIO_API(r5, 0xc018aa3f, &(0x7f0000000040)={0xaa, 0x20})
>>    r6 = userfaultfd(0x80801)
>>    ioctl$UFFDIO_API(r6, 0xc018aa3f, &(0x7f0000000140))
>>    ioctl$UFFDIO_REGISTER(r6, 0xc020aa00, 
>> &(0x7f0000000100)={{&(0x7f0000ffc000/0x4000)=nil, 0x4000}, 0x2})
>>    ioctl$UFFDIO_ZEROPAGE(r5, 0xc020aa04, 
>> &(0x7f0000000000)={{&(0x7f0000ffd000/0x1000)=nil, 0x1000}})
>>    r7 = bpf$PROG_LOAD(0x5, &(0x7f0000000140)={0x2, 0x3, 
>> &(0x7f0000000200)=ANY=[@ANYBLOB="1800000000120000000000000000000095"], 
>> &(0x7f0000000000)='GPL\x00', 0x7, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 
>> @fallback=0x30, 0xffffffffffffffff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
>> 0x0, 0x0, 0x0, 0x0, 0x10, 0x0, @void, @value}, 0x94)
>>    bpf$BPF_LINK_CREATE_XDP(0x1c, &(0x7f0000000040)={r7, 0x0, 0x30, 
>> 0x1e, @val=@uprobe_multi={&(0x7f0000000080)='./file0\x00', 
>> &(0x7f0000000100)=[0x2], 0x0, 0x0, 0x1}}, 0x40)
>>
>> The cause is that zero pfn is set to the pte without increasing the rss
>> count in mfill_atomic_pte_zeropage() and the refcount of zero folio does
>> not increase accordingly. Then, the operation on the same pfn is 
>> performed
>> in uprobe_write_opcode()->__replace_page() to unconditional decrease the
>> rss count and old_folio's refcount.
>>
>> Therefore, two bugs are introduced:
>> 1. The rss count is incorrect, when process exit, the check_mm() report
>>     error "Bad rss-count".
>> 2. The reserved folio (zero folio) is freed when folio->refcount is zero,
>>     then free_pages_prepare->free_page_is_bad() report error
>>     "Bad page state".
> 
> Well, there is more, like triggering the
> 
>      VM_WARN_ON_FOLIO(is_zero_folio(folio), folio);
> 
> in __folio_rmap_sanity_checks() I assume.
> 
> So maybe just call the patch
> 
>      "uprobes: reject the share zeropage in uprobe_write_opcode)()"
> 
> Thanks!

OK, This subject is more appropriate.

Thanks.

> 

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Oleg Nesterov]

On 02/21, Tong Tiangen wrote:
>
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -506,6 +506,11 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (ret <= 0)
>  		goto put_old;
>
> +	if (is_zero_page(old_page)) {
> +		ret = -EINVAL;
> +		goto put_old;
> +	}

I agree with David, the subject looks a bit misleading.

And. I won't insist, this is cosmetic, but if you send V2 please consider
moving the "verify_opcode()" check down, after the is_zero_page/PageCompound
checks.

Oleg.

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Tong Tiangen]

在 2025/2/21 23:28, Oleg Nesterov 写道:
> On 02/21, Tong Tiangen wrote:
>>
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -506,6 +506,11 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>>   	if (ret <= 0)
>>   		goto put_old;
>>
>> +	if (is_zero_page(old_page)) {
>> +		ret = -EINVAL;
>> +		goto put_old;
>> +	}
> 
> I agree with David, the subject looks a bit misleading.
> 
> And. I won't insist, this is cosmetic, but if you send V2 please consider
> moving the "verify_opcode()" check down, after the is_zero_page/PageCompound
> checks.
> 
> Oleg.

OK, check the validity of the old page first and modify the subject in
v3 .

Thanks.

> 
> 
> .

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Tong Tiangen]

在 2025/2/22 10:37, Tong Tiangen 写道:
> 
> 
> 在 2025/2/21 23:28, Oleg Nesterov 写道:
>> On 02/21, Tong Tiangen wrote:
>>>
>>> --- a/kernel/events/uprobes.c
>>> +++ b/kernel/events/uprobes.c
>>> @@ -506,6 +506,11 @@ int uprobe_write_opcode(struct arch_uprobe 
>>> *auprobe, struct mm_struct *mm,
>>>       if (ret <= 0)
>>>           goto put_old;
>>>
>>> +    if (is_zero_page(old_page)) {
>>> +        ret = -EINVAL;
>>> +        goto put_old;
>>> +    }
>>
>> I agree with David, the subject looks a bit misleading.
>>
>> And. I won't insist, this is cosmetic, but if you send V2 please consider
>> moving the "verify_opcode()" check down, after the 
>> is_zero_page/PageCompound
>> checks.
>>
>> Oleg.
> 
> OK, check the validity of the old page first and modify the subject in
> v3 .
> 
> Thanks.

I'm going to add a new patch to moving the "verify_opcode()" check down
, IIUC that "!PageAnon(old_page)" below also needs to be moved together,
and as David said this can be triggered by user space, so delete the use
  of "WARN", as follows:


@@ -502,20 +502,16 @@ int uprobe_write_opcode(struct arch_uprobe 
*auprobe, struct mm_struct *mm,
         if (IS_ERR(old_page))
                 return PTR_ERR(old_page);

-       ret = verify_opcode(old_page, vaddr, &opcode);
-       if (ret <= 0)
+       ret = -EINVAL;
+       if (is_zero_page(old_page))
                 goto put_old;

-       if (is_zero_page(old_page)) {
-               ret = -EINVAL;
+       if (!is_register && (PageCompound(old_page) || !PageAnon(old_page)))
                 goto put_old;
-       }

-       if (WARN(!is_register && PageCompound(old_page),
-                "uprobe unregister should never work on compound 
page\n")) {
-               ret = -EINVAL;
+       ret = verify_opcode(old_page, vaddr, &opcode);
+       if (ret <= 0)
                 goto put_old;
-       }

         /* We are going to replace instruction, update ref_ctr. */
         if (!ref_ctr_updated && uprobe->ref_ctr_offset) {
@@ -526,10 +522,6 @@ int uprobe_write_opcode(struct arch_uprobe 
*auprobe, struct mm_struct *mm,
                 ref_ctr_updated = 1;
         }

-       ret = 0;
-       if (!is_register && !PageAnon(old_page))
-               goto put_old;
-
         ret = anon_vma_prepare(vma);

Thanks.
> 
>>
>>
>> .
> 
> .

Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace_page()

[Oleg Nesterov]

On 02/22, Tong Tiangen wrote:
>
>
> I'm going to add a new patch to moving the "verify_opcode()" check down
> , IIUC that "!PageAnon(old_page)" below also needs to be moved together,

No, no.

I forgot everything, but please don't do this. IIUC This is optimization
for the case when the probed file has int3 at this offset. We should not
skip update_ref_ctr() in this case, just we can avoid __replace_page().

> and as David said this can be triggered by user space, so delete the use
>  of "WARN", as follows:

Hmm... I think that David meant the new WARN_ON() added by you in V1?

Please don't remove the old WARN(PageCompound(old_page) check. If userspace
can trigger this warning we need to to fix this code and add FOLL_SPLIT_PMD
unconditionally (and likely do something else).

I take my words back ;) Don't do the additional cleanups, just add the
is_zero_page(old_page) check right after get_user_page_vma_remote() and
update the subject/changelog as David suggests.

This function needs more cleanups anyway. Say, the usage of orig_page_huge
_looks_ obviously wrong even if (afaics) nothing bad can happen. It should
be reinitialized after "goto retry" or it should be checked before the
"orig_page = find_get_page()" code. The usage of gup_flags looks confusing
too. Lets do this later.

Oleg.

>
>
> @@ -502,20 +502,16 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe,
> struct mm_struct *mm,
>         if (IS_ERR(old_page))
>                 return PTR_ERR(old_page);
>
> -       ret = verify_opcode(old_page, vaddr, &opcode);
> -       if (ret <= 0)
> +       ret = -EINVAL;
> +       if (is_zero_page(old_page))
>                 goto put_old;
>
> -       if (is_zero_page(old_page)) {
> -               ret = -EINVAL;
> +       if (!is_register && (PageCompound(old_page) || !PageAnon(old_page)))
>                 goto put_old;
> -       }
>
> -       if (WARN(!is_register && PageCompound(old_page),
> -                "uprobe unregister should never work on compound page\n"))
> {
> -               ret = -EINVAL;
> +       ret = verify_opcode(old_page, vaddr, &opcode);
> +       if (ret <= 0)
>                 goto put_old;
> -       }
>
>         /* We are going to replace instruction, update ref_ctr. */
>         if (!ref_ctr_updated && uprobe->ref_ctr_offset) {
> @@ -526,10 +522,6 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe,
> struct mm_struct *mm,
>                 ref_ctr_updated = 1;
>         }
>
> -       ret = 0;
> -       if (!is_register && !PageAnon(old_page))
> -               goto put_old;
> -
>         ret = anon_vma_prepare(vma);
>
> Thanks.
> >
> >>
> >>
> >>.
> >
> >.
>