From nobody Thu Dec 15 00:42:47 2022 X-Original-To: freebsd-current@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4NXYP427bqz4l0s6 for ; Thu, 15 Dec 2022 00:43:04 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Received: from mail-pl1-x62e.google.com (mail-pl1-x62e.google.com [IPv6:2607:f8b0:4864:20::62e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4NXYP32BcCz3Jp4 for ; Thu, 15 Dec 2022 00:43:03 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=qlrCqzSr; spf=pass (mx1.freebsd.org: domain of rick.macklem@gmail.com designates 2607:f8b0:4864:20::62e as permitted sender) smtp.mailfrom=rick.macklem@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pl1-x62e.google.com with SMTP id l10so5184111plb.8 for ; Wed, 14 Dec 2022 16:43:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=AEA2BUpfluP6ogZx8bDAfse1XGuAkoud8hvOsYE9MwU=; b=qlrCqzSrt5wbhAN1AWsBDTyJta/w9pnZfX7tL5Q+OgYf3JhtzcKt/DF9MLOSkwsB/F BwzRVg9fa6u7Rhm0U6ocCyYsi9gogGPqo3eYHtYGbdT2JTrV0p5KW1x/tJ7tNWDsQAXj HtFfFUP1J7w9j7ZO4HN5HKuDwq5WkBNZYX76/Rg0lI6rs3TIQVkuF6C0SI2IHhLtbVih 9bf+5zn3K8yK+7SXeLoIkEe4fs78AMwydKShiIZQQHrkPRf1dp0Ll+RPM0YIpggv8oJd NuaPc3YYuhGTyMMl3KIasWHmyp19QJ0NdNVSYRb57zdywgvbwWZGwlmD1jKie8gYBNRh xCgQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=AEA2BUpfluP6ogZx8bDAfse1XGuAkoud8hvOsYE9MwU=; b=ahE2RClibL7R9QnDvkpa0YVmJpS3FDgbgy15iGCRdkQm6dvWkhtRD+WLNRAyND5Kpf iIUbtjpqxcrwNIAEIT38nKltEgyc+i9xAMoM/0oY87z/ma08atNmTQILNKuf0Zp2ionY 5tVWl9EjQTGEf6uvbKucPmkTAds3gYBgZZDqV8DlKo7r1hWNxSQzPd9mYrHNJhJ+fXNu gOWh/2Uw2FvCeb2ZXhHMcnCmRbuDJbeKxvAsfdJ03GzR0FQtfQgSj2XC8852IESQcI5A nZuWNL3oSS+YAvZwrCd3GI+55w3CxnmBTcy3ScCSAexSIoKHr7++SExsDABpjP+2WSqM jMJQ== X-Gm-Message-State: ANoB5plXBlIpWsFCcl9HVuqYsBrFzRhgI48yNtD+t18SUTO/PCVH5+mH LG1tqXE7V3/gNKv6K8RrHzi4bkxtKUWy3SPPPwFcBaQnmw== X-Google-Smtp-Source: AA0mqf64zSVZSZ7YhTyqlAsredcMuKUOrNPgG6XTsFXtryGcSkNHANtiQmEzIs66mJ3zPiNWmZjCBNGXcyIOLs9zLks= X-Received: by 2002:a17:902:d585:b0:189:9fb2:2541 with SMTP id k5-20020a170902d58500b001899fb22541mr44704449plh.60.1671064982203; Wed, 14 Dec 2022 16:43:02 -0800 (PST) List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 References: <20221215021431.d190e55ee911f5e94799f953@dec.sakura.ne.jp> In-Reply-To: From: Rick Macklem Date: Wed, 14 Dec 2022 16:42:47 -0800 Message-ID: Subject: Re: What to do about a few lines in vfs_domount() never executed? To: Tomoaki AOKI Cc: freebsd-current@freebsd.org Content-Type: multipart/alternative; boundary="000000000000a92fe205efd322f6" X-Spamd-Result: default: False [-2.70 / 15.00]; SUBJECT_ENDS_QUESTION(1.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-0.88)[-0.879]; NEURAL_HAM_SHORT(-0.82)[-0.817]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; PREVIOUSLY_DELIVERED(0.00)[freebsd-current@freebsd.org]; ARC_NA(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::62e:from]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_TLS_LAST(0.00)[]; TAGGED_FROM(0.00)[]; MLMMJ_DEST(0.00)[freebsd-current@freebsd.org]; DKIM_TRACE(0.00)[gmail.com:+]; MID_RHS_MATCH_FROMTLD(0.00)[]; FREEMAIL_FROM(0.00)[gmail.com]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim]; RCPT_COUNT_TWO(0.00)[2]; MIME_TRACE(0.00)[0:+,1:+,2:~]; FROM_EQ_ENVFROM(0.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FREEMAIL_ENVFROM(0.00)[gmail.com]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Queue-Id: 4NXYP32BcCz3Jp4 X-Spamd-Bar: -- X-ThisMailContainsUnwantedMimeParts: N --000000000000a92fe205efd322f6 Content-Type: text/plain; charset="UTF-8" On Wed, Dec 14, 2022 at 2:04 PM Rick Macklem wrote: > > > On Wed, Dec 14, 2022 at 9:15 AM Tomoaki AOKI > 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 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 >> >> --000000000000a92fe205efd322f6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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

<= /div>
O= n Wed, Dec 14, 2022 at 9:15 AM Tomoaki AOKI <junchoon@dec.sakura.ne.jp> wrote= :
Tracking the c= ommits, 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 hs= u@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
=C2=A0*vfs.usermount sysctl is non-zero,
=C2=A0*underlying FS (to be exported) allows it, and
=C2=A0*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 m= ountd
as converted to nmount() { r158857 about 17years ago }.
The c= ode in vfs_mount.c assumed that MNT_EXPORTED was set in the
argument pa= ssed in from mountd.c when it called nmount(), but
that was not the cas= e.
--> As such, the check was not performed.

The check = was for suser() until r164033 when it was replaced
by priv_check(td, PR= IV_VFS_MOUNT_EDPORTED).
-->=C2=A0This does the same thing, failing if cr_u= id !=3D 0.

So, I think the snippet was supposed to enforce &qu= ot;only root
can set exports", but the check has not worked post r= 158857
because MNT_EXPORTED was never set in fsflags.
= (nmount() use= s the "export" option.)

So, should I set MNT_EXPORTE= D in fsflags when nmount()
has specified the "export" option = and restore the
"must be root to export" check?
<= div>
= rick
ps: Thanks Tomoaki AOKI for looking at the old code and
=C2=A0= =C2=A0 =C2=A0spotting this.
I don't think = that restriction is exactly enforced now, since
vfs_suser() allows a no= n-root owner to do the update (which
would include updating exports).
(As I noted, MNT_EXPORTED never gets set in fsflags. The variable=
= =C2=A0is local to one of the functions in vfs_mount.c and a search shows
=C2=A0it never gets set.)

I suppose you could argue that pri= v_check(td, =C2=A0PRIV_VFS_MOUNT_EXPORTED)
should check for caller being root= , since that is what ancient code did.
Or, you could argue that, if a n= on-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 (a= nd 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, s= ince the system basically assumes that
mountd will manage all exports.<= /span>

Do others think adding code to check that cr_uid =3D=3D 0 for<= /span>
PRIV_VFS_MOUNT_EXPORTED to be allowed makes sense?

rick




[1]
https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c= ?revision=3D22521&view=3Dmarkup&pathrev=3D99264

[2]
https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?r1= =3D22520&r2=3D22521&pathrev=3D99264&

[3]
https://cgit.freebsd.org/src/commit/sys/kern/vfs_mount.c?id=3D2b4edb69f1e= f62fc38b02ac22b0a3ac09e43fa77


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) {
>=C2=A0 =C2=A0 =C2=A0 error =3D priv_check(td, PRIV_VFS_MOUNT_EXPORTED);=
>=C2=A0 =C2=A0 =C2=A0 if (error)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (error);
> }
>
> #1 - Since MNT_EXPORTED is never set in fsflags, this code never
>=C2=A0 =C2=A0 =C2=A0 gets executed.
>=C2=A0 =C2=A0 =C2=A0 --> I am asking what to do with the above code,= since that
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 changes for the patch that allows mo= untd to run in a vnet
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 prison.
> #2 - priv_check(td, PRIV_VFS_MOUNT_EXPORTED) always returns 0
>=C2=A0 =C2=A0 =C2=A0 because nothing in sys/kern/kern_priv.c checks
>=C2=A0 =C2=A0 =C2=A0 PRIV_VFS_MOUNT_EXPORTED.
>
> I don't know what the original author's thinking was w.r.t. th= is.
> 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
>=C2=A0 =C2=A0a vnet prison, MNT_EXPORTED will be set in fsflags, but th= e
>=C2=A0 =C2=A0priv_check() call will just return 0. (A little overhead,<= br> >=C2=A0 =C2=A0but otherwise no semantics change.)
>
> rick


--
Tomoaki AOKI=C2=A0 =C2=A0 <junchoon@dec.sakura.ne.jp>

--000000000000a92fe205efd322f6--