diff mbox series

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

Message ID 20210420165001.3790670-3-Liam.Howlett@Oracle.com (mailing list archive)
State New
Headers show
Series [1/3] arm64: armv8_deprecated: Fix swp_handler() signal generation | expand

Commit Message

Liam Howlett April 20, 2021, 4:50 p.m. UTC
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(-)

Comments

Will Deacon April 22, 2021, 12:53 p.m. UTC | #1
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.

Will
Liam Howlett April 22, 2021, 6:56 p.m. UTC | #2
* 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
diff mbox series

Patch

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;