Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
User-Agent: Cyrus-JMAP/3.3.0-dev0-464-g810d66a-fmstable-20200518v1
Mime-Version: 1.0
Message-Id: <a9a7f3d5-6c9b-4c2c-848e-2d61971ae37b@www.fastmail.com>
In-Reply-To: <20200519150204.GG1551@shell.armlinux.org.uk>
References: <20200519143132.603579-1-andrew@aj.id.au>
 <20200519150204.GG1551@shell.armlinux.org.uk>
Date:   Wed, 20 May 2020 10:14:23 +0930
From:   "Andrew Jeffery" <andrew@aj.id.au>
To:     "Russell King" <linux@armlinux.org.uk>
Cc:     linux-arm-kernel@lists.infradead.org, oleg@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: Implement functions for HAVE_FUNCTION_ARG_ACCESS_API
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Russell,

On Wed, 20 May 2020, at 00:32, Russell King - ARM Linux admin wrote:
> On Wed, May 20, 2020 at 12:01:32AM +0930, Andrew Jeffery wrote:
> > This allows extraction of kernel function arguments via kprobes on A=
RM.
> > Based on the arm64 implementation and adapted for the 32-bit AAPCS.
> >=20
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > The description for HAVE_FUNCTION_ARG_ACCESS_API was pretty vague on=
 what was
> > required. I've implemented enough to enable argument extraction for =
kprobes; is
> > there anything else needed to satisfy HAVE_FUNCTION_ARG_ACCESS_API?
>=20
> What about 64-bit arguments?  How do they get handled?
>=20
> regs_get_kernel_argument() talks about 'n' being the argument number,
> and maps this directly to a register.  If a function argument
> prototype is:
>=20
> 	(something *foo, long long bar, int baz)
>=20
> The foo is in r0, bar is in r2/r3 on EABI, and baz is on the stack.
>=20
> n=3D0 will return foo.  n=3D1 will be undefined.  n=3D2 will return pa=
rt of
> bar, and n=3D3 will return the other half.  Is this what is expected?

Certainly doesn't feel right, however the broken behaviour seems to be a=

compromise accepted in the existing couple of implementations for arm64
and x86. The API prototype doesn't give us any type information, so we'r=
e
left to guess.

Here's the commentary from the others:

arch/x86/include/asm/ptrace.h:
```
/**
 * regs_get_kernel_argument() - get Nth function argument in kernel
 * @regs:       pt_regs of that context
 * @n:          function argument number (start from 0)
 *
 * regs_get_argument() returns @n th argument of the function call.
 * Note that this chooses most probably assignment, in some case
 * it can be incorrect.
 * This is expected to be called from kprobes or ftrace with regs
 * where the top of stack is the return address.
 */
static inline unsigned long regs_get_kernel_argument(struct pt_regs *reg=
s,
                                                     unsigned int n)
{
...
```

arch/arm64/include/asm/ptrace.h:
```
/**
 * regs_get_kernel_argument() - get Nth function argument in kernel
 * @regs:       pt_regs of that context
 * @n:          function argument number (start from 0)
 *
 * regs_get_argument() returns @n th argument of the function call.
 *
 * Note that this chooses the most likely register mapping. In very rare=

 * cases this may not return correct data, for example, if one of the
 * function parameters is 16 bytes or bigger. In such cases, we cannot
 * get access the parameter correctly and the register assignment of
 * subsequent parameters will be shifted.
 */
static inline unsigned long regs_get_kernel_argument(struct pt_regs *reg=
s,
                                                     unsigned int n)
{
...
```

As for handling arguments on the stack, arm64 doesn't and I cribbed from=

that:

```
static inline unsigned long regs_get_kernel_argument(struct pt_regs *reg=
s,
                                                     unsigned int n)
{
#define NR_REG_ARGUMENTS 8
        if (n < NR_REG_ARGUMENTS)
                return pt_regs_read_reg(regs, n);
        return 0;
}
```

Would you accept a v2 that adds stack argument handling but leaves the
=C2=AF\_(=E3=83=84)_/=C2=AF ABI behaviour? Or will I need to fix the API=
?

Thanks for the quick response.

Andrew