Date: Wed, 14 Dec 2022 16:42:47 -0800 From: Rick Macklem <rick.macklem@gmail.com> To: Tomoaki AOKI <junchoon@dec.sakura.ne.jp> Cc: freebsd-current@freebsd.org Subject: Re: What to do about a few lines in vfs_domount() never executed? Message-ID: <CAM5tNy5DZ0%2Bw39SmLKHcSkbh_mLWx%2BwkLN_xRuc3W__Z8UaFyg@mail.gmail.com> In-Reply-To: <CAM5tNy6gAFn5GHXycsNRXqTyWM21ALq-G70VrFLRrwnCi1EGyg@mail.gmail.com> References: <CAM5tNy7DM__tj4CQ0L_-ugo5krmEvLJgbU-WFLs47MrRBuGaNQ@mail.gmail.com> <20221215021431.d190e55ee911f5e94799f953@dec.sakura.ne.jp> <CAM5tNy6gAFn5GHXycsNRXqTyWM21ALq-G70VrFLRrwnCi1EGyg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000a92fe205efd322f6
Content-Type: text/plain; charset="UTF-8"
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>
>>
>>
--000000000000a92fe205efd322f6
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><div class=3D"gmail_default" style=3D"fon=
t-family:monospace"><br></div></div><br><div class=3D"gmail_quote"><div dir=
=3D"ltr" class=3D"gmail_attr">On Wed, Dec 14, 2022 at 2:04 PM Rick Macklem =
<<a href=3D"mailto:rick.macklem@gmail.com">rick.macklem@gmail.com</a>>=
; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px=
0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div di=
r=3D"ltr"><div dir=3D"ltr"><div style=3D"font-family:monospace"><br></div><=
/div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">O=
n Wed, Dec 14, 2022 at 9:15 AM Tomoaki AOKI <<a href=3D"mailto:junchoon@=
dec.sakura.ne.jp" target=3D"_blank">junchoon@dec.sakura.ne.jp</a>> wrote=
:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.=
8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Tracking the c=
ommits, it was originally introduced to<br>
sys/kern/vfs_syscalls.c at r22521 [1][2] (Mon Feb 10 02:22:35 1997 by<br>
dyson, submitted by <a href=3D"mailto:hsu@freebsd.org" target=3D"_blank">hs=
u@freebsd.org</a>) and later centralized into<br>
sys/kern/vfs_mount.c at r99264 [2].<br>
<br>
But according to the comment above the codes, maybe it would be<br>
intended to block userland programs or ports FS modules setting <br>
MNT_EXPORTED.<br>
<br>
If I'm not mis-understanding, it can be the case when<br>
=C2=A0*vfs.usermount sysctl is non-zero,<br>
=C2=A0*underlying FS (to be exported) allows it, and<br>
=C2=A0*non-root user tries to mount the FS via NFS.<br></blockquote><div><s=
pan class=3D"gmail_default" style=3D"font-family:monospace">It does appear =
that ancient code restricted doing NFS exports</span></div><div><span class=
=3D"gmail_default" style=3D"font-family:monospace">to root only.</span></di=
v></div></div></blockquote><div><span class=3D"gmail_default" style=3D"font=
-family:monospace">It looks like the semantics change was introduced when m=
ountd</span></div><div><span class=3D"gmail_default" style=3D"font-family:m=
onospace">as converted to nmount() { r158857 about 17years ago }.</span></d=
iv><div><span class=3D"gmail_default" style=3D"font-family:monospace">The c=
ode in vfs_mount.c assumed that MNT_EXPORTED was set in the</span></div><di=
v><span class=3D"gmail_default" style=3D"font-family:monospace">argument pa=
ssed in from mountd.c when it called nmount(), but</span></div><div><span c=
lass=3D"gmail_default" style=3D"font-family:monospace">that was not the cas=
e.</span></div><div><span class=3D"gmail_default" style=3D"font-family:mono=
space">--> As such, the check was not performed.</span></div><div><span =
class=3D"gmail_default" style=3D"font-family:monospace"><br></span></div><d=
iv><span class=3D"gmail_default" style=3D"font-family:monospace">The check =
was for suser() until r164033 when it was replaced</span></div><div><span c=
lass=3D"gmail_default" style=3D"font-family:monospace">by priv_check(td, PR=
IV_VFS_MOUNT_EDPORTED).</span></div><div><span class=3D"gmail_default" styl=
e=3D"font-family:monospace">--></span>=C2=A0<span class=3D"gmail_default=
" style=3D"font-family:monospace">This does the same thing, failing if cr_u=
id !=3D 0.</span></div><div><span class=3D"gmail_default" style=3D"font-fam=
ily:monospace"><br></span></div><div><span class=3D"gmail_default" style=3D=
"font-family:monospace">So, I think the snippet was supposed to enforce &qu=
ot;only root</span></div><div><span class=3D"gmail_default" style=3D"font-f=
amily:monospace">can set exports", but the check has not worked post r=
158857</span></div><div><span class=3D"gmail_default" style=3D"font-family:=
monospace">because MNT_EXPORTED was never set in fsflags.</span></div><div>=
<span class=3D"gmail_default" style=3D"font-family:monospace">(nmount() use=
s the "export" option.)</span></div><div><span class=3D"gmail_def=
ault" style=3D"font-family:monospace"><br></span></div><div><span class=3D"=
gmail_default" style=3D"font-family:monospace">So, should I set MNT_EXPORTE=
D in fsflags when nmount()</span></div><div><span class=3D"gmail_default" s=
tyle=3D"font-family:monospace">has specified the "export" option =
and restore the</span></div><div><span class=3D"gmail_default" style=3D"fon=
t-family:monospace">"must be root to export" check?</span></div><=
div><span class=3D"gmail_default" style=3D"font-family:monospace"><br></spa=
n></div><div><span class=3D"gmail_default" style=3D"font-family:monospace">=
rick</span></div><div><span class=3D"gmail_default" style=3D"font-family:mo=
nospace">ps: Thanks Tomoaki AOKI for looking at the old code and</span></di=
v><div><span class=3D"gmail_default" style=3D"font-family:monospace">=C2=A0=
=C2=A0 =C2=A0spotting this.</span></div><div><span class=3D"gmail_default"=
style=3D"font-family:monospace"></span></div><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,20=
4);padding-left:1ex"><div dir=3D"ltr"><div class=3D"gmail_quote"><div><span=
class=3D"gmail_default" style=3D"font-family:monospace">I don't think =
that restriction is exactly enforced now, since</span></div><div><span clas=
s=3D"gmail_default" style=3D"font-family:monospace">vfs_suser() allows a no=
n-root owner to do the update (which</span></div><div><span class=3D"gmail_=
default" style=3D"font-family:monospace">would include updating exports).</=
span></div><div><span class=3D"gmail_default" style=3D"font-family:monospac=
e">(As I noted, MNT_EXPORTED never gets set in fsflags. The variable</span>=
</div><div><span class=3D"gmail_default" style=3D"font-family:monospace">=
=C2=A0is local to one of the functions in vfs_mount.c and a search shows</s=
pan></div><div><span class=3D"gmail_default" style=3D"font-family:monospace=
">=C2=A0it never gets set.)</span></div><div><span class=3D"gmail_default" =
style=3D"font-family:monospace"><br></span></div><div><span class=3D"gmail_=
default" style=3D"font-family:monospace">I suppose you could argue that pri=
v_check(td, </span>=C2=A0<span class=3D"gmail_default" style=3D"font-family=
:monospace">PRIV_VFS_MOUNT_EXPORTED)</span></div><div><span class=3D"gmail_=
default" style=3D"font-family:monospace">should check for caller being root=
, since that is what ancient code did.</span></div><div><span class=3D"gmai=
l_default" style=3D"font-family:monospace">Or, you could argue that, if a n=
on-root user is allowed to mount a file</span></div><div><span class=3D"gma=
il_default" style=3D"font-family:monospace">system that they should also be=
allowed to export it, which is what I</span></div><div><span class=3D"gmai=
l_default" style=3D"font-family:monospace">think the current code allows (a=
nd has for at least a decade).</span></div><div><span class=3D"gmail_defaul=
t" style=3D"font-family:monospace"><br></span></div><div><span class=3D"gma=
il_default" style=3D"font-family:monospace">Admittedly, allowing a non-root=
user to do a mount and also add exports</span></div><div><span class=3D"gm=
ail_default" style=3D"font-family:monospace">to it could cause confusion, s=
ince the system basically assumes that</span></div><div><span class=3D"gmai=
l_default" style=3D"font-family:monospace">mountd will manage all exports.<=
/span></div><div><span class=3D"gmail_default" style=3D"font-family:monospa=
ce"><br></span></div><div><span class=3D"gmail_default" style=3D"font-famil=
y:monospace">Do others think adding code to check that cr_uid =3D=3D 0 for<=
/span></div><div><span class=3D"gmail_default" style=3D"font-family:monospa=
ce">PRIV_VFS_MOUNT_EXPORTED to be allowed makes sense?</span></div><div><sp=
an class=3D"gmail_default" style=3D"font-family:monospace"><br></span></div=
><div><span class=3D"gmail_default" style=3D"font-family:monospace">rick</s=
pan></div><div><span class=3D"gmail_default" style=3D"font-family:monospace=
"><br></span></div><div><span class=3D"gmail_default" style=3D"font-family:=
monospace"><br></span></div><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e=
x">
<br>
<br>
[1]<br>
<a href=3D"https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?rev=
ision=3D22521&view=3Dmarkup&pathrev=3D99264" rel=3D"noreferrer" tar=
get=3D"_blank">https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c=
?revision=3D22521&view=3Dmarkup&pathrev=3D99264</a><br>
<br>
[2]<br>
<a href=3D"https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?r1=
=3D22520&r2=3D22521&pathrev=3D99264&" rel=3D"noreferrer" target=
=3D"_blank">https://svnweb.freebsd.org/base/head/sys/kern/vfs_syscalls.c?r1=
=3D22520&r2=3D22521&pathrev=3D99264&</a><br>
<br>
[3]<br>
<a href=3D"https://cgit.freebsd.org/src/commit/sys/kern/vfs_mount.c?id=3D2b=
4edb69f1ef62fc38b02ac22b0a3ac09e43fa77" rel=3D"noreferrer" target=3D"_blank=
">https://cgit.freebsd.org/src/commit/sys/kern/vfs_mount.c?id=3D2b4edb69f1e=
f62fc38b02ac22b0a3ac09e43fa77</a><br>
<br>
<br>
On Tue, 13 Dec 2022 14:19:39 -0800<br>
Rick Macklem <<a href=3D"mailto:rick.macklem@gmail.com" target=3D"_blank=
">rick.macklem@gmail.com</a>> wrote:<br>
<br>
> Hi,<br>
> <br>
> While working on getting mountd/nfsd to run in a vnet<br>
> prison, I came across the following lines near the<br>
> beginning of vfs_domount() in sys/kern/vfs_mount.c:<br>
> <br>
> if (fsflags & MNT_EXPORTED) {<br>
>=C2=A0 =C2=A0 =C2=A0 error =3D priv_check(td, PRIV_VFS_MOUNT_EXPORTED);=
<br>
>=C2=A0 =C2=A0 =C2=A0 if (error)<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (error);<br>
> }<br>
> <br>
> #1 - Since MNT_EXPORTED is never set in fsflags, this code never<br>
>=C2=A0 =C2=A0 =C2=A0 gets executed.<br>
>=C2=A0 =C2=A0 =C2=A0 --> I am asking what to do with the above code,=
since that<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 changes for the patch that allows mo=
untd to run in a vnet<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 prison.<br>
> #2 - priv_check(td, PRIV_VFS_MOUNT_EXPORTED) always returns 0<br>
>=C2=A0 =C2=A0 =C2=A0 because nothing in sys/kern/kern_priv.c checks<br>
>=C2=A0 =C2=A0 =C2=A0 PRIV_VFS_MOUNT_EXPORTED.<br>
> <br>
> I don't know what the original author's thinking was w.r.t. th=
is.<br>
> Setting exports already checks that the mount operation can be<br>
> done by the requestor.<br>
> <br>
> So, what do you think should be done with the above code snippet?<br>
> - Consider it cruft and delete it.<br>
> - Try and figure out what PRIV_VFS_MOUNT_EXPORTED should check?<br>
> - Leave it as is. After the patch that allows mountd to run in<br>
>=C2=A0 =C2=A0a vnet prison, MNT_EXPORTED will be set in fsflags, but th=
e<br>
>=C2=A0 =C2=A0priv_check() call will just return 0. (A little overhead,<=
br>
>=C2=A0 =C2=A0but otherwise no semantics change.)<br>
> <br>
> rick<br>
<br>
<br>
-- <br>
Tomoaki AOKI=C2=A0 =C2=A0 <<a href=3D"mailto:junchoon@dec.sakura.ne.jp" =
target=3D"_blank">junchoon@dec.sakura.ne.jp</a>><br>
<br>
</blockquote></div></div>
</blockquote></div></div>
--000000000000a92fe205efd322f6--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy5DZ0%2Bw39SmLKHcSkbh_mLWx%2BwkLN_xRuc3W__Z8UaFyg>