Page MenuHomeFreeBSD

Allow setting a per-Jail fallback ABI brand.
Needs ReviewPublic

Authored by nyan_myuji.xyz on May 25 2023, 4:16 AM.
Tags
Referenced Files
Unknown Object (File)
Dec 29 2023, 7:30 AM
Unknown Object (File)
Dec 23 2023, 2:54 AM
Unknown Object (File)
Dec 22 2023, 4:34 PM
Unknown Object (File)
Dec 10 2023, 8:16 PM
Unknown Object (File)
Dec 8 2023, 9:34 PM
Unknown Object (File)
Dec 8 2023, 11:28 AM
Unknown Object (File)
Dec 7 2023, 6:19 PM
Unknown Object (File)
Nov 30 2023, 7:02 AM

Details

Reviewers
emaste
lwhsu
khng
freebsd_igalic.co
rew
Group Reviewers
Jails
Summary

Similar to kern.elfXX.fallback_brand sysctl, this allow setting a fallback ELF32/64
brand, but in a per-Jail basis. If the fallback brand in Jail does not work, it will attempt
to use the system-wise kern.elfXX.fallback_brand ABI.

Useful in running statically linked Penguin binaries with wrong ABI value in
their EFI header in a Jail, without brandelf (which modifies the file) nor
setting systemwise kern.elfXX.fallback_brand sysctl.

Event: Kitchener-Waterloo Hackathon 202305

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 51730
Build 48621: arc lint + arc unit

Event Timeline

  • fix sysctl parametetr description
nyan_myuji.xyz retitled this revision from Allow setting a per-Jail efi fallback ABI brand. to Allow setting a per-Jail fallback ABI brand..May 25 2023, 4:24 AM

please extend the man page as well, to mention this setting

This revision now requires changes to proceed.May 25 2023, 5:27 AM
usr.sbin/jail/jail.8
28

Don't forget bump the date. :)

765

If you really want a blank line, you need .Pp

775

.Xr brandelf 1

  • use .Pp instead of empty lines in manpage
nyan_myuji.xyz marked 2 inline comments as done.
  • update date

looks good otherwise!

usr.sbin/jail/jail.8
773

cause —> causes

jamie added inline comments.
sys/sys/jail.h
209

This would be better as a replacement for pr_spare.

Rather than add a separate value different from the kern.fallback_elf_brand sysctl, it would make sense for the jail parameter to be tied to the sysctl itself, such as the securelevel parameter is. It's complicated somewhat by the fact that there are three similar sysctls.

  • Use pr_spare for pr_elf_fallback_brand

Rather than add a separate value different from the kern.fallback_elf_brand sysctl, it would make sense for the jail parameter to be tied to the sysctl itself, such as the securelevel parameter is. It's complicated somewhat by the fact that there are three similar sysctls.

I think that will be a good idea. However right now the kern.elfXX.fallback_elf_brand are tied to 2 global variables which their name are generated by the __elfN(xxx) macro. We could technically remove them and tie all 3 to the jail one? Maybe that belongs to a separate review?

However right now the kern.elfXX.fallback_elf_brand are tied to 2 global variables which their name are generated by the __elfN(xxx) macro. We could technically remove them and tie all 3 to the jail one? Maybe that belongs to a separate review?

Interesting. I didn't expect that kern.fallback_elf_brand was a just an alias for kern.elfXX.fallback_brand. But then I also didn't expect it would have a comment /* XXX compatibility, remove for 6.0 */ ;-). This does complicate things, since you can't just use struct prison for dynamic sysctls like the per-arch elf fallbacks, but would need to use OSD(9) instead. So yes, perhaps that's something for another review.

But then, since there's not a non-arch-specific fallback value (though ther kern.fallback_elf_brand sysctl alias makes it look like there is), I'm not sure we should be introducing one for jail only. But maybe it would be good to make such a global fallback, tied to the jail as I mention, and use kern.fallback_elf_brand for that. And then perhaps that old XXX comment could go away.

dchagin added inline comments.
sys/kern/imgact_elf.c
349

I suggest drop it below, ie, initialize prison_fallback_brand before using

  • assign value to prison_fallback_brand varibale right before it is used

The overall idea seems ok to me for what it's worth, my comments are about cosmetic issues.

sys/kern/imgact_elf.c
469

This line looks too long.

486

Don't we want to first match on prison_fallback_brand so long as it's not equal to -1? That is, it looks like there should be two loops, first searching for the per-jail fallback brand, then for the kernel's fallback brand.

sys/kern/kern_jail.c
1092

This line is too long.

2441

Indentation of continuing lines should be by four spaces.

usr.sbin/jail/jail.8
764

"jail" shouldn't be capitalized here or below.

766
770
771
774
775