| Message ID | 20240115100731.91007-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] uprobes: use pagesize-aligned virtual address when replacing pages | expand |
On 01/15, David Hildenbrand wrote: > > --- 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); Acked-by: Oleg Nesterov <oleg@redhat.com>
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@kernel.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@kernel.org> > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Alexander Shishkin > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Ian Rogers <irogers@google.com> > Cc: Adrian Hunter <adrian.hunter@intel.com> > Signed-off-by: David Hildenbrand <david@redhat.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 > >
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:
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@kernel.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@kernel.org> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Shishkin Cc: Jiri Olsa <jolsa@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Ian Rogers <irogers@google.com> Cc: Adrian Hunter <adrian.hunter@intel.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- kernel/events/uprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)