linux-kernel - Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in __replace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250222123952.GA17836@redhat.com>
Date: Sat, 22 Feb 2025 13:39:53 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Tong Tiangen <tongtiangen@...wei.com>
Cc: David Hildenbrand <david@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, Peter Xu <peterx@...hat.com>,
	Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, linux-trace-kernel@...r.kernel.org,
	bpf@...r.kernel.org, wangkefeng.wang@...wei.com,
	Guohanjun <guohanjun@...wei.com>
Subject: Re: [PATCH -next v2] uprobes: fix two zero old_folio bugs in
 __replace_page()

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.
> >
> >>
> >>
> >>.
> >
> >.
>