Re: [PATCH v13 10/20] kernel, arm64: untag user pointers in prctl_set_mm*

From: Andrey Konovalov <andreyknvl@google.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: "Will Deacon" <will.deacon@arm.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Kees Cook" <keescook@chromium.org>,
	"Kate Stewart" <kstewart@linuxfoundation.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Ingo Molnar" <mingo@kernel.org>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Vincenzo Frascino" <vincenzo.frascino@arm.com>,
	"Eric Dumazet" <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>,
	"David (ChunMing) Zhou" <David1.Zhou@amd.com>,
	"Yishai Hadas" <yishaih@mellanox.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Jens Wiklander" <jens.wiklander@linaro.org>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"Linux ARM" <linux-arm-kernel@lists.infradead.org>,
	"Linux Memory Management List" <linux-mm@kvack.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-rdma@vger.kernel.org, linux-media@vger.kernel.org,
	kvm@vger.kernel.org,
	"open list:KERNEL SELFTEST FRAMEWORK"
	<linux-kselftest@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Dmitry Vyukov" <dvyukov@google.com>,
	"Kostya Serebryany" <kcc@google.com>,
	"Evgeniy Stepanov" <eugenis@google.com>,
	"Lee Smith" <Lee.Smith@arm.com>,
	"Ramana Radhakrishnan" <Ramana.Radhakrishnan@arm.com>,
	"Jacob Bramley" <Jacob.Bramley@arm.com>,
	"Ruben Ayrapetyan" <Ruben.Ayrapetyan@arm.com>,
	"Chintan Pandya" <cpandya@codeaurora.org>,
	"Luc Van Oostenryck" <luc.vanoostenryck@gmail.com>,
	"Dave Martin" <Dave.Martin@arm.com>,
	"Kevin Brodsky" <kevin.brodsky@arm.com>,
	"Szabolcs Nagy" <Szabolcs.Nagy@arm.com>
Subject: Re: [PATCH v13 10/20] kernel, arm64: untag user pointers in prctl_set_mm*
Date: Mon, 1 Apr 2019 18:44:34 +0200	[thread overview]
Message-ID: <CAAeHK+yHp27eT+wTE3Uy4DkN8XN3ZjHATE+=HgjgRjrHjiXs3Q@mail.gmail.com> (raw)
In-Reply-To: <20190322154136.GP13384@arrakis.emea.arm.com>

On Fri, Mar 22, 2019 at 4:41 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, Mar 20, 2019 at 03:51:24PM +0100, Andrey Konovalov wrote:
> > @@ -2120,13 +2135,14 @@ static int prctl_set_mm(int opt, unsigned long addr,
> >       if (opt == PR_SET_MM_AUXV)
> >               return prctl_set_auxv(mm, addr, arg4);
> >
> > -     if (addr >= TASK_SIZE || addr < mmap_min_addr)
> > +     if (untagged_addr(addr) >= TASK_SIZE ||
> > +                     untagged_addr(addr) < mmap_min_addr)
> >               return -EINVAL;
> >
> >       error = -EINVAL;
> >
> >       down_write(&mm->mmap_sem);
> > -     vma = find_vma(mm, addr);
> > +     vma = find_vma(mm, untagged_addr(addr));
> >
> >       prctl_map.start_code    = mm->start_code;
> >       prctl_map.end_code      = mm->end_code;
>
> Does this mean that we are left with tagged addresses for the
> mm->start_code etc. values? I really don't think we should allow this,
> I'm not sure what the implications are in other parts of the kernel.
>
> Arguably, these are not even pointer values but some address ranges. I
> know we decided to relax this notion for mmap/mprotect/madvise() since
> the user function prototypes take pointer as arguments but it feels like
> we are overdoing it here (struct prctl_mm_map doesn't even have
> pointers).
>
> What is the use-case for allowing tagged addresses here? Can user space
> handle untagging?

I don't know any use cases for this. I did it because it seems to be
covered by the relaxed ABI. I'm not entirely sure what to do here,
should I just drop this patch?

>
> --
> Catalin