~sircmpwn/helios-devel

helios: map_flags::X should mean that code execution is allowed v1 PROPOSED

Gilles Duboscq: 1
 map_flags::X should mean that code execution is allowed

 2 files changed, 2 insertions(+), 2 deletions(-)
#1113679 .build.yml failed
Hi Drew,
Yes, i have tested that `make check` still works as much as before
those changes.
On aarch64, all tests pass except

task::pagefault................. Abort: helios/endpoint.ha:84:22:
assertion failed

At least for me this fails on master as well. I have not checked what
this is about.

Note that i also tested that with this patch, the X flag handling in
x86_64's arch_flags [1] can also be uncommented and it works.
Before this patch, uncommenting that would cause a fault. I wasn't
sure why this had been commented out so i left it alone. I can also
update my patch to uncomment it if you'd like.

Overall, i think this is a case of double negations: the patch i sent
doesn't break anything because it flips the logic in both phdr_load
and aarch64's map_flags (currently the sole active user).

I noticed that in a RISC-V port that i am playing with. I had
implemented map_flags::X as meaning "allow execution when set" and i
was getting strange faults. That's how i noticed that the code in
phdr_load is inverted.
 Gilles

[1]: https://git.sr.ht/~sircmpwn/helios/tree/master/item/objects/+x86_64/arch_vspace.ha#L185-187
I also checked mercury and after this patch, it also needs the same
inversion in its loader [1].
 Gilles

[1]: https://git.sr.ht/~sircmpwn/mercury/tree/master/item/cmd/sysinit/loader.ha#L135

Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~sircmpwn/helios-devel/patches/47684/mbox | git am -3
Learn more about email & git

[PATCH helios] map_flags::X should mean that code execution is allowed Export this patch

Use that definition in init::phdr_load and flip usage in aarch64's
objects::arch_flags.
---
 init/load.ha                    | 2 +-
 objects/+aarch64/arch_vspace.ha | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/init/load.ha b/init/load.ha
index b485b6c..1d407ab 100644
--- a/init/load.ha
+++ b/init/load.ha
@@ -63,7 +63,7 @@ fn phdr_load(task: *task, image: *elf::header64, ph: *elf::phdr64) void = {
	assert(npage <= types::UINT_MAX);

	let flags: map_flags = 0;
	if (ph.p_flags: elf::pf & elf::pf::X == 0) {
	if (ph.p_flags: elf::pf & elf::pf::X != 0) {
		flags |= map_flags::X;
	};
	if (ph.p_flags: elf::pf & elf::pf::W != 0) {
diff --git a/objects/+aarch64/arch_vspace.ha b/objects/+aarch64/arch_vspace.ha
index a19b70a..4677f0c 100644
--- a/objects/+aarch64/arch_vspace.ha
+++ b/objects/+aarch64/arch_vspace.ha
@@ -150,7 +150,7 @@ fn arch_flags(page: *page, flags: map_flags) pte = {
	} else {
		arch |= pte::RO_EL0;
	};
	if (flags & map_flags::X != 0) {
	if (flags & map_flags::X == 0) {
		// XXX: Do we want to add PXN to this?
		arch |= pte::UXN;
	};
-- 
2.43.0
helios/patches/.build.yml: FAILED in 39s

[map_flags::X should mean that code execution is allowed][0] from [Gilles Duboscq][1]

[0]: https://lists.sr.ht/~sircmpwn/helios-devel/patches/47684
[1]: mailto:gilles.m.duboscq@gmail.com

✗ #1113679 FAILED helios/patches/.build.yml https://builds.sr.ht/~sircmpwn/job/1113679
I'm confused. aarch64 currently, well, works, as in userspace boots up
and the test suite passes and so on. I would imagine that it would not
work if the X flag was not handled appropriately. Have you tested that
this does not break anything?