Re: What to do about a few lines in vfs_domount() never executed?

On Wed, Dec 14, 2022 at 2:04 PM Rick Macklem <rick.macklem@gmail.com> wrote:

>
>
> On Wed, Dec 14, 2022 at 9:15 AM Tomoaki AOKI <junchoon@dec.sakura.ne.jp>
> wrote:
>
>> Tracking the commits, it was originally introduced to
>> sys/kern/vfs_syscalls.c at r22521 [1][2] (Mon Feb 10 02:22:35 1997 by
>> dyson, submitted by hsu@freebsd.org) and later centralized into
>> sys/kern/vfs_mount.c at r99264 [2].
>>
>> But according to the comment above the codes, maybe it would be
>> intended to block userland programs or ports FS modules setting
>> MNT_EXPORTED.
>>
>> If I'm not mis-understanding, it can be the case when
>>  *vfs.usermount sysctl is non-zero,
>>  *underlying FS (to be exported) allows it, and
>>  *non-root user tries to mount the FS via NFS.
>>
> It does appear that ancient code restricted doing NFS exports
> to root only.
>
It looks like the semantics change was introduced when mountd
as converted to nmount() { r158857 about 17years ago }.
The code in vfs_mount.c assumed that MNT_EXPORTED was set in the
argument passed in from mountd.c when it called nmount(), but
that was not the case.
--> As such, the check was not performed.

The check was for suser() until r164033 when it was replaced
by priv_check(td, PRIV_VFS_MOUNT_EDPORTED).
--> This does the same thing, failing if cr_uid != 0.

So, I think the snippet was supposed to enforce "only root
can set exports", but the check has not worked post r158857
because MNT_EXPORTED was never set in fsflags.
(nmount() uses the "export" option.)

So, should I set MNT_EXPORTED in fsflags when nmount()
has specified the "export" option and restore the
"must be root to export" check?

rick
ps: Thanks Tomoaki AOKI for looking at the old code and
     spotting this.

> I don't think that restriction is exactly enforced now, since
> vfs_suser() allows a non-root owner to do the update (which
> would include updating exports).
> (As I noted, MNT_EXPORTED never gets set in fsflags. The variable
>  is local to one of the functions in vfs_mount.c and a search shows
>  it never gets set.)
>
> I suppose you could argue that priv_check(td,  PRIV_VFS_MOUNT_EXPORTED)
> should check for caller being root, since that is what ancient code did.
> Or, you could argue that, if a non-root user is allowed to mount a file
> system that they should also be allowed to export it, which is what I
> think the current code allows (and has for at least a decade).
>
> Admittedly, allowing a non-root user to do a mount and also add exports
> to it could cause confusion, since the system basically assumes that
> mountd will manage all exports.
>
> Do others think adding code to check that cr_uid == 0 for
> PRIV_VFS_MOUNT_EXPORTED to be allowed makes sense?
>
> rick
>
>
>
>>
>> [1]
>>
>> https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?revision=22521&view=markup&pathrev=99264
>>
>> [2]
>>
>> https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?r1=22520&r2=22521&pathrev=99264&
>>
>> [3]
>>
>> https://cgit.freebsd.org/src/commit/sys/kern/vfs_mount.c?id=2b4edb69f1ef62fc38b02ac22b0a3ac09e43fa77
>>
>>
>> On Tue, 13 Dec 2022 14:19:39 -0800
>> Rick Macklem <rick.macklem@gmail.com> wrote:
>>
>> > Hi,
>> >
>> > While working on getting mountd/nfsd to run in a vnet
>> > prison, I came across the following lines near the
>> > beginning of vfs_domount() in sys/kern/vfs_mount.c:
>> >
>> > if (fsflags & MNT_EXPORTED) {
>> >      error = priv_check(td, PRIV_VFS_MOUNT_EXPORTED);
>> >      if (error)
>> >            return (error);
>> > }
>> >
>> > #1 - Since MNT_EXPORTED is never set in fsflags, this code never
>> >      gets executed.
>> >      --> I am asking what to do with the above code, since that
>> >          changes for the patch that allows mountd to run in a vnet
>> >          prison.
>> > #2 - priv_check(td, PRIV_VFS_MOUNT_EXPORTED) always returns 0
>> >      because nothing in sys/kern/kern_priv.c checks
>> >      PRIV_VFS_MOUNT_EXPORTED.
>> >
>> > I don't know what the original author's thinking was w.r.t. this.
>> > Setting exports already checks that the mount operation can be
>> > done by the requestor.
>> >
>> > So, what do you think should be done with the above code snippet?
>> > - Consider it cruft and delete it.
>> > - Try and figure out what PRIV_VFS_MOUNT_EXPORTED should check?
>> > - Leave it as is. After the patch that allows mountd to run in
>> >   a vnet prison, MNT_EXPORTED will be set in fsflags, but the
>> >   priv_check() call will just return 0. (A little overhead,
>> >   but otherwise no semantics change.)
>> >
>> > rick
>>
>>
>> --
>> Tomoaki AOKI    <junchoon@dec.sakura.ne.jp>
>>
>>