geom: Check file flags before handling control request
Needs ReviewPublic
Actions

Authored by des on Fri, Sep 11, 2:51 PM.

Details

Reviewers

kevans
markj
phk
mav
glebius
imp

Group Reviewers

geom

Summary

Pass fflag down to g_ctl_req() and:

Refuse all requests unless fflag has the FREAD bit set

Refuse all but getxml unless fflag also has the FWRITE bit set

This should allow us to restore read access to /dev/geom.ctl for group
operator; they will then be able to run gpart list or gpart backup
but not commands that actually modify the table.

For backward compatibility, we set the FWRITE bit if it is not already
set and the calling thread has PRIV_IO privileges. This should cover
the case of older binaries (which open /dev/geom.ctl read-only) running
as root on a newer kernel. Since we intend to merge this to stable/15,
the compatibility code is conditional on COMPAT_FREEBSD14.

MFC after: 1 week
Event: EuroBSDCon 2026 DevSummit

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76776
Build 73659: arc lint + arc unit

Event Timeline

des created this revision.Fri, Sep 11, 2:51 PM

Herald added a subscriber: imp. · View Herald TranscriptFri, Sep 11, 2:51 PM

des requested review of this revision.Fri, Sep 11, 2:51 PM

Harbormaster completed remote builds in B76776: Diff 186467.Fri, Sep 11, 2:51 PM

des added a child revision: D59592: libgeom: Open the control device read-write.Fri, Sep 11, 2:51 PM

imp accepted this revision.Fri, Sep 11, 4:35 PM

This revision is now accepted and ready to land.Fri, Sep 11, 4:35 PM

des edited the summary of this revision. (Show Details)Sat, Sep 12, 12:36 PM

backcompat

This revision now requires review to proceed.Sat, Sep 12, 12:39 PM

Harbormaster completed remote builds in B76793: Diff 186512.Sat, Sep 12, 12:39 PM

des mentioned this in D59592: libgeom: Open the control device read-write.Sat, Sep 12, 12:39 PM

reword

Harbormaster completed remote builds in B76794: Diff 186513.Sat, Sep 12, 12:45 PM

des added inline comments.Sat, Sep 12, 12:45 PM

sys/geom/geom_ctl.c
658	Is `PRIV_IO` the right privilege to check here or should we create a new `PRIV_GEOM`?

The change seems right to me, but I am not an expert in the area.

sys/geom/geom_ctl.c
651	Is the `void *` cast needed at all?

des added inline comments.Sat, Sep 12, 1:51 PM

sys/geom/geom_ctl.c
651	`data` is a `caddr_t` which is currently a `char *` but might be `uintptr_t` in the future (and in fact _is_ `uintptr_t` in some downstream projects).

des marked an inline comment as done.Sat, Sep 12, 1:51 PM

I did a cursory pass over verbs in base to see if we /might/ want to expand this to others / pass it through to mp->ctlreq, but it doesn't seem very likely except maybe in getactive?

sys/geom/geom_ctl.c
658	Looking at current uses of `PRIV_IO`, yeah- we should break this out into its own `PRIV_GEOM` (or `PRIV_GEOM_CTL`, but I guess I can't imagine what another GEOM' privilege might look like) -- I can easily see one wanting to regain geom ctl with something like MAC, and PRIV_IO includes scary looking things like io(4).
658	We also discussed in person ensuring that we don't grant the new priv(9) to jails by default, but with special consideration for `COMPAT_FREEBSD14` vs. not