Page MenuHomeFreeBSD

linux: implement pkey_alloc, pkey_free and pkey_mprotect
ClosedPublic

Authored by dteske on Tue, Aug 11, 5:02 AM.
Tags
None
Referenced Files
F166887032: D58782.id184120.diff
Mon, Aug 17, 1:12 PM
F166843968: D58782.diff
Mon, Aug 17, 12:51 AM
F166823498: D58782.id184025.diff
Sun, Aug 16, 9:14 PM
Unknown Object (File)
Sun, Aug 16, 9:47 AM
Unknown Object (File)
Sat, Aug 15, 8:07 PM
Unknown Object (File)
Sat, Aug 15, 8:07 PM
Unknown Object (File)
Sat, Aug 15, 12:56 AM
Unknown Object (File)
Fri, Aug 14, 11:14 PM

Details

Summary

Bridge the Linux memory protection key syscalls to FreeBSD's native
MPK support instead of returning ENOSYS. Modern Linux software
probes these at startup: Chromium-based browsers (found via
www/linux-brave) use protection keys for V8's heap and JIT
sandboxing, and glibc >= 2.27 exposes the full API.

pkey_alloc() allocates from a per-process bitmap kept in the process
emuldata (key 0 implicitly allocated, matching Linux's
mm_pkey_allocation_map; ENOSPC once keys 1..15 are exhausted or when
PKU is absent, as Linux returns on such hardware) and applies the
requested initial access rights to the calling thread's PKRU, located
in the XSAVE area via xsave_area_offset(). pkey_free() is
bookkeeping only: as on Linux, freeing neither untags pages nor
updates PKRU. pkey_mprotect() performs the protection change and
tags the range through amd64_pkru_update(), factored out of
sysarch(2)'s AMD64_SET_PKRU/AMD64_CLEAR_PKRU implementation so that
both share the same argument checking and map read lock
synchronization with a parallel pmap_vmspace_copy() on fork; tags die
with the mapping, matching Linux VMA semantics. A pkey of -1
degrades to plain mprotect.

The allocation map is inherited on fork and reset on exec. At exec
the Linux sysvecs initialize PKRU to 0x55555554, Linux's init_pkru
default (access disabled for keys 1..15), so memory tagged with a
not yet allocated key is inaccessible to threads that were never
granted rights -- the property V8's thread isolation relies on.
Setting PKRU at exec initializes the user FPU state slightly earlier
than the lazy first-use path; the state would be initialized moments
later in rtld/libc startup regardless. Protection key faults
already deliver SEGV_PKUERR through the existing siginfo
translation.

The common code carries no architecture ifdefs. Machine-dependent
state lives in struct linux_pemuldata_md, embedded in the process
emuldata in the manner of struct mdthread, and common code calls
per-arch lifecycle hooks (linux_pemuldata_init_md/_exec_md) and pkey
back ends after performing the parameter validation Linux applies
regardless of hardware support. On amd64 the implementation lives
in sys/amd64/linux/linux_pkru.c, compiled into linux_common and
serving both the 64-bit and 32-bit Linux ABIs. Elsewhere (arm64,
i386) linux_emul_md.c provides stubs returning what Linux returns on
hardware without protection keys (ENOSPC from pkey_alloc;
pkey_mprotect with a pkey of -1 acts as plain mprotect), so
applications take their normal no-PKU fallback instead of the ENOSYS
path.

PR: 297427
MFC after: 1 month

Test Plan
  1. In PR 297427 I attached a litmus test https://bz-attachments.freebsd.org/attachment.cgi?id=273612 which should produce VERDICT: PASS on patched system, ENOSYS on unpatched system, and ENOSPC on a CPU without PKU
  2. Install www/linux-brave from ports, no dmesg/stderr saying syscall pkey_alloc not implemented on launch
  3. Run brave-browser --js-flags=--force_memory_protection_keys which forces Aw, Snap! failure on systems without PKRU, succeeds on patched kernel
  4. Each time you open a new window/tab in brave you can see V8.WasmMemoryProtectionKeysSupport increment in brave://histograms (with mean = 1.0 over samples)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/compat/linux/linux_emul.c
170

What is the purpose of this acquire?

sys/compat/linux/linux_mmap.c
250

amd64 implementation should go into amd64/linux

273

This data is already available in amd64/fpu.c:xsave_area_descr. Add a helper to fetch it from there, instead of direct use of cpuid.

Also doing CPUID on each op is too expensive and not needed.

sys/compat/linux/linux_mmap.c
450

I guess vm_map_check_boundary() will catch an overflow here?

458

I'd prefer to factor out this common code rather than duplicate it.

477

Virtually all of this code is machine-dependent, it should live in sys/amd64/linux/linux_machdep.c or so.

dteske added inline comments.
sys/compat/linux/linux_emul.c
170

was taking consistent snapshot of parent's map, but question is well taken. The map is a single aligned word, so read was atomic, and a pkey_alloc() racing the fork in another thread yields valid serialization with or without the lock. Dropped (with a comment noting why the unlocked read suffices).

sys/compat/linux/linux_mmap.c
250

Done. Implementation now lives in sys/amd64/linux/linux_pkru.c, compiled into linux_common so it serves boht the 64-bit and 32-bit ABIs; MI file keeps only no-PKU fallbacks

273

Thanks. xsave_area_offset() already provides exactly that (the user save area is standard format, so it reduces to the xsave_area_desc[] lookup), and xsave_area_hdr_offset() replaces the hand-computed header offset. No CPUID left in the path.

450

Yes. vm_map_check_boundary() starts with vm_map_range_valid(), which rejects end < start, so wrapped addr + len fails cleanly with EINVAL. Now the same way sysarch(2) does it, so two can't drift apart

458

Done. factored into amd64_pkru_update() in sys_machdep.c (I had previously been trying to keep this entirely within linuxulator code), and shared with sysarch(2)'s AMD64_SET_PKRU/AMD64_CLEAR_PKRU. Going through the shared helper also fixed a subtle difference: old copy here rounded before checking, so len == 0 at an unaligned address would have tagged a page; len == 0 is now the mprotect no-op Linux specifies

477

Roger that. Moved to a dedicated sys/amd64/linux/linux_pkru.c rather than linux_machdep.c, since linux_machdep.c only serves the 64-bit ABI module and this code is shared with linux32 via linux_common.

Had previously been trying to localize all changes to linux*.ko for testing convenience (easier to recompile module and reload without a reboot than to touch kernel sources)

sys/amd64/linux/linux_pkru.c
20

param.h is not needed

81

There, you only update the xsave area. What does prevent a context switch from occurring after fpugetregs(), which would load the xsave context into CPU, but before the writes to the xsave area? I believe in this case the update to the xsave area is lost.

dteske marked 6 inline comments as done.

Inline comments (kib)

dteske added inline comments.
sys/amd64/linux/linux_pkru.c
20

Removed

81

Well taken. the 'cannot regain FPU ownership in the kernel' assumption was wrong; a switch-in after fpugetregs() can load the context back into the CPU, and the next save would then overwrite the area update. The critical section is now held across fpugetregs() and the stores (fpugetregs() nests its own critical sections and does not sleep), so no context switch can intervene. The CPU-resident case was already handled under the same section by the wrpkru fast path.

sys/compat/linux/linux_emul.h
74

BTW this member should be arch-specific, right?

sys/compat/linux/linux_misc.c
363

I still think that this is not correctly structured.

The common implementation of linux_pkey_X() should do the common parameters validation, and then call the arch-specific implementation. For amd64, the implementation would be what is needed to handle PKRU, for all other arches (basically arm64 and might be i386 if you care) it would be stubs returning just error.

dteske marked 2 inline comments as done.

Address inline comments (kib)

dteske edited the summary of this revision. (Show Details)
sys/compat/linux/linux_emul.c
141

My desire there, when I asked to move the amd64 specific code into sys/amd64. is to eliminate the #if defined(__amd64__) blocks from the common code, completely, IMO each such block makes a tax on the FreeBSD maintainability. For the implementer of the given feature, it is an overhead. But in the long run, it is liability.

As the bad example, take a look at sys/kern/subr_smp.c

The good case is how we handle e.g. struct thread or struct pcpu. There is struct mdthread which is per-arch and encapsulates all machine-dependent fields. Also, there are per-arch hooks (documented in cpu_machdep.9 by jhb recently) which eliminate #ifdef arch from the common sources.

I believe that the same structure can be usefully applied there. It would put the foundational work on handing future arch-specific linuxolator state as well.

I think that what is in the review now is working correctly, but we can make it better by properly maintain the good code structure.

sys/compat/linux/linux_emul.c
141

My desire there, when I asked to move the amd64 specific code into sys/amd64. is to eliminate the #if defined(__amd64__) blocks from the common code, completely, IMO each such block makes a tax on the FreeBSD maintainability. For the implementer of the given feature, it is an overhead. But in the long run, it is liability.

Gotcha.

As the bad example, take a look at sys/kern/subr_smp.c

Thanks. I'll have a look.

The good case is how we handle e.g. struct thread or struct pcpu. There is struct mdthread which is per-arch and encapsulates all machine-dependent fields. Also, there are per-arch hooks (documented in cpu_machdep.9 by jhb recently) which eliminate #ifdef arch from the common sources.

Perfect. Thank you for pointing me in the right direction.

I believe that the same structure can be usefully applied there. It would put the foundational work on handing future arch-specific linuxolator state as well.

👍 (thumbs up)

I think that what is in the review now is working correctly, but we can make it better by properly maintain the good code structure.

Each revision, I am dogfooding and running unit-tests on to make sure the implementation is solid.

Working on next iteration. Cheers.

Implement per-arch hooks akin to mdthread; no arch ifdefs in common code

dteske marked an inline comment as done.
This revision is now accepted and ready to land.Fri, Aug 14, 12:45 AM

Jenkins shows aarch64 build failures since: https://ci.freebsd.org/job/FreeBSD-main-aarch64-build/35832/ :

sys/compat/linux/linux_emul.h:58:49: error: declaration of
 'struct image_args' will not be visible outside of this function [-Werror,-Wvisibility]
   58 | int     linux_common_execve(struct thread *, struct image_args*);
      |                                                     ^
1 error generated.