Re: New in-kernel privilege API: priv(9)

--5xSkJheCpeK0RUEJ
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Oct 31, 2006 at 03:04:30PM +0000, Robert Watson wrote:[color=blue]
>=20
> On Tue, 31 Oct 2006, Pawel Jakub Dawidek wrote:
>=20[color=green]
> >Here are few nits I found:[/color]
>=20
> Thanks! Comments below.
>=20[color=green][color=darkred]
> >>Index: sys/fs/hpfs/hpfs_vnops.c
> >>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=[/color][/color][/color]
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D[color=blue][color=green][color=darkred]
> >>RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/hpfs/hpfs_vnops.c,v
> >>retrieving revision 1.68
> >>diff -u -r1.68 hpfs_vnops.c
> >>--- sys/fs/hpfs/hpfs_vnops.c 17 Jan 2006 17:29:01 -0000 1.68
> >>+++ sys/fs/hpfs/hpfs_vnops.c 30 Oct 2006 17:07:55 -0000
> >>@@ -501,11 +501,12 @@
> >> if (vap->va_atime.tv_sec !=3D VNOVAL || vap->va_mtime.tv_sec !=3D VNO=[/color][/color][/color]
VAL) {[color=blue][color=green][color=darkred]
> >> if (vp->v_mount->mnt_flag & MNT_RDONLY)
> >> return (EROFS);
> >>- if (cred->cr_uid !=3D hp->h_uid &&
> >>- (error =3D suser_cred(cred, SUSER_ALLOWJAIL)) &&
> >>- ((vap->va_vaflags & VA_UTIMES_NULL) =3D=3D 0 ||
> >>- (error =3D VOP_ACCESS(vp, VWRITE, cred, td))))
> >>- return (error);
> >>+ if (vap->va_vaflags & VA_UTIMES_NULL) {
> >>+ error =3D VOP_ACCESS(vp, VADMIN, cred, td);
> >>+ if (error)
> >>+ error =3D VOP_ACCESS(vp, VWRITE, cred, td);
> >>+ } else
> >>+ error =3D VOP_ACCESS(vp, VADMIN, cred, td);[/color]
> >
> >Eliminating privilege check here was intentional? Doesn't it change func=[/color][/color]
tionality? I found the same check in few other places, so it's probably int=
entional, but worth=20[color=blue][color=green]
> >checking still.[/color]
>=20
> Yeah, it took my quite a while to puzzle through what the security check =[/color]
here is supposed to accomplish, but once I did, the correct structure becam=
e more clear. The key=20[color=blue]
> here is that VOP_ACCESS() will also perform a privilege check, so the use=[/color]
of privilege in the existing code appears to be redundant. The logic here=
should essentially=20[color=blue]
> read:
>=20
> - To update the time stamp to the current time (VA_UTIMES_NULL), you must
> either be the owner or have write access. The correct error value is
> EACCES, so we try the write check only if the owner check fails, so tha=[/color]
t the[color=blue]
> error from the write check overrides the owner result.
>=20
> - To update the time stamp to any other time, you must be the owner. The
> error here is EPERM on failure.
>=20
> Given this description, do you think the change is right?[/color]

Yes, I agree. I really hate complex ifs like the one you replaced:)
[color=blue][color=green][color=darkred]
> >>+ error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> >>+ td->td_ucred, td);
> >>+ if (error)
> >>+ error =3D priv_check(td, PRIV_VFS_MOUNT_PERM);
> >>+ if (error) {
> >> VOP_UNLOCK(devvp, 0, td);
> >>+ return (error);
> >> }
> >>+ VOP_UNLOCK(devvp, 0, td);[/color]
> >
> >This doesn't seem right to me. If VOP_ACCESS() returns an error if call =[/color][/color]
priv_check(), I think you wanted to call it when it return success, no?[color=blue][color=green]
> >
> >I'd change it to:
> >
> > devvp =3D pmp->pm_devvp;
> > vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> > error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> > td->td_ucred, td);
> > VOP_UNLOCK(devvp, 0, td);
> > if (!error)
> > error =3D priv_check(td, PRIV_VFS_MOUNT_PERM);
> > if (error)
> > return (error);[/color]
>=20
> Again, this is a tricky case in the original code. I believe the intende=[/color]
d logic here, in English, is:[color=blue]
>=20
> - In order to mount on a device vnode, you must be able to read and write=[/color]
to[color=blue]
> it. This is subject to the normal definition of read and write privile=[/color]
ge,[color=blue]
> so the superuser can override on that basis (as part of VOP_ACCESS).
>=20
> - However, if you don't have read and write access, you can also use priv=[/color]
ilege[color=blue]
> (PRIV_VFS_MOUNT_PERM) to override that.
>=20
> This is expressed in the original code by first checking for privilege, a=[/color]
nd if that fails, then looking for read/write access. This doesn't make a =
lot of sense because the=20[color=blue]
> read/write check is also exempted by privilege, but was probably "a littl=[/color]
e faster" because suser() was cheaper than VOP_ACCESS().[color=blue]
>=20
> The reason we call priv_check() in the error case is that we only want to=[/color]
check for privilege if the normal access control check fails, in which cas=
e we override the normal=20[color=blue]
> access control error check with the result of the privilege check.
>=20
> Given this description, does the new code still make sense?[/color]

After more thinking I understand and agree with your change, maybe we
can add a comment describing the behaviour as it is not obvious on first
look.

We could still move VOP_UNLOCK() right after VOP_ACCESS(), BTW.

--=20
Pawel Jakub Dawidek [url]http://www.wheel.pl[/url]
[email]pjd@FreeBSD.org[/email] [url]http://www.FreeBSD.org[/url]
FreeBSD committer Am I Evil? Yes, I Am!

--5xSkJheCpeK0RUEJ
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFR2t9ForvXbEpPzQRAsdtAJ9YGXxSAZqi/hx/JaklroluVCeTLgCePcfg
oe5V/iIWlo3ssmAp+S4jNQQ=
=lrJ7
-----END PGP SIGNATURE-----

--5xSkJheCpeK0RUEJ--