linux-kernel - Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol

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: <daaec290-2d7c-551a-1129-4b672b7ef739@linaro.org>
Date:	Thu, 12 May 2016 14:08:29 +0100
From:	Daniel Thompson <daniel.thompson@...aro.org>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	patches@...aro.org, linaro-kernel@...ts.linaro.org,
	Jason Wessel <jason.wessel@...driver.com>,
	Dave P Martin <Dave.Martin@....com>,
	Vijaya Kumar K <Vijaya.Kumar@...iumnetworks.com>
Subject: Re: [PATCH] arm64: kgdb: Match pstate size with gdbserver protocol

On 10/05/16 15:31, Mark Rutland wrote:
> On Tue, May 10, 2016 at 02:41:54PM +0100, Daniel Thompson wrote:
>> On 10/05/16 11:45, Mark Rutland wrote:
>>>> When commit 44679a4f142b ("arm64: KGDB: Add step debugging support") was
>>>> introduced it was paired with a gdb patch that made an incompatible
>>>> change to the gdbserver protocol. This patch was eventually merged into
>>>> the gdb sources:
>>>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
>>>>
>>>> The change to the protocol was mostly made to simplify big-endian support
>>>> inside the kernel gdb stub.
>>>
>>> While that was how we discovered the inconsistency, a major concern is
>>> that SPSR_EL* (i.e. PSTATE), as accessed by MRS/MSR is a 64-bit
>>> quantity, even if the upper 32 bits are RES0 today.
>>>
>>> It is conceivable that the upper 32 bits could be used in future (as
>>> happened with CLIDR_EL1), and for this reason we expose those upper 32
>>> bits from the kernel, and treat system registers as 64-bit quantities
>>> generally.
>>
>> These cases are not exactly the same.
>>
>> CLIDR_ELx is (or was) architecturally defined as a 64-bit register
>> and explicitly marks the upper 32-bits as RES0.
>>
>> That is not the case for SPSR_ELx; this register is architecturally
>> defined to be 32-bit.
>
> The below doesn't necessarily change your subsequent argument, but that
> isn't quite true.
>
> The two cases are in fact identical if you dig into the history a bit
> further. Take a look in an earlier revision of the ARM ARM (e.g. ARM DDI
> 0487A.b), where it was explicitly stated:
>
> 	Attributes
> 		CLIDR_EL1 is a 32-bit register.

I see. I only when back as far as A.e ...


> Based on this, my PoV is that any register that the ARM ARM describes as
> "a 32-bit register" is a 64-bit register for which the upper 32 bits are
> RES0.

Understood.

>> I know that doesn't *prevent* SPSR_ELx from being expanded in the
>> future it is not unreasonable for gdb to design its wire protocol
>> based on the description found in the architecture manual.
>>
>>> So this was also about ensuring the interface was consistent and to some
>>> extent future-proof.
>>
>> gdb remote protocol is already future proof and has never at any
>> point contradicted the architecture.
>>
>> However the changes to the protocol in 7.7.x and 7.8.0 were
>> analogous to an unexpected ABI change rather than a carefully
>> controlled introduction of a new feature. Like the kernel, once
>> detected, they were reversed ;-) .
>
> To be clear, I don't disagree with this. The ARM ARM is at best
> amigibious w.r.t. what it means by "a 32-bit register", and that's the
> only point of contention.

Agree... such ambiguity strikes me as a particularly serious problem for 
switchable context registers since it takes *very* close attention to 
the ARM ARM for an existing OS to determine that it is required to treat 
the upper 32-bits of context should-be-zero-or-preserved.


> However, we should make note of the above as a key point of rationale,
> as it affects other decisions we make in this area.

Ok. I'll add comments describing how the gdbremote protocol is 
interpreting the spec.


Daniel.