Re: [PATCH 18/18] powerpc/fault: Use analyse_instr() to check for store with updates to sp

From: Daniel Axtens <dja@axtens.net>
To: Jordan Niethe <jniethe5@gmail.com>, linuxppc-dev@lists.ozlabs.org
Cc: alistair@popple.id.au, Jordan Niethe <jniethe5@gmail.com>
Subject: Re: [PATCH 18/18] powerpc/fault: Use analyse_instr() to check for store with updates to sp
Date: Thu, 19 Dec 2019 01:11:33 +1100	[thread overview]
Message-ID: <87immdu3je.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20191126052141.28009-19-jniethe5@gmail.com>

Jordan Niethe <jniethe5@gmail.com> writes:

> A user-mode access to an address a long way below the stack pointer is
> only valid if the instruction is one that would update the stack pointer
> to the address accessed. This is checked by directly looking at the
> instructions op-code. As a result is does not take into account prefixed
> instructions. Instead of looking at the instruction our self, use
> analyse_instr() determine if this a store instruction that will update
> the stack pointer.
>
> Something to note is that there currently are not any store with update
> prefixed instructions. Actually there is no plan for prefixed
> update-form loads and stores. So this patch is probably not needed but
> it might be preferable to use analyse_instr() rather than open coding
> the test anyway.

Yes please. I was looking through this code recently and was
horrified. This improves things a lot and I think is justification
enough as-is.

Regards,
Daniel
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 39 +++++++++++----------------------------
>  1 file changed, 11 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index b5047f9b5dec..cb78b3ca1800 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -41,37 +41,17 @@
>  #include <asm/siginfo.h>
>  #include <asm/debug.h>
>  #include <asm/kup.h>
> +#include <asm/sstep.h>
>  
>  /*
>   * Check whether the instruction inst is a store using
>   * an update addressing form which will update r1.
>   */
> -static bool store_updates_sp(unsigned int inst)
> +static bool store_updates_sp(struct instruction_op *op)
>  {
> -	/* check for 1 in the rA field */
> -	if (((inst >> 16) & 0x1f) != 1)
> -		return false;
> -	/* check major opcode */
> -	switch (inst >> 26) {
> -	case OP_STWU:
> -	case OP_STBU:
> -	case OP_STHU:
> -	case OP_STFSU:
> -	case OP_STFDU:
> -		return true;
> -	case OP_STD:	/* std or stdu */
> -		return (inst & 3) == 1;
> -	case OP_31:
> -		/* check minor opcode */
> -		switch ((inst >> 1) & 0x3ff) {
> -		case OP_31_XOP_STDUX:
> -		case OP_31_XOP_STWUX:
> -		case OP_31_XOP_STBUX:
> -		case OP_31_XOP_STHUX:
> -		case OP_31_XOP_STFSUX:
> -		case OP_31_XOP_STFDUX:
> +	if (GETTYPE(op->type) == STORE) {
> +		if ((op->type & UPDATE) && (op->update_reg == 1))
>  			return true;
> -		}
>  	}
>  	return false;
>  }
> @@ -278,14 +258,17 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
>  
>  		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
>  		    access_ok(nip, sizeof(*nip))) {
> -			unsigned int inst;
> +			unsigned int inst, sufx;
> +			struct instruction_op op;
>  			int res;
>  
>  			pagefault_disable();
> -			res = __get_user_inatomic(inst, nip);
> +			res = __get_user_instr_inatomic(inst, sufx, nip);
>  			pagefault_enable();
> -			if (!res)
> -				return !store_updates_sp(inst);
> +			if (!res) {
> +				analyse_instr(&op, uregs, inst, sufx);
> +				return !store_updates_sp(&op);
> +			}
>  			*must_retry = true;
>  		}
>  		return true;
> -- 
> 2.20.1