--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=3 D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3 D=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--