Page MenuHomeFreeBSD

MAC/do: Factor out setting/destroying rule structures
ClosedPublic

Authored by olce on Nov 15 2024, 5:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 16 2025, 2:50 PM
Unknown Object (File)
Mar 4 2025, 11:39 PM
Unknown Object (File)
Feb 8 2025, 7:05 PM
Unknown Object (File)
Feb 3 2025, 2:36 PM
Unknown Object (File)
Jan 12 2025, 9:14 PM
Unknown Object (File)
Jan 5 2025, 9:08 AM
Unknown Object (File)
Dec 27 2024, 11:05 AM
Unknown Object (File)
Dec 26 2024, 8:15 PM

Details

Summary

This revision is part of a series. Click on the Stack tab below to see the context.
This series has also been squeezed into D47633 to provide an overall view.

Commit message:
This generally removes duplicate code and clarifies higher-level
operations, allowing to fix several important bugs.

New (internal) functions:

  • ensure_rules(): Ensure that a jail has a populated 'mac_do_osd_jail_slot', and returns the corresponding 'struct rules'.
  • dealloc_rules(): Destroy the 'mac_do_osd_jail_slot' slot of a jail.
  • set_rules(): Assign already parsed rules to a jail. Leverages ensure_rules().
  • parse_and_set_rules(): Combination of parse_rules() and set_rules().

Bugs fixed in mac_do_prison_set():

  • A panic if "mdo" is explicitly passed to JAIL_SYS_NEW but "mdo.rules" is absent, in which case 'rules_string' wasn't set (setting 'rules' at this point would do nothing).
  • In the JAIL_SYS_NEW case, would release the prison lock and reacquire it, but still using the same 'rules' pointer that can have been freed and changed concurrently, as the prison lock is temporary unlocked. (This is generally a bug of the mac_do_alloc_prison()'s interface when 'lrp' is not NULL.)

Suppress mac_do_alloc_prison(), as it has the following bugs:

  • The interface bug mentioned just above.
  • Wrong locking, leading to deadlocks in case of setting jail parameters multiple times (concurrently or not).

It has been replaced by either parse_and_set_rules(), or by
ensure_rules() directly coupled with prison_unlock().

Rename mac_do_dealloc_prison(), the OSD destructor, to dealloc_osd(),
and make it free the 'struct rules' itself (which was leaking).

While here, in parse_rules(): Clarify the contract by adding comments,
and check (again) for the rules specification's length.

Diff Detail

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

Event Timeline

olce requested review of this revision.Nov 15 2024, 5:06 PM
bapt added inline comments.
sys/security/mac_do/mac_do.c
487

the void casting is not necessary

This revision is now accepted and ready to land.Nov 19 2024, 7:57 AM
olce added inline comments.
sys/security/mac_do/mac_do.c
487

You're right that it isn't technically needed, but I prefer to keep such uses as they clearly mark that the author knew the function returns something and deliberately ignores it (when reviewing code, it is a tiny help to detect programming errors).

As you saw, these calls have been removed in the next revision anyway.

This revision was automatically updated to reflect the committed changes.
olce marked an inline comment as done.