Re: [PATCH 3/3] arch/arm64/kernel/traps: Use find_vma_intersection() in traps for setting si_code

From: Liam Howlett <liam.howlett@oracle.com>
To: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Julien Grall <julien.grall@arm.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: [PATCH 3/3] arch/arm64/kernel/traps: Use find_vma_intersection() in traps for setting si_code
Date: Thu, 22 Apr 2021 18:56:17 +0000	[thread overview]
Message-ID: <20210422185611.ccdf3rm4zr3xtuzl@revolver> (raw)
In-Reply-To: <20210422125334.GB1521@willie-the-truck>

* Will Deacon <will@kernel.org> [210422 08:53]:
> On Tue, Apr 20, 2021 at 04:50:13PM +0000, Liam Howlett wrote:
> > find_vma() will continue to search upwards until the end of the virtual
> > memory space.  This means the si_code would almost never be set to
> > SEGV_MAPERR even when the address falls outside of any VMA.  The result
> > is that the si_code is not reliable as it may or may not be set to the
> > correct result, depending on where the address falls in the address
> > space.
> > 
> > Using find_vma_intersection() allows for what is intended by only
> > returning a VMA if it falls within the range provided, in this case a
> > window of 1.
> > 
> > Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm)
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > ---
> >  arch/arm64/kernel/traps.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index a05d34f0e82a..a44007904a64 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -383,9 +383,10 @@ void force_signal_inject(int signal, int code, unsigned long address, unsigned i
> >  void arm64_notify_segfault(unsigned long addr)
> >  {
> >  	int code;
> > +	unsigned long ut_addr = untagged_addr(addr);
> >  
> >  	mmap_read_lock(current->mm);
> > -	if (find_vma(current->mm, untagged_addr(addr)) == NULL)
> > +	if (find_vma_intersection(current->mm, ut_addr, ut_addr + 1) == NULL)
> >  		code = SEGV_MAPERR;
> >  	else
> >  		code = SEGV_ACCERR;
> 
> I'm not seeing how this addresses VM_GROWSDOWN as Catalin mentioned before.

Sorry for not including the link before, but for context, I've added the
URL for the lore conversation from before at the end of my message [1].

I thought this was resolved by the fact that the stack expansion would
have already taken care of the VM_GROWSDOWN code path?  Did I
misunderstand what was said?

I've modified the other paths to this function to avoid causing issues
elsewhere and to hopefully do the necessary cleanup that Catalin said
needed to be tidied.

Link:  https://lore.kernel.org/lkml/20210413180030.GA31164@arm.com/

Thanks,
Liam