Date: Tue, 31 Oct 2006 16:27:57 +0100 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: arch@FreeBSD.org Subject: Re: New in-kernel privilege API: priv(9) Message-ID: <20061031152757.GD13359@garage.freebsd.pl> In-Reply-To: <20061031144237.B87421@fledge.watson.org> References: <20061031092122.D96078@fledge.watson.org> <20061031144050.GC13359@garage.freebsd.pl> <20061031144237.B87421@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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:
>=20
> On Tue, 31 Oct 2006, Pawel Jakub Dawidek wrote:
>=20
> >Here are few nits I found:
>=20
> Thanks! Comments below.
>=20
> >>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=
=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
> >>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=
VAL) {
> >> 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);
> >
> >Eliminating privilege check here was intentional? Doesn't it change func=
tionality? I found the same check in few other places, so it's probably int=
entional, but worth=20
> >checking still.
>=20
> Yeah, it took my quite a while to puzzle through what the security check =
here is supposed to accomplish, but once I did, the correct structure becam=
e more clear. The key=20
> here is that VOP_ACCESS() will also perform a privilege check, so the use=
of privilege in the existing code appears to be redundant. The logic here=
should essentially=20
> 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=
t the
> 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?
Yes, I agree. I really hate complex ifs like the one you replaced:)
> >>+ 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);
> >
> >This doesn't seem right to me. If VOP_ACCESS() returns an error if call =
priv_check(), I think you wanted to call it when it return success, no?
> >
> >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);
>=20
> Again, this is a tricky case in the original code. I believe the intende=
d logic here, in English, is:
>=20
> - In order to mount on a device vnode, you must be able to read and write=
to
> it. This is subject to the normal definition of read and write privile=
ge,
> 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=
ilege
> (PRIV_VFS_MOUNT_PERM) to override that.
>=20
> This is expressed in the original code by first checking for privilege, a=
nd if that fails, then looking for read/write access. This doesn't make a =
lot of sense because the=20
> read/write check is also exempted by privilege, but was probably "a littl=
e faster" because suser() was cheaper than VOP_ACCESS().
>=20
> The reason we call priv_check() in the error case is that we only want to=
check for privilege if the normal access control check fails, in which cas=
e we override the normal=20
> access control error check with the result of the privilege check.
>=20
> Given this description, does the new code still make sense?
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 http://www.wheel.pl
pjd@FreeBSD.org http://www.FreeBSD.org
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061031152757.GD13359>