linux-kernel - Re: [PATCH 5/8] arm64: kprobes instruction simulation support

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]

Message-ID: <56CDCA6B.7000207@arm.com>
Date:	Wed, 24 Feb 2016 15:21:15 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	David Long <dave.long@...aro.org>
Cc:	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>,
	William Cohen <wcohen@...hat.com>,
	Pratyush Anand <panand@...hat.com>,
	Steve Capper <steve.capper@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Dave P Martin <Dave.Martin@....com>,
	Mark Rutland <mark.rutland@....com>,
	Robin Murphy <Robin.Murphy@....com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Jens Wiklander <jens.wiklander@...aro.org>,
	Christoffer Dall <christoffer.dall@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Yang Shi <yang.shi@...aro.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	"Suzuki K. Poulose" <suzuki.poulose@....com>,
	Kees Cook <keescook@...omium.org>,
	Zi Shen Lim <zlim.lnx@...il.com>,
	John Blackwood <john.blackwood@...r.com>,
	Feng Kan <fkan@....com>,
	Balamurugan Shanmugam <bshanmugam@....com>,
	James Morse <james.morse@....com>,
	Vladimir Murzin <Vladimir.Murzin@....com>,
	Mark Salyzyn <salyzyn@...roid.com>,
	Petr Mladek <pmladek@...e.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 5/8] arm64: kprobes instruction simulation support

On 24/02/16 15:07, David Long wrote:
> On 02/24/2016 04:05 AM, Marc Zyngier wrote:
>> On Wed, 24 Feb 2016 01:56:52 -0500
>> David Long <dave.long@...aro.org> wrote:
>>
>>> On 02/19/2016 09:04 AM, Marc Zyngier wrote:
>>>> Hi David,
>>>>
>>>> On 18/02/16 23:48, David Long wrote:
>>>>> From: Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
>>>>>
>>>>> Kprobes needs simulation of instructions that cannot be stepped
>>>>> from different memory location, e.g.: those instructions
>>>>> that uses PC-relative addressing. In simulation, the behaviour
>>>>> of the instruction is implemented using a copy of pt_regs.
>>>>>
>>>>> Following instruction catagories are simulated:
>>>>>    - All branching instructions(conditional, register, and immediate)
>>>>>    - Literal access instructions(load-literal, adr/adrp)
>>>>>
>>>>> Conditional execution is limited to branching instructions in
>>>>> ARM v8. If conditions at PSTATE do not match the condition fields
>>>>> of opcode, the instruction is effectively NOP. Kprobes considers
>>>>> this case as 'miss'.
>>>>>
>>>>> This code also replaces the use of arch/arm/opcodes.c for
>>>>> arm_check_condition().
>>>>>
>>>>> Thanks to Will Cohen for assorted suggested changes.
>>>>>
>>>>> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
>>>>> Signed-off-by: William Cohen <wcohen@...hat.com>
>>>>> Signed-off-by: David A. Long <dave.long@...aro.org>
>>
>> [...]
>>
>>>>> +};
>>>>> +
>>>>> +asmlinkage unsigned int __kprobes arm_check_condition(u32 opcode, u32 psr)
>>>>
>>>> Why asmlinkage? This function is never called from assembly code on arm64.
>>>>
>>>
>>> This comes from the 32-bit ARM code that tests the condition from
>>> entry.S.  We include arch/arm/include/asm/opcodes.h in
>>> arch/arm64/include/asm/opcodes.h so it gets declared there with
>>> asmlinkage. I can remove the asmlinkage in the actual function
>>> definition and it still compiles but I'm not sure that is kosher.
>>
>> asmlinkage is only meaningful if you're calling it from assembly code.
>> As you seem to only call it from C code, having asmlinkage is both
>> pointless and confusing.
>>
>>> Will Deacon was advocating getting rid of the include of the 32-bit header
>>> file but it looked to me like this would mean a lot of duplicated
>>> defines and the work would be mostly unrelated to kprobes.
>>
>> Arguably, arm_check_condition() (which only matters to 32bit code,
>> hence userspace) is also completely unrelated to kprobes. I still think
>> Will's point stands.
>>
> 
> Yes, I would not argue about that cross-architecture include needing to 
> be fixed.  Can I assume you agree that need not be a part of this 
> kprobes patch though, nor a prerequisite patch for it?

My usual stand on this kind of issue is that if you start digging in the
middle of the road, you're also responsible for cleaning the pavement
(sorry if that's a bad analogy).

In the end, this is the arm64 maintainers that decide what they want to
see, and by the look of it, Will has made his point clear...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...