You've added an optional permission bit, but there's no option to change it. If it would make sense to allow it to all jails, there's no need for PR_ALLOW_ICMP_ACCESS. If it would make sense to restrict to some jails, there needs to be a matching jail.allow parameter, as defined in kern_jail.c's pr_flag_allow array and SYSCTL_JAIL_PARAM(_allow, ...).

The patch looks good to me, but I'm unable to get that LOR on an unpatched system. Has something been fixed in the meantime?

I'll try the jail_set approach.

Considering jail_set(2) can also attach with the JAIL_ATTACH flag, it would be handy to put these new flags in the same space, with a JAIL_ATTACH_MASK including them. Then the attaching done by jail_set can also do the right thing if it chooses.

Might it also be useful to include something similar on the stop side, run after taking down the IP addresses? I can't think of a use offhand, unless one wants to leave absolutely no trace of jails, and remove everything that was added in exec.prepare.

It would make sense for this to be two separate commits - one for the reordering of IP_*.

There's a "right thing" to do on SIGINT, though it's not immediately obvious what that is. In the jail creation process, it would make sense to clean up a partially created jail, probably in conjunction with letting the jailed processes handle their own SIGINT. But that's not quite the same as just ignoring it, because there are other cases:

If jail_attach(2) doesn't leave a process sufficiently jailed to the point that it can be used for jailbreak, that's a bug in jail_attach that should be fixed.

Looks good!

It may be a better user interface if "-j" automatically did the right thing - limit output to the specified jail for traditional jails, and attach to vnet jails. So after the jail_getid(), a jail_get() or jail_getv() to fetch the jail's vnet parameter. Then the jail_attach() can happen if the vnet is JAIL_SYS_NEW.

Aside from a few trailing whitespaces, this all looks good to me.

Tested with ps and jexec, passing jail name and jid, and with numerically and non-numerically named jails. All's good :-).

New jails are now created with the unprivileged_proc_debug bit inherited from the parent unless otherwise specified.

Huh - so I can. I didn't know of (or even suspect) such a possibility.

Sorry for a post-acceptance note, but on trying it out I noticed that jails are created by default with allow.nounprivileged_proc_debug. That's an easy fix - the bit needs to be added to PW_DEFAULT_ALLOW in kern_jail.h. I'm apparently unable to change the diff in this revision, so instead of creating a new revision I'll just mention that's what I'll be committing.

In D18319#388858, @lattera-gmail.com wrote:

Because of this, having the check in sys/kern/kern_priv.c is the right place. There's no real need to duplicate the logic to prison_priv_check. I can still add it, if you want, but I believe it would be a waste of cycles.

priv_check_cred() in kern_priv.c isn't the right place to make the check, but prison_priv_check() in kern_jail.c. PRIV_DEBUG_UNPRIV is already in that function's list, in the part that lets jails do things, and it needs to be moved to the bottom part of the function where you'll see a number of other cases where a certain privilege checks a certain pr_allow bit.

In D18319#388775, @lattera-gmail.com wrote:

Then the jail must simply obey the existing state of unprivileged process debugging. We could go that route, but I wanted to make it flexible. I think setting CTLFLAG_SECURE is a good compromise between flexibility and security.

OK, if the jail needs to have that bit set before anything is run, then yes it needs to be a parameter.

Since this bit is under the full control of the prison itself, does it belong in pr_allow? On the plus side, that lets the system create a jail with this turned on, but that can be just as easily done in the jail's sysctl.conf. It's something of a departure from the idea of this being something the jail is or isn't allowed to do. If you forgo the ability to set it as a jail parameter, then the bit can go into pr_flags and you won't have to bother noting which PR_ALLOW bits are allowed to be set .

It's allow.mount.nofusefs. It should (currently) work once the kld is loaded, but the new strcmp will need to be added to make it work before it's loaded.

It works with allow.mount.fusefs, but not with allow.mount.nofusefs (which will still try to kldload "fusefs"). Comparing the fs name to both "fusefs" and "nofusefs" should be all that's needed.

I'm keeping the sysctls around, though without COMPAT_FREEBSD11 (or with BURN_BRIDGES), they're read-only. This preserves the expected behavior for programs that want to find out what they're allowed to do before attempting it (e.g. rc.d/hostname and rc.d/zfs). But they will no longer be used to set global permissions for jails.

OK, looks good with one last-minute nit: spaces in the jailp.h line where a tab should be

Yes, this is a need that has gone unanswered for a while now.

One more thing to do: jail(8) should mention the flag. There's a section about module-specific flags where I think it would fit better than the main allow.* section.

It's good to see that jail-enabling a filesystem is indeed easier now than it used to be!

I've added D16146, which makes a new allow.* bit easy:

In addition to the question of where to check the permissions, there's also the issue that the allow.vmm parameter shouldn't exist in a non-VMM system. This means the SYSCTL_JAIL_PARAM should be defined in vmm_dev.c or some other vmm-related file; that way, if VMM is loaded as a module, the parameter would be attached to that module.

What does this buy us? What I can come up with is:

Once again, this time actually updating the diff...

As suggested by bz@, I've only removed the sysctls #ifdef BURN_BRIDGES.

I have another revision in the works, D14791, which removes those deprecated global permission parameters. Since this patch works with those parameters, I would naturally adjust whichever one goes in last (provided I get away with the parameter removal).

Te latest diff, to account for the changes I committed to neaten the somewhat messy pr_allow_names/pr_allow_nonames array pairs. And I also went with bumping VFS_VERSION instead of __FreeBSD_version.

In D14681#309678, @kib wrote:

I you might want to bump VFS_VERSION instead of __FreeBSD_version.

The latest changes:

• Put the bits back into pr_allow. Adding pr_allow_mount only served to duplicate code.
• Make the KBI change: put a prison flag in struct vfsconf.
• Replace prison_check_vfs with a call to prison_allow (another advantage of using pr_allow).
• Use asprintf in prison_add_vfs, instead of sprintf/strdup.
• Do the right thing ifdef NO_SYSCTL_DESCR.

I've updated the diff to:

In D14681#308570, @jhb wrote:

One more thought: if you use the value from 'vfc_name' as the pointer you set in the array, you don't have to do actual string comparisons but can just do pointer compares to find the matching index (and thus bit) in the array in prison_check_vfs(). The jail parameter logic would still have to do string compares though.

In D14681#308541, @jhb wrote:

For per-jail settings I would still be tempted to not try to reserve space in the names, but instead perhaps have a separate "allow" mask just for VFS, and parse mount parameters explicitly.

Yes, I could do that - there are a few unused spots in struct prison to make it easy. Then I could leave pr_allow_names pretty much alone (except removing the old static allow.mount.*).

In D14681#308495, @jhb wrote:

... You could just add a new 'VFCF_JAIL_ALLOW' which is a dynamic flag that the sysctl knobs turn on/off. The sysctl node would be a SYSCTL_PROC handler and it can take a pointer to the 'struct vfsconf' as its arg2 value.

In D14681#308495, @jhb wrote:

I'm not quite a fan of the manual sysctl tree walking. I also don't think you need to worry about pre-creating sysctls if they are written to. I think it is perfectly reasonable to only create the sysctl when the VFS module is loaded (and that's more typical). I think it is cleaner instead of allocating pr_allow bits on the fly, to instead use a flag in the 'struct vfsconf' to be the jail permission. You could just add a new 'VFCF_JAIL_ALLOW' which is a dynamic flag that the sysctl knobs turn on/off. The sysctl node would be a SYSCTL_PROC handler and it can take a pointer to the 'struct vfsconf' as its arg2 value. This avoids concerns about running out of bits, etc. For this you would want to change prison_check_vfs() to take a pointer to 'struct vfsconf' instead of the name.