From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
To: Srinath Parvathaneni <srinath.parvathaneni@arm.com>,
newlib@sourceware.org
Cc: nd@arm.com, richard.earnshaw@arm.com
Subject: Re: [PATCH][Newlib] arm: Restrict processor mode change when in hypervisor mode.
Date: Thu, 1 Dec 2022 13:55:55 +0000 [thread overview]
Message-ID: <2a819813-159b-5987-d9d3-c907aa7a5728@foss.arm.com> (raw)
In-Reply-To: <be861f45-34e3-448e-81d1-874dc5050d3e@AZ-NEU-EX04.Arm.com>
On 30/11/2022 16:49, Srinath Parvathaneni wrote:
> Hi All,
>
> In _stack_init function of crt0.S file, when the current mode is not user mode,
> all the processor modes are parsed and the corresponding stack limit are set for
> these modes for all A-profile and R-profile CPU's. But when the current processor
> mode is hypervisor mode, changing to any other mode using CPSR will result in an
> illegal instruction as per Arm-arm and simulator throws undefined instruction
> exception.
So under what circumstances can a program be started while in HYP mode?
I may have got the wrong end of the stick (sorry, virtualization isn't
my forte), but isn't the whole point of HYP mode trapping these
attempted mode changes meant to allow the supervising OS to fake up the
relevant behaviours?
> This patch prevent the change of hypervisor mode to any other mode in _stack_init
> function in crt0.S files.
>
> Regression tested on arm-none-eabi target for newlib and newlib-nano and found
> no regressions.
>
> Ok for newlib master?
>
> Regards,
> Srinath.
>
> libgloss/ChangeLog:
>
> 2022-11-28 Srinath Parvathaneni <srinath.parvathaneni@arm.com>
>
> * arm/crt0.S (_stack_init): Add check for hypervisor mode.
>
> newlib/ChangeLog:
>
> 2022-11-28 Srinath Parvathaneni <srinath.parvathaneni@arm.com>
>
> * libc/sys/arm/crt0.S (_stack_init): Add check for hypervisor mode.
>
>
> ############### Attachment also inlined for ease of reply ###############
>
>
> diff --git a/libgloss/arm/crt0.S b/libgloss/arm/crt0.S
> index 78515180bf06f1da37669e4c7e6608c76e1b096d..e3c0ca00fd5d25e8059be07a0fb62490350b4dcb 100644
> --- a/libgloss/arm/crt0.S
> +++ b/libgloss/arm/crt0.S
> @@ -148,13 +148,17 @@
> /* M profile doesn't have CPSR register. */
> #if (__ARM_ARCH_PROFILE != 'M')
> /* Following code is compatible for both ARM and Thumb ISA. */
> - mrs r4, CPSR
> - /* Test mode bits - in User of all are 0. */
> - tst r4, #(CPSR_M_MASK)
> + mrs r4, CPSR
You've (incorrectly) changed the hard tab after 'mrs' to spaces. Please
fix this and all the other cases where you've done this. By convention
we have one hard tab after the mnemonic before the first operand.
> + mov r3, sp /* Save input SP value. */
> + /* Test mode bits - in User mode all are 0. */
> + ands r1, r4, #(CPSR_M_MASK)
> /* "eq" means r4 AND #0x0F is 0. */
> beq .Lskip_cpu_modes
>
> - mov r3, sp /* Save input SP value. */
> + /* Test mode bits - in Hypervisor Mode value is 0X0A. */
> + cmp r1, #(CPSR_M_HYP)
> + /* "eq" means hypervisor mode and change of mode is not acceptable. */
> + beq .Lskip_cpu_hyp_mode
Why the new label rather than jumping to skip_cpu_modes? This doesn't
feel right as I'd expect us to want to take exactly the same path as
when we enter in user mode.
>
> /* FIQ mode, interrupts disabled. */
> mov r1, #(CPSR_M_FIQ|CPSR_M_32BIT|CPSR_I_MASK|CPSR_F_MASK)
> @@ -236,6 +240,7 @@
> sub sl, r3, #64 << 10
> #endif
> #endif
> +.Lskip_cpu_hyp_mode:
>
> FN_RETURN
> FN_EH_END
> diff --git a/newlib/libc/sys/arm/crt0.S b/newlib/libc/sys/arm/crt0.S
> index 6b01d8a88b77f44b1ba495aa1f69156f12749527..b974c36c92c8f092643f63d2056927a107a1e85b 100644
> --- a/newlib/libc/sys/arm/crt0.S
> +++ b/newlib/libc/sys/arm/crt0.S
> @@ -148,13 +148,17 @@
> /* M profile doesn't have CPSR register. */
> #if (__ARM_ARCH_PROFILE != 'M')
> /* Following code is compatible for both ARM and Thumb ISA. */
> - mrs r4, CPSR
> - /* Test mode bits - in User of all are 0. */
> - tst r4, #(CPSR_M_MASK)
> + mrs r4, CPSR
> + mov r3, sp /* Save input SP value. */
> + /* Test mode bits - in User mode all are 0. */
> + ands r1, r4, #(CPSR_M_MASK)
> /* "eq" means r4 AND #0x0F is 0. */
> - beq .Lskip_cpu_modes
> + beq .Lskip_cpu_modes
>
> - mov r3, sp /* Save input SP value. */
> + /* Test mode bits - in Hypervisor Mode value is 0X0A. */
> + cmp r1, #(CPSR_M_HYP)
> + /* "eq" means hypervisor mode and change of mode is not acceptable. */
> + beq .Lskip_cpu_hyp_mode
>
> /* FIQ mode, interrupts disabled. */
> mov r1, #(CPSR_M_FIQ|CPSR_M_32BIT|CPSR_I_MASK|CPSR_F_MASK)
> @@ -236,6 +240,7 @@
> sub sl, r3, #64 << 10
> #endif
> #endif
> +.Lskip_cpu_hyp_mode:
>
> FN_RETURN
> FN_EH_END
R.
prev parent reply other threads:[~2022-12-01 13:56 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-30 16:49 Srinath Parvathaneni
2022-12-01 13:55 ` Richard Earnshaw [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2a819813-159b-5987-d9d3-c907aa7a5728@foss.arm.com \
--to=richard.earnshaw@foss.arm.com \
--cc=nd@arm.com \
--cc=newlib@sourceware.org \
--cc=richard.earnshaw@arm.com \
--cc=srinath.parvathaneni@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).