Page MenuHomeFreeBSD
Differential D40262

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

Authored by nyan_myuji.xyz on May 25 2023, 4:16 AM.
Tags
Referenced Files
Unknown Object (File)
Thu, Jun 11, 7:58 PM
Unknown Object (File)
Thu, Jun 11, 8:31 AM
Unknown Object (File)
Thu, Jun 11, 8:25 AM
Unknown Object (File)
Thu, Jun 11, 6:30 AM
Unknown Object (File)
Thu, Jun 11, 5:16 AM
Unknown Object (File)
Thu, Jun 11, 1:12 AM
Unknown Object (File)
Wed, Jun 10, 7:05 AM
Unknown Object (File)
Thu, Jun 4, 4:19 AM
View All Files
Subscribers
antranigv_freebsd.am
dchagin
freebsd_igalic.co
imp
jamie
markj

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 51674
Build 48565: arc lint + arc unit

Event Timeline

nyan_myuji.xyz created this revision.May 25 2023, 4:16 AM
Herald added a subscriber: imp. · View Herald TranscriptMay 25 2023, 4:16 AM
nyan_myuji.xyz requested review of this revision.May 25 2023, 4:16 AM
Harbormaster completed remote builds in B51673: Diff 122392.May 25 2023, 4:16 AM
nyan_myuji.xyz updated this revision to Diff 122393.May 25 2023, 4:19 AM
  • fix sysctl parametetr description
Harbormaster completed remote builds in B51674: Diff 122393.May 25 2023, 4:19 AM
nyan_myuji.xyz added reviewers: emaste, lwhsu, khng, Jails.May 25 2023, 4:20 AM
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
nyan_myuji.xyz edited the summary of this revision. (Show Details)May 25 2023, 4:50 AM
freebsd_igalic.co requested changes to this revision.May 25 2023, 5:27 AM
freebsd_igalic.co added a subscriber: freebsd_igalic.co.

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

This revision now requires changes to proceed.May 25 2023, 5:27 AM
nyan_myuji.xyz updated this revision to Diff 122429.May 25 2023, 5:58 PM

Man page update

Harbormaster completed remote builds in B51685: Diff 122429.May 25 2023, 5:58 PM
lwhsu added inline comments.May 25 2023, 6:15 PM
usr.sbin/jail/jail.8
28 ↗(On Diff #122429)

Don't forget bump the date. :)

765 ↗(On Diff #122429)

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

775 ↗(On Diff #122429)

.Xr brandelf 1

nyan_myuji.xyz updated this revision to Diff 122431.May 25 2023, 6:17 PM
  • use .Xr in manpage
Harbormaster completed remote builds in B51686: Diff 122431.May 25 2023, 6:17 PM
nyan_myuji.xyz updated this revision to Diff 122432.May 25 2023, 6:19 PM
  • use .Pp instead of empty lines in manpage
Harbormaster completed remote builds in B51687: Diff 122432.May 25 2023, 6:19 PM
nyan_myuji.xyz updated this revision to Diff 122433.May 25 2023, 6:20 PM
nyan_myuji.xyz marked 2 inline comments as done.
  • update date
Harbormaster completed remote builds in B51688: Diff 122433.May 25 2023, 6:21 PM
nyan_myuji.xyz marked an inline comment as done.May 25 2023, 6:21 PM
freebsd_igalic.co added a comment.May 25 2023, 11:19 PM

looks good otherwise!

usr.sbin/jail/jail.8
773 ↗(On Diff #122433)

cause —> causes

jamie added a subscriber: jamie.May 25 2023, 11:53 PM
jamie added inline comments.
sys/sys/jail.h
208

This would be better as a replacement for pr_spare.

jamie added a comment.May 26 2023, 12:04 AM

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.

nyan_myuji.xyz updated this revision to Diff 122449.May 26 2023, 12:29 AM
  • Use pr_spare for pr_elf_fallback_brand
Harbormaster completed remote builds in B51697: Diff 122449.May 26 2023, 12:30 AM
nyan_myuji.xyz marked 2 inline comments as done.May 26 2023, 12:30 AM
nyan_myuji.xyz added a comment.May 26 2023, 12:42 AM
In D40262#917097, @jamie wrote:

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?

jamie added a comment.May 26 2023, 3:20 AM
In D40262#917102, @nyan_myuji.xyz wrote:

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 a subscriber: dchagin.May 26 2023, 9:25 AM
dchagin added inline comments.
sys/kern/imgact_elf.c
349

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

nyan_myuji.xyz updated this revision to Diff 122506.May 27 2023, 3:36 AM
  • assign value to prison_fallback_brand varibale right before it is used
Harbormaster completed remote builds in B51730: Diff 122506.May 27 2023, 3:36 AM
nyan_myuji.xyz added a child revision: D40299: Tie kern.fallback_elf_brand to the fallback elf brand of the current prison.May 27 2023, 4:57 AM
nyan_myuji.xyz marked an inline comment as done.May 27 2023, 4:59 AM
ihor_antonovs.family added a project: Jails.Jun 4 2023, 2:00 PM
allanjude added a reviewer: rew.Jul 11 2023, 11:41 PM
antranigv_freebsd.am added a subscriber: antranigv_freebsd.am.Aug 8 2023, 9:57 AM
antranigv_freebsd.am removed a subscriber: antranigv_freebsd.am.
antranigv_freebsd.am added a subscriber: antranigv_freebsd.am.
markj added a subscriber: markj.Sep 15 2023, 3:12 PM

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

sys/kern/imgact_elf.c
471

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
1052

This line is too long.

2402

Indentation of continuing lines should be by four spaces.

usr.sbin/jail/jail.8
764 ↗(On Diff #122506)

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

766 ↗(On Diff #122506)
770 ↗(On Diff #122506)
771 ↗(On Diff #122506)
774 ↗(On Diff #122506)
775 ↗(On Diff #122506)

Revision Contents
Changeset List

PathSize
sys/
kern/
8 lines
25 lines
sys/
1 line

Diff 122393

View Options

sys/kern/imgact_elf.c

Loading...
View Options

sys/kern/kern_jail.c

Loading...
View Options

sys/sys/jail.h

Loading...