Path: csiph.com!aioe.org!bofh.it!news.nic.it!robomod
From: Daniel Thompson <daniel.thompson@linaro.org>
Newsgroups: linux.kernel
Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support
Date: Fri, 29 Jul 2016 11:10:01 +0200
Message-ID: <s0bXP-2EU-7@gated-at.bofh.it>
References: <rX2el-3Bs-1@gated-at.bofh.it> <rXpaW-1tK-33@gated-at.bofh.it> <rXpXk-23K-13@gated-at.bofh.it> <rXr33-2Nx-1@gated-at.bofh.it> <rXFIJ-4tz-3@gated-at.bofh.it> <rXL1M-7Ib-9@gated-at.bofh.it> <rYRHP-7ix-1@gated-at.bofh.it> <rYWxP-25U-1@gated-at.bofh.it> <rZvFf-7n2-5@gated-at.bofh.it> <rZFlg-5mX-17@gated-at.bofh.it> <rZUNj-7Ak-1@gated-at.bofh.it>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: robomod@news.nic.it
Approved: robomod@news.nic.it
Lines: 94
Organization: linux.* mail to news gateway
Xref: csiph.com linux.kernel:1452335

On 28/07/16 15:40, Catalin Marinas wrote:
> On Wed, Jul 27, 2016 at 06:13:37PM -0400, David Long wrote:
>> On 07/27/2016 07:50 AM, Daniel Thompson wrote:
>>> On 25/07/16 23:27, David Long wrote:
>>>> On 07/25/2016 01:13 PM, Catalin Marinas wrote:
>>>>> The problem is that the original design was done on x86 for its PCS and
>>>>> it doesn't always fit other architectures. So we could either ignore the
>>>>> problem, hoping that no probed function requires argument passing on
>>>>> stack or we copy all the valid data on the kernel stack:
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kprobes.h
>>>>> b/arch/arm64/include/asm/kprobes.h
>>>>> index 61b49150dfa3..157fd0d0aa08 100644
>>>>> --- a/arch/arm64/include/asm/kprobes.h
>>>>> +++ b/arch/arm64/include/asm/kprobes.h
>>>>> @@ -22,7 +22,7 @@
>>>>>
>>>>>  #define __ARCH_WANT_KPROBES_INSN_SLOT
>>>>>  #define MAX_INSN_SIZE            1
>>>>> -#define MAX_STACK_SIZE            128
>>>>> +#define MAX_STACK_SIZE            THREAD_SIZE
>>>>>
>>>>>  #define flush_insn_slot(p)        do { } while (0)
>>>>>  #define kretprobe_blacklist_size    0
>>>>
>>>> I doubt the ARM PCS is unusual.  At any rate I'm certain there are other
>>>> architectures that pass aggregate parameters on the stack. I suspect
>>>> other RISC(-ish) architectures have similar PCS issues and I think this
>>>> is at least a big part of where this simple copy with a 64/128 limit
>>>> comes from, or at least why it continues to exist.  That said, I'm not
>>>> enthusiastic about researching that assertion in detail as it could be
>>>> time consuming.
>>>
>>> Given Mark shared a test program I *was* curious enough to take a look
>>> at this.
>>>
>>> The only architecture I can find that behaves like arm64 with the
>>> implicit pass-by-reference described by Catalin/Mark is sparc64.
>>>
>>> In contrast alpha, arm (32-bit), hppa64, mips64 and powerpc64 all use a
>>> hybrid approach where the first fragments of the structure are passed in
>>> registers and the remainder on the stack.
>>
>> That's interesting.  It also looks like sparc64 does not copy any stack for
>> jprobes. I guess that approach at least makes it clear what will and won't
>> work.
>
> I suggest we do the same for arm64 - avoid the copying entirely as it's
> not safe anyway. We don't know how much to copy, nor can we be sure it
> is safe (see Dave's DMA to the stack example). This would need to be
> documented in the kprobes.txt file and MAX_STACK_SIZE removed from the
> arm64 kprobes support.
>
> There is also the case that Daniel was talking about - passing more than
> 8 arguments. I don't think it's worth handling this

Its actually quite hard to document the (architecture specific) "no big 
structures" *and* the "8 argument" limits. It ends up as something like:

   Structures/unions >16 bytes must not be passed by value and the
   size of all arguments, after padding each to an 8 byte boundary, must
   be less than 64 bytes.

We cannot avoid tackling big structures through documentation but when 
we impose additional limits like "only 8 arguments" we are swapping an 
architecture neutral "gotcha" that affects almost all jprobes uses (and 
can be inferred from the documentation) with an architecture specific one!


 > but we should at
> least add a warning and skip the probe:
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index bf9768588288..84e02606ec3d 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -491,6 +491,10 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  	long stack_ptr = kernel_stack_pointer(regs);
>
> +	/* do not allow arguments passed on the stack */
> +	if (WARN_ON_ONCE(regs->sp != regs->regs[29]))
> +		return 0;
> +

I don't really understand this test.

If we could reliably assume that the frame record was at the lowest 
address within a stack frame then we could exploit that to store the 
stacked arguments without risking overwriting volatile variables on the 
stack.


Daniel.