Date: Wed, 14 Dec 2022 14:04:12 -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: <CAM5tNy6gAFn5GHXycsNRXqTyWM21ALq-G70VrFLRrwnCi1EGyg@mail.gmail.com> In-Reply-To: <20221215021431.d190e55ee911f5e94799f953@dec.sakura.ne.jp> References: <CAM5tNy7DM__tj4CQ0L_-ugo5krmEvLJgbU-WFLs47MrRBuGaNQ@mail.gmail.com> <20221215021431.d190e55ee911f5e94799f953@dec.sakura.ne.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
--0000000000008216dc05efd0eb0b Content-Type: text/plain; charset="UTF-8" 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. 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> > > --0000000000008216dc05efd0eb0b 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 9:15 AM Tomoaki AOKI = <<a href=3D"mailto:junchoon@dec.sakura.ne.jp">junchoon@dec.sakura.ne.jp<= /a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0= px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">T= racking the commits, 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><span class=3D"gmail_default" style=3D"font-family:monospace">I don&= #39;t think that restriction is exactly enforced now, since</span></div><di= v><span class=3D"gmail_default" style=3D"font-family:monospace">vfs_suser()= allows a non-root owner to do the update (which</span></div><div><span cla= ss=3D"gmail_default" style=3D"font-family:monospace">would include updating= exports).</span></div><div><span class=3D"gmail_default" style=3D"font-fam= ily:monospace">(As I noted, MNT_EXPORTED never gets set in fsflags. The var= iable</span></div><div><span class=3D"gmail_default" style=3D"font-family:m= onospace">=C2=A0is local to one of the functions in vfs_mount.c and a searc= h shows</span></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 argu= e that priv_check(td, </span>=C2=A0<span class=3D"gmail_default" style=3D"f= ont-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 cla= ss=3D"gmail_default" style=3D"font-family:monospace">Or, you could argue th= at, if a non-root user is allowed to mount a file</span></div><div><span cl= ass=3D"gmail_default" style=3D"font-family:monospace">system that they shou= ld also be allowed to export it, which is what I</span></div><div><span cla= ss=3D"gmail_default" style=3D"font-family:monospace">think the current code= allows (and has for at least a decade).</span></div><div><span class=3D"gm= ail_default" style=3D"font-family:monospace"><br></span></div><div><span cl= ass=3D"gmail_default" style=3D"font-family:monospace">Admittedly, allowing = a non-root user to do a mount and also add exports</span></div><div><span c= lass=3D"gmail_default" style=3D"font-family:monospace">to it could cause co= nfusion, since the system basically assumes that</span></div><div><span cla= ss=3D"gmail_default" style=3D"font-family:monospace">mountd will manage all= exports.</span></div><div><span class=3D"gmail_default" style=3D"font-fami= ly:monospace"><br></span></div><div><span class=3D"gmail_default" style=3D"= font-family: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-fami= ly:monospace">PRIV_VFS_MOUNT_EXPORTED to be allowed makes sense?</span></di= v><div><span class=3D"gmail_default" style=3D"font-family:monospace"><br></= span></div><div><span class=3D"gmail_default" style=3D"font-family:monospac= e">rick</span></div><div><span class=3D"gmail_default" style=3D"font-family= :monospace"><br></span></div><div><span class=3D"gmail_default" style=3D"fo= nt-family:monospace"><br></span></div><blockquote class=3D"gmail_quote" sty= le=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddi= ng-left:1ex"> <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> --0000000000008216dc05efd0eb0b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy6gAFn5GHXycsNRXqTyWM21ALq-G70VrFLRrwnCi1EGyg>