linux-kernel - Re: [PATCH v1] uprobes: use pagesize-aligned virtual address when replacing pages

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Wed, 17 Jan 2024 15:02:20 +0100
From: Jiri Olsa <olsajiri@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	linux-perf-users@...r.kernel.org,
	Ryan Roberts <ryan.roberts@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>, bpf@...r.kernel.org
Subject: Re: [PATCH v1] uprobes: use pagesize-aligned virtual address when
 replacing pages

cc-ing bpf list

jirka

On Mon, Jan 15, 2024 at 11:07:31AM +0100, David Hildenbrand wrote:
> uprobes passes an unaligned page mapping address to
> folio_add_new_anon_rmap(), which ends up triggering a VM_BUG_ON() we
> recently extended in commit 372cbd4d5a066 ("mm: non-pmd-mappable, large
> folios for folio_add_new_anon_rmap()").
> 
> Arguably, this is uprobes code doing something wrong; however,
> for the time being it would have likely worked in rmap code because
> __folio_set_anon() would set folio->index to the same value.
> 
> Looking at __replace_page(), we'd also pass slightly wrong values to
> mmu_notifier_range_init(), page_vma_mapped_walk(), flush_cache_page(),
> ptep_clear_flush() and set_pte_at_notify(). I suspect most of them are
> fine, but let's just mark the introducing commit as the one needed
> fixing. I don't think CC stable is warranted.
> 
> We'll add more sanity checks in rmap code separately, to make sure that
> we always get properly aligned addresses.
> 
> Reported-by: Jiri Olsa <jolsa@...nel.org>
> Closes: https://lkml.kernel.org/r/ZaMR2EWN-HvlCfUl@krava
> Fixes: c517ee744b96 ("uprobes: __replace_page() should not use page_address_in_vma()")
> Tested-by: Jiri Olsa <jolsa@...nel.org>
> Reviewed-by: Ryan Roberts <ryan.roberts@....com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Alexander Shishkin
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Ian Rogers <irogers@...gle.com>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  kernel/events/uprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 485bb0389b488..929e98c629652 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -537,7 +537,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  		}
>  	}
>  
> -	ret = __replace_page(vma, vaddr, old_page, new_page);
> +	ret = __replace_page(vma, vaddr & PAGE_MASK, old_page, new_page);
>  	if (new_page)
>  		put_page(new_page);
>  put_old:
> -- 
> 2.43.0
> 
>