linux-kernel - Re: [PATCH 1/3] ARM64: perf: add support for perf registers API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 15 Jan 2014 11:30:48 +0100
From:	Jean Pihet <jean.pihet@...aro.org>
To:	Will Deacon <will.deacon@....com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
	Arnaldo <acme@...stprotocols.net>,
	"patches@...aro.org" <patches@...aro.org>,
	Jean Pihet <jean.pihet@...oldbits.com>
Subject: Re: [PATCH 1/3] ARM64: perf: add support for perf registers API

Hi Will,

On 6 January 2014 19:30, Will Deacon <will.deacon@....com> wrote:
> Hi Jean,
>
> Thanks for the updated patches. One minor comment on this one.
>
> On Mon, Dec 30, 2013 at 04:25:30PM +0000, Jean Pihet wrote:
>> From: Jean Pihet <jean.pihet@...oldbits.com>
>>
>> This patch implements the functions required for the perf registers API,
>> allowing the perf tool to interface kernel register dumps with libunwind
>> in order to provide userspace backtracing.
>> Compat mode is also supported.
>
> [...]
>
>> +u64 perf_reg_value(struct pt_regs *regs, int idx)
>> +{
>> +     if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
>> +             return 0;
>
> While this is probably fine, I'd feel more comfortable if you had separate
> limit checks for native and compat...
In fact in the native and compat modes the same set of registers are
accessed, based on the native regs that are registered to the perf
event core, cf. the definition of PERF_REGS_MASK in
tools/perf/arch/arm64/include/perf_regs.h.

The regs set could be registered differently based on the binary to
trace, but unfortunately the perf core code does not allow that.

I would leave the code as is, what do you think?

Cheers,
Jean

>
>> +     /*
>> +      * Compat (i.e. 32 bit) mode:
>> +      * - PC has been set in the pt_regs struct in kernel_entry,
>> +      * - Handle SP and LR here.
>> +      */
>> +     if (compat_user_mode(regs)) {
>
> i.e. have a WARN_ON_ONCE here for the compat structure size.
>
>> +             if ((u32)idx == PERF_REG_ARM64_SP)
>> +                     return regs->compat_sp;
>> +             if ((u32)idx == PERF_REG_ARM64_LR)
>> +                     return regs->compat_lr;
>> +     }
>
> then stick an else here with the original check.
>
> Make sense?
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/