Is PRIV_IO the right privilege to check here or should we create a new PRIV_GEOM?

Is the void * cast needed at all?

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).

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).

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

But the g_ctl_ioctl_ctl() is void *. Isn't it possible to pass anything there?

A char * yes, but not uintptr_t.

Note that the cast was already here, I just moved it one level out.

gctl_error() sets req->nerror = EINVAL if it's not already set. Don't we want to return EACCES?

It should probably be EPERM rather than EACCES, no?

I guess so, but the error string you're using with gctl_error() corresponds to EACCES, not EPERM ("Operation not permitted").

So one thing I could do before is create an md device in a jail, and then create a GEOM on top of that. Now that's no longer possible.

It's not obvious to me that that's the right behaviour: if the jail admin decided to expose /dev/geom.ctl to a jail, shouldn't it be usable?

Thinking about this more (albeit without any coffee yet), I don't understand the purpose of the priv_check() here. Shouldn't write access to /dev/geom.ctl be sufficient to grant this privilege?

oh yeah I guess we have quite a few tests that do exactly this and would stop working if we don't grant a jailed superuser write access.

What we are doing here is the historical behavior of allowing configuration changes regardless of write access to the control node, but only for the superuser, not for e.g. members of group operator.

Oh sorry I thought you were replying to the similar line a few lines up. The purpose of this line is to allow MAC modules to selectively disable PRIV_GEOM. The code to do that is in kern_priv.c but without this line, it would have no effect.

I just finished a test suite run with your two GEOM patches and I believe there's only one new failure, which I didn't dig into, in sys/fs/unionfs/unionfs_test:unionfs_exec

But if I decide I want to make /dev/geom.ctl writeable by an unprivileged user or group, this priv_check() will still strip FWRITE, so my change will have no effect. To implement what you describe, I'd expect to see something like

if ((fflag & FWRITE) == 0 && priv_check(td, PRIV_GEOM) == 0)
    fflag |= FWRITE;

instead.

But if I decide I want to make /dev/geom.ctl writeable by an unprivileged user or group

that would effectively give them complete control of the system...

I don't immediately see how. It's certainly a powerful capability, but how does it automatically give you unfettered access to the system? Said another way, why don't we need an equivalent check for, say, /dev/xpt0?