Date: Mon, 25 Feb 2013 00:59:36 +0100 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Christoph Mallon <christoph.mallon@gmx.de> Cc: freebsd-arch@FreeBSD.org Subject: Re: Large Capsicum patch for review. Message-ID: <20130224235936.GX1377@garage.freebsd.pl> In-Reply-To: <512A2CA0.2050509@gmx.de> References: <20130213025547.GA2025@garage.freebsd.pl> <20130213230221.GB1375@garage.freebsd.pl> <20130223221116.GR1377@garage.freebsd.pl> <5129ADC5.5040306@gmx.de> <512A2CA0.2050509@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
--yTwVabqJa5IUz21H Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Feb 24, 2013 at 04:07:12PM +0100, Christoph Mallon wrote: > On 24.02.2013 07:05, Christoph Mallon wrote: > > I started reading the patch and found some minor glitches, e.g. mode in= cap_sandboxed() should be u_int, not int. > > I will report more later. >=20 > I placed several patches with suggested changes at http://tron.homeunix.o= rg/zeug/FreeBSD/capsicum/ (I also attached them). > Please have a look at them. > I also have some comments: > - A bitmask for cap_rights_limit() seems limited. > There are only six bits left unused. > Maybe a more general mechanism should be used. Yes, we are aware of this. bindat(2) and connectat(2) takes two more. Solution is not decided yet. > - I think that CAP_SEND, CAP_RECV, CAP_FCHMODAT, CAP_FCHOWNAT, CAP_FSTATA= T and CAP_FUTIMESAT are pointless aliases, which provide no benefit, but ra= ther might cause confusion, whether there is a difference between e.g. CAP_= WRITE and CAP_SEND. I am little worry that it might cause confusion, but not providing them would make it even more confusing, as it wouldn't be obvious which right should I have for send(2) to work. The aliases match system calls nicely. They are also documented as aliases in cap_rights_limit(2). > - Why did you choose INT_MAX to represent "all capabilities"? > The data type used is ssize_t, so SSIZE_MAX seems more logical. > Further, there are several places where size_t and ssize_t come into co= ntact, which need careful tests and casts to get right. > Maybe it is better to always use size_t and represent "all capabilities= " with SIZE_T_MAX or (size_t)-1 (which is guaranteed by C to be the maximal= value). This is not used for capabilities, but for white-listing ioctl commands. INT_MAX seems to just be least odd. We only allow for 256 anyway. I could provide some dedicated define for it, eg. #define CAP_IOCTLS_ALL <some random big number> > - Is it correct, that fdp seems to be not locked in fp_getfvp()? > Otherweise, fget_locked() could be used instead of the manual check. Yeah, I don't like this too, but AFAIR it doesn't have to be locked, as it is used by nfsd processes only that don't manipulate filedesc table. This is at least what I recall I was told. > - I could not verify many changes, e.g. changed requested permissions, be= cause this is just a big diff with no information about the intention of th= e changes. > A series of smaller diffs with commit logs to state their intent would = be really useful for reviewing. The entire history is in perforce, but there are many commits in there. The key goals of the patch are: - Move from special capability descriptors that were used to keep capability rights in the file structure to ability to configure capability rights on any descriptor type. - Make capability rights more practical. - Allow for ioctl(2) in capability mode by providing a way to limit permitted ioctl commands. - Allow for limiting permitted fcntl(2) commands. You can find comments to your patches below. Thank you very much for the changes. I updated the patch: http://people.freebsd.org/~pjd/patches/capkern.diff > From 7f18186c451c235bcc3dbf2e25f65613e1ce7f16 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:01:00 +0100 > Subject: [PATCH 02/24] Use the correct type for the parameter of > cap_getmode(). >=20 > --- > lib/libc/gen/cap_sandboxed.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/lib/libc/gen/cap_sandboxed.c b/lib/libc/gen/cap_sandboxed.c > index 95e5b23..5743f2a 100644 > --- a/lib/libc/gen/cap_sandboxed.c > +++ b/lib/libc/gen/cap_sandboxed.c > @@ -39,7 +39,7 @@ __FBSDID("$FreeBSD$"); > bool > cap_sandboxed(void) > { > - int mode; > + u_int mode; > =20 > if (cap_getmode(&mode) =3D=3D -1) { > assert(errno =3D=3D ENOSYS); Applied, although I prefer 'unsigned int' in userland. > From 7ad09a0cf0ceb9828171133e32f7148ffde62d97 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:03:03 +0100 > Subject: [PATCH 03/24] Use cheaper !=3D 0 tests. >=20 > --- > lib/libc/gen/cap_sandboxed.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) >=20 > diff --git a/lib/libc/gen/cap_sandboxed.c b/lib/libc/gen/cap_sandboxed.c > index 5743f2a..f09b590 100644 > --- a/lib/libc/gen/cap_sandboxed.c > +++ b/lib/libc/gen/cap_sandboxed.c > @@ -41,10 +41,10 @@ cap_sandboxed(void) > { > u_int mode; > =20 > - if (cap_getmode(&mode) =3D=3D -1) { > + if (cap_getmode(&mode) !=3D 0) { > assert(errno =3D=3D ENOSYS); > return (false); > } > assert(mode =3D=3D 0 || mode =3D=3D 1); > - return (mode =3D=3D 1); > + return (mode !=3D 0); > } I prefer comparison with -1, so I skipped this one. > From ae6ac8b82374e83dcc2bda7dd6e17da5d65d9eae Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:03:24 +0100 > Subject: [PATCH 04/24] Add cap_sandboxed to Symbol.map. >=20 > --- > lib/libc/sys/Symbol.map | 1 + > 1 file changed, 1 insertion(+) >=20 > diff --git a/lib/libc/sys/Symbol.map b/lib/libc/sys/Symbol.map > index 376ace0..7738e46 100644 > --- a/lib/libc/sys/Symbol.map > +++ b/lib/libc/sys/Symbol.map > @@ -384,6 +384,7 @@ FBSD_1.3 { > cap_ioctls_limit; > cap_rights_get; > cap_rights_limit; > + cap_sandboxed; > clock_getcpuclockid2; > ffclock_getcounter; > ffclock_getestimate; Applied. > From 4822faec8ea5c53c673740eeec0cb604960af37b Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:03:59 +0100 > Subject: [PATCH 05/24] Use standard inline instead of the GNUism __inline. >=20 > --- > sys/kern/sys_capability.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index 941ea49..0f4ef12 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -144,7 +144,7 @@ sys_cap_getmode(struct thread *td, struct cap_getmode= _args *uap) > =20 > FEATURE(security_capabilities, "Capsicum Capabilities"); > =20 > -static __inline int > +static inline int > _cap_check(cap_rights_t have, cap_rights_t need, enum ktr_cap_fail_type = type) > { > =20 Applied. > From 1d1defdf7dd117afa56330c958daeac5d52be05f Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:04:41 +0100 > Subject: [PATCH 06/24] Avoid polluting the global namespace with stdbool.= h. >=20 > --- > sys/sys/capability.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) >=20 > diff --git a/sys/sys/capability.h b/sys/sys/capability.h > index df95ae4..9eed4d1 100644 > --- a/sys/sys/capability.h > +++ b/sys/sys/capability.h > @@ -251,7 +251,6 @@ int cap_fcntl_check(struct filedesc *fdp, int fd, int= cmd); > #else /* !_KERNEL */ > =20 > __BEGIN_DECLS > -#include <stdbool.h> > =20 > /* > * cap_enter(): Cause the process to enter capability mode, which will > @@ -270,7 +269,7 @@ int cap_enter(void); > * Are we sandboxed (in capability mode)? > * This is libc wrapper around cap_getmode(2) system call. > */ > -bool cap_sandboxed(void); > +_Bool cap_sandboxed(void); > =20 > /* > * cap_getmode(): Are we in capability mode? Not sure yet about this one. > From 5107ab9aec6766c9ce5285939637c5739b2aed0c Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:05:19 +0100 > Subject: [PATCH 07/24] Use ${CC} instead of hardcoding gcc. >=20 > --- > tools/regression/capsicum/syscalls/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/tools/regression/capsicum/syscalls/Makefile b/tools/regressi= on/capsicum/syscalls/Makefile > index cb1b07b..5d34226 100644 > --- a/tools/regression/capsicum/syscalls/Makefile > +++ b/tools/regression/capsicum/syscalls/Makefile > @@ -14,7 +14,7 @@ all: ${SYSCALLS} ${SYSCALLS:=3D.t} > .for SYSCALL in ${SYSCALLS} > =20 > ${SYSCALL}: ${SYSCALL}.c misc.c > - gcc ${CFLAGS} ${@}.c misc.c -o $@ > + ${CC} ${CFLAGS} ${@}.c misc.c -o $@ > =20 > ${SYSCALL}.t: ${SYSCALL} > @printf "#!/bin/sh\n\n%s/%s\n" ${.CURDIR} ${@:.t=3D} > $@ Applied. > From d2fe7a6f9d8e08943600b1f5d6a2818c6c4aa794 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:31:37 +0100 > Subject: [PATCH 08/24] Use fget_locked() instead of duplicating it. >=20 > --- > sys/kern/uipc_usrreq.c | 3 +-- > sys/netsmb/smb_dev.c | 4 +--- > 2 files changed, 2 insertions(+), 5 deletions(-) >=20 > diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c > index 15a1c8a..529d2eb 100644 > --- a/sys/kern/uipc_usrreq.c > +++ b/sys/kern/uipc_usrreq.c > @@ -1858,8 +1858,7 @@ unp_internalize(struct mbuf **controlp, struct thre= ad *td) > FILEDESC_SLOCK(fdesc); > for (i =3D 0; i < oldfds; i++) { > fd =3D *fdp++; > - if (fd < 0 || fd >=3D fdesc->fd_nfiles || > - fdesc->fd_ofiles[fd].fde_file =3D=3D NULL) { > + if (fget_locked(fdp, fd) =3D=3D NULL) { > FILEDESC_SUNLOCK(fdesc); > error =3D EBADF; > goto out; > diff --git a/sys/netsmb/smb_dev.c b/sys/netsmb/smb_dev.c > index 0cee325..a09d74d 100644 > --- a/sys/netsmb/smb_dev.c > +++ b/sys/netsmb/smb_dev.c > @@ -399,9 +399,7 @@ nsmb_getfp(struct filedesc* fdp, int fd, int flag) > struct file* fp; > =20 > FILEDESC_SLOCK(fdp); > - if (fd < 0 || fd >=3D fdp->fd_nfiles || > - (fp =3D fdp->fd_ofiles[fd].fde_file) =3D=3D NULL || > - (fp->f_flag & flag) =3D=3D 0) { > + if ((fp =3D fget_locked(fdp, fd)) =3D=3D NULL || (fp->f_flag & flag) = =3D=3D 0) { > FILEDESC_SUNLOCK(fdp); > return (NULL); > } Applied. > From cc2de42e944828943c17fcba2a821becc634b9a4 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:33:49 +0100 > Subject: [PATCH 09/24] Remove duplicate checks. >=20 > fget_locked() just below does the same checks. > --- > sys/kern/kern_descrip.c | 5 ----- > 1 file changed, 5 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index bed5d8f..473ab40 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -2459,11 +2459,6 @@ fgetvp_rights(struct thread *td, int fd, cap_right= s_t need, > if (td =3D=3D NULL || (fdp =3D td->td_proc->p_fd) =3D=3D NULL) > return (EBADF); > =20 > - FILEDESC_LOCK_ASSERT(fdp); > - > - if (fd < 0 || fd >=3D fdp->fd_nfiles) > - return (EBADF); > - > fp =3D fget_locked(fdp, fd); > if (fp =3D=3D NULL || fp->f_ops =3D=3D &badfileops) > return (EBADF); Applied. > From 2b9add7396a9485add6a4246071cf041a50161d1 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:34:43 +0100 > Subject: [PATCH 10/24] Make fp_getfvp() static. >=20 > --- > sys/fs/nfs/nfsdport.h | 2 -- > sys/fs/nfsserver/nfs_nfsdport.c | 2 +- > 2 files changed, 1 insertion(+), 3 deletions(-) >=20 > diff --git a/sys/fs/nfs/nfsdport.h b/sys/fs/nfs/nfsdport.h > index 529ada2..a09a6dd 100644 > --- a/sys/fs/nfs/nfsdport.h > +++ b/sys/fs/nfs/nfsdport.h > @@ -94,8 +94,6 @@ struct nfsexstuff { > #define NFSFPCRED(f) ((f)->f_cred) > #define NFSFPFLAG(f) ((f)->f_flag) > =20 > -int fp_getfvp(NFSPROC_T *, int, struct file **, struct vnode **); > - > #define NFSNAMEICNDSET(n, c, o, f) do { \ > (n)->cn_cred =3D (c); \ > (n)->cn_nameiop =3D (o); \ > diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdp= ort.c > index cd7814c..880f965 100644 > --- a/sys/fs/nfsserver/nfs_nfsdport.c > +++ b/sys/fs/nfsserver/nfs_nfsdport.c > @@ -2767,7 +2767,7 @@ out: > /* > * glue for fp. > */ > -int > +static int > fp_getfvp(struct thread *p, int fd, struct file **fpp, struct vnode **vp= p) > { > struct filedesc *fdp; Applied. > From 4645cc6db54f29c1f5ec32c47794fd135f99cf60 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:37:22 +0100 > Subject: [PATCH 11/24] Use simple assignment instead of bcopy() to copy > structs. >=20 > --- > sys/kern/kern_descrip.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 473ab40..94bb74a 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -839,8 +839,7 @@ do_dup(struct thread *td, int flags, int old, int new, > /* > * Duplicate the source descriptor. > */ > - bcopy(&fdp->fd_ofiles[old], &fdp->fd_ofiles[new], > - sizeof(fdp->fd_ofiles[new])); > + fdp->fd_ofiles[new] =3D fdp->fd_ofiles[old]; > filecaps_copy(&fdp->fd_ofiles[old].fde_caps, > &fdp->fd_ofiles[new].fde_caps); > if ((flags & DUP_CLOEXEC) !=3D 0) { > @@ -1374,7 +1373,7 @@ filecaps_copy(const struct filecaps *src, struct fi= lecaps *dst) > { > size_t size; > =20 > - bcopy(src, dst, sizeof(*dst)); > + *dst =3D *src; > if (src->fc_ioctls !=3D NULL) { > KASSERT(src->fc_nioctls > 0, > ("fc_ioctls !=3D NULL, but fc_nioctls=3D%hd", src->fc_nioctls)); > @@ -1392,7 +1391,7 @@ static void > filecaps_move(struct filecaps *src, struct filecaps *dst) > { > =20 > - bcopy(src, dst, sizeof(*dst)); > + *dst =3D *src; > bzero(src, sizeof(*src)); > } > =20 > @@ -1835,7 +1834,7 @@ fdcopy(struct filedesc *fdp) > (ofde->fde_file->f_ops->fo_flags & DFLAG_PASSABLE) && > ofde->fde_file->f_ops !=3D &badfileops) { > nfde =3D &newfdp->fd_ofiles[i]; > - bcopy(ofde, nfde, sizeof(*nfde)); > + *nfde =3D *ofde; > filecaps_copy(&ofde->fde_caps, &nfde->fde_caps); > fhold(nfde->fde_file); > newfdp->fd_lastfile =3D i; > @@ -2681,8 +2680,7 @@ dupfdopen(struct thread *td, struct filedesc *fdp, = int dfd, int mode, > return (EACCES); > } > fhold(fp); > - bcopy(&fdp->fd_ofiles[dfd], &fdp->fd_ofiles[indx], > - sizeof(fdp->fd_ofiles[indx])); > + fdp->fd_ofiles[indx] =3D fdp->fd_ofiles[dfd]; > filecaps_copy(&fdp->fd_ofiles[dfd].fde_caps, > &fdp->fd_ofiles[indx].fde_caps); > break; > @@ -2690,8 +2688,7 @@ dupfdopen(struct thread *td, struct filedesc *fdp, = int dfd, int mode, > /* > * Steal away the file pointer from dfd and stuff it into indx. > */ > - bcopy(&fdp->fd_ofiles[dfd], &fdp->fd_ofiles[indx], > - sizeof(fdp->fd_ofiles[indx])); > + fdp->fd_ofiles[indx] =3D fdp->fd_ofiles[dfd]; > bzero(&fdp->fd_ofiles[dfd], sizeof(fdp->fd_ofiles[dfd])); > fdunused(fdp, dfd); > break; Applied. > From fc8878d0a43ea3f9d77b1fd51f90f4b4be188fb2 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:40:32 +0100 > Subject: [PATCH 12/24] Remove redundant null pointer tests before free(). >=20 > --- > sys/kern/kern_descrip.c | 3 +-- > sys/kern/sys_capability.c | 15 +++++---------- > 2 files changed, 6 insertions(+), 12 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 94bb74a..53b24b1 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -1415,8 +1415,7 @@ void > filecaps_free(struct filecaps *fcaps) > { > =20 > - if (fcaps->fc_ioctls !=3D NULL) > - free(fcaps->fc_ioctls, M_TEMP); > + free(fcaps->fc_ioctls, M_TEMP); > bzero(fcaps, sizeof(*fcaps)); > } > =20 > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index 0f4ef12..ecb3a6b 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -228,10 +228,8 @@ sys_cap_rights_limit(struct thread *td, struct cap_r= ights_limit_args *uap) > if (error =3D=3D 0) { > fdp->fd_ofiles[fd].fde_rights =3D rights; > if ((rights & CAP_IOCTL) =3D=3D 0) { > - if (fdp->fd_ofiles[fd].fde_ioctls !=3D NULL) { > - free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP); > - fdp->fd_ofiles[fd].fde_ioctls =3D NULL; > - } > + free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP); > + fdp->fd_ofiles[fd].fde_ioctls =3D NULL; > fdp->fd_ofiles[fd].fde_nioctls =3D 0; > } > if ((rights & CAP_FCNTL) =3D=3D 0 && > @@ -376,8 +374,7 @@ sys_cap_ioctls_limit(struct thread *td, struct cap_io= ctls_limit_args *uap) > =20 > FILEDESC_XUNLOCK(fdp); > =20 > - if (ocmds !=3D NULL) > - free(ocmds, M_TEMP); > + free(ocmds, M_TEMP); > =20 > return (0); > } > @@ -556,10 +553,8 @@ sys_cap_new(struct thread *td, struct cap_new_args *= uap) > */ > fdp->fd_ofiles[newfd].fde_rights =3D rights; > if ((rights & CAP_IOCTL) =3D=3D 0) { > - if (fdp->fd_ofiles[newfd].fde_ioctls !=3D NULL) { > - free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP); > - fdp->fd_ofiles[newfd].fde_ioctls =3D NULL; > - } > + free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP); > + fdp->fd_ofiles[newfd].fde_ioctls =3D NULL; > fdp->fd_ofiles[newfd].fde_nioctls =3D 0; > } > if ((rights & CAP_FCNTL) =3D=3D 0 && fdp->fd_ofiles[newfd].fde_fcntls != =3D 0) Applied. > From 41fc1f39a18331f57e6a03b54f4b78cbbd123cd5 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:41:38 +0100 > Subject: [PATCH 13/24] Remove redundant test. >=20 > if needrights is 0, the inner tests will succeed. > --- > sys/kern/kern_descrip.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 53b24b1..d1465a0 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -2270,15 +2270,13 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_r= ights_t needrights, > return (EBADF); > #ifdef CAPABILITIES > haverights =3D cap_rights(fdp, fd); > - if (needrights !=3D 0) { > - error =3D cap_check(haverights, needrights); > + error =3D cap_check(haverights, needrights); > + if (error !=3D 0) > + return (error); > + if ((needrights & CAP_FCNTL) !=3D 0) { > + error =3D cap_fcntl_check(fdp, fd, needfcntl); > if (error !=3D 0) > return (error); > - if ((needrights & CAP_FCNTL) !=3D 0) { > - error =3D cap_fcntl_check(fdp, fd, needfcntl); > - if (error !=3D 0) > - return (error); > - } > } > #endif > count =3D fp->f_count; Applied. > From d4c8e6bbeb678867b2bd1c0ec09faf60642465d0 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:43:28 +0100 > Subject: [PATCH 14/24] Avoide double test. >=20 > --- > sys/kern/sys_capability.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) >=20 > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index ecb3a6b..dbbda01 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -314,12 +314,12 @@ cap_ioctl_limit_check(struct filedesc *fdp, int fd,= const u_long *cmds, > =20 > ocmds =3D fdp->fd_ofiles[fd].fde_ioctls; > for (i =3D 0; i < ncmds; i++) { > - for (j =3D 0; j < oncmds; j++) { > + for (j =3D 0;; j++) { > + if (j =3D=3D oncmds) > + return (ENOTCAPABLE); > if (cmds[i] =3D=3D ocmds[j]) > break; > } > - if (j =3D=3D oncmds) > - return (ENOTCAPABLE); > } > =20 > return (0); I decided to skip this one. My version is more readable, IMHO, as it is used for other cases like TAILQ_FOREACH(), etc. where the check is already done by the macro. > From ba10274ab48bf9b2a2e6b8d50c59eb24074e6ec0 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:45:09 +0100 > Subject: [PATCH 15/24] Simplify if (x !=3D 0) x =3D 0; to just x =3D 0;. >=20 > --- > sys/kern/sys_capability.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) >=20 > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index dbbda01..153364b 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -232,10 +232,8 @@ sys_cap_rights_limit(struct thread *td, struct cap_r= ights_limit_args *uap) > fdp->fd_ofiles[fd].fde_ioctls =3D NULL; > fdp->fd_ofiles[fd].fde_nioctls =3D 0; > } > - if ((rights & CAP_FCNTL) =3D=3D 0 && > - fdp->fd_ofiles[fd].fde_fcntls !=3D 0) { > + if ((rights & CAP_FCNTL) =3D=3D 0) > fdp->fd_ofiles[fd].fde_fcntls =3D 0; > - } > } > FILEDESC_XUNLOCK(fdp); > return (error); > @@ -557,7 +555,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *u= ap) > fdp->fd_ofiles[newfd].fde_ioctls =3D NULL; > fdp->fd_ofiles[newfd].fde_nioctls =3D 0; > } > - if ((rights & CAP_FCNTL) =3D=3D 0 && fdp->fd_ofiles[newfd].fde_fcntls != =3D 0) > + if ((rights & CAP_FCNTL) =3D=3D 0) > fdp->fd_ofiles[newfd].fde_fcntls =3D 0; > FILEDESC_XUNLOCK(fdp); > =20 Applied. > From 38ec55338714e68b845ee45272fac7e7333727ef Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:47:23 +0100 > Subject: [PATCH 16/24] Reduce indentation. >=20 > --- > sys/kern/kern_descrip.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index d1465a0..612d51a 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -2463,15 +2463,13 @@ fgetvp_rights(struct thread *td, int fd, cap_righ= ts_t need, > if (error !=3D 0) > return (error); > =20 > - if (fp->f_vnode =3D=3D NULL) { > - error =3D EINVAL; > - } else { > - *vpp =3D fp->f_vnode; > - vref(*vpp); > - filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps); > - } > + if (fp->f_vnode =3D=3D NULL) > + return EINVAL; > =20 > - return (error); > + *vpp =3D fp->f_vnode; > + vref(*vpp); > + filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps); > + return (0); > } > =20 > int Applied. > From 64e66277c265b3a1221d9e269c8f33345e43332c Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 13:49:37 +0100 > Subject: [PATCH 17/24] Avoid code duplication on error paths. >=20 > --- > sys/kern/sys_capability.c | 42 ++++++++++++++++++------------------------ > 1 file changed, 18 insertions(+), 24 deletions(-) >=20 > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index 153364b..e87a4e5 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -344,37 +344,32 @@ sys_cap_ioctls_limit(struct thread *td, struct cap_= ioctls_limit_args *uap) > } else { > cmds =3D malloc(sizeof(cmds[0]) * ncmds, M_TEMP, M_WAITOK); > error =3D copyin(uap->cmds, cmds, sizeof(cmds[0]) * ncmds); > - if (error !=3D 0) { > - free(cmds, M_TEMP); > - return (error); > - } > + if (error !=3D 0) > + goto out; > } > =20 > fdp =3D td->td_proc->p_fd; > FILEDESC_XLOCK(fdp); > =20 > if (fget_locked(fdp, fd) =3D=3D NULL) { > - FILEDESC_XUNLOCK(fdp); > - free(cmds, M_TEMP); > - return (EBADF); > + error =3D EBADF; > + goto out_locked; > } > =20 > error =3D cap_ioctl_limit_check(fdp, fd, cmds, ncmds); > - if (error !=3D 0) { > - FILEDESC_XUNLOCK(fdp); > - free(cmds, M_TEMP); > - return (error); > - } > + if (error !=3D 0) > + goto out_locked; > =20 > ocmds =3D fdp->fd_ofiles[fd].fde_ioctls; > fdp->fd_ofiles[fd].fde_ioctls =3D cmds; > fdp->fd_ofiles[fd].fde_nioctls =3D ncmds; > + cmds =3D ocmds; > =20 > +out_locked: > FILEDESC_XUNLOCK(fdp); > - > - free(ocmds, M_TEMP); > - > - return (0); > +out: > + free(cmds, M_TEMP); > + return (error); > } > =20 > int > @@ -396,8 +391,8 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioct= ls_get_args *uap) > FILEDESC_SLOCK(fdp); > =20 > if (fget_locked(fdp, fd) =3D=3D NULL) { > - FILEDESC_SUNLOCK(fdp); > - return (EBADF); > + error =3D EBADF; > + goto out; > } > =20 > /* > @@ -410,19 +405,18 @@ sys_cap_ioctls_get(struct thread *td, struct cap_io= ctls_get_args *uap) > if (cmds !=3D NULL && fdep->fde_ioctls !=3D NULL) { > error =3D copyout(fdep->fde_ioctls, cmds, > sizeof(cmds[0]) * MIN(fdep->fde_nioctls, maxcmds)); > - if (error !=3D 0) { > - FILEDESC_SUNLOCK(fdp); > - return (error); > - } > + if (error !=3D 0) > + goto out; > } > if (fdep->fde_nioctls =3D=3D -1) > td->td_retval[0] =3D INT_MAX; > else > td->td_retval[0] =3D fdep->fde_nioctls; > + error =3D 0; > =20 > +out: > FILEDESC_SUNLOCK(fdp); > - > - return (0); > + return (error); > } > =20 > /* Applied, but I removed first goto and now out label is placed before unlock. > From 0e692e05accd56a4db1f938ad7aa4c50979a6148 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 14:00:01 +0100 > Subject: [PATCH 18/24] (Hopefully) slightly improve manpages and comments. >=20 > --- > lib/libc/gen/cap_sandboxed.3 | 6 +++--- > lib/libc/sys/cap_fcntls_limit.2 | 14 +++++++------- > lib/libc/sys/cap_ioctls_limit.2 | 24 ++++++++++++------------ > lib/libc/sys/cap_rights_limit.2 | 10 +++++----- > sys/sys/capability.h | 4 ++-- > 5 files changed, 29 insertions(+), 29 deletions(-) >=20 > diff --git a/lib/libc/gen/cap_sandboxed.3 b/lib/libc/gen/cap_sandboxed.3 > index 4896e02..067d6d2 100644 > --- a/lib/libc/gen/cap_sandboxed.3 > +++ b/lib/libc/gen/cap_sandboxed.3 > @@ -44,10 +44,10 @@ > .Fn cap_sandboxed > returns > .Va true > -is the process is in a capability mode sandbox or > +if the process is in a capability mode sandbox or > .Va false > if it is not. > -This function is more handy alternative to the > +This function is a more handy alternative to the > .Xr cap_getmode 2 > system call as it always succeeds, so there is no need for error checkin= g. > If the support for capability mode is not compiled into the kernel, > @@ -57,7 +57,7 @@ will always return > .Sh RETURN VALUES > Function > .Fn cap_sandboxed > -is always successful and can return either > +is always successful and will return either > .Va true > or > .Va false . > diff --git a/lib/libc/sys/cap_fcntls_limit.2 b/lib/libc/sys/cap_fcntls_li= mit.2 > index 38ab9cb..8fa7463 100644 > --- a/lib/libc/sys/cap_fcntls_limit.2 > +++ b/lib/libc/sys/cap_fcntls_limit.2 > @@ -44,15 +44,15 @@ > .Ft int > .Fn cap_fcntls_get "int fd" "uint32_t *fcntlrightsp" > .Sh DESCRIPTION > -If a file descriptor is granted > +If a file descriptor is granted the > .Dv CAP_FCNTL > -capability right, list of allowed > +capability right, the list of allowed > .Xr fcntl 2 > commands can be selectively reduced (but never expanded) with the > .Fn cap_fcntls_limit > system call. > .Pp > -Bitmask of allowed fcntls commands for a given file descriptor can be ob= tained > +A bitmask of allowed fcntls commands for a given file descriptor can be = obtained > with the > .Fn cap_fcntls_get > system call. > @@ -89,13 +89,13 @@ succeeds unless: > .It Bq Er EBADF > The > .Fa fd > -argument is not a valid active descriptor. > +argument is not a valid descriptor. > .It Bq Er EINVAL > An invalid flag has been passed in > .Fa fcntlrights . > .It Bq Er ENOTCAPABLE > .Fa fcntlrights > -would expand list of allowed > +would expand the list of allowed > .Xr fcntl 2 > commands. > .El > @@ -106,11 +106,11 @@ succeeds unless: > .It Bq Er EBADF > The > .Fa fd > -argument is not a valid active descriptor. > +argument is not a valid descriptor. > .It Bq Er EFAULT > The > .Fa fcntlrightsp > -argument points at invalid address. > +argument points at an invalid address. > .El > .Sh SEE ALSO > .Xr cap_ioctls_limit 2 , > diff --git a/lib/libc/sys/cap_ioctls_limit.2 b/lib/libc/sys/cap_ioctls_li= mit.2 > index 34a9bff..de524d7 100644 > --- a/lib/libc/sys/cap_ioctls_limit.2 > +++ b/lib/libc/sys/cap_ioctls_limit.2 > @@ -44,9 +44,9 @@ > .Ft ssize_t > .Fn cap_ioctls_get "int fd" "unsigned long *cmds" "size_t maxcmds" > .Sh DESCRIPTION > -If a file descriptor is granted > +If a file descriptor is granted the > .Dv CAP_IOCTL > -capability right, list of allowed > +capability right, the list of allowed > .Xr ioctl 2 > commands can be selectively reduced (but never expanded) with the > .Fn cap_ioctls_limit > @@ -62,21 +62,21 @@ There might be up to > .Va 256 > elements in the array. > .Pp > -List of allowed ioctl commands for a given file descriptor can be obtain= ed > +The list of allowed ioctl commands for a given file descriptor can be ob= tained > with the > .Fn cap_ioctls_get > system call. > The > .Fa cmds > -arguments points at the memory that can hold up to > +argument points at memory that can hold up to > .Fa maxcmds > values. > -The function populates provided buffer with up to > +The function populates the provided buffer with up to > .Fa maxcmds > -elements, but always returns total number of ioctl commands allowed for = the > +elements, but always returns the total number of ioctl commands allowed = for the > given file descriptor. > -Total number of ioctls commands for the given file descriptor can be obt= ained > -by passing > +The total number of ioctls commands for the given file descriptor can be > +obtained by passing > .Dv NULL as the > .Fa cmds > argument and > @@ -114,11 +114,11 @@ succeeds unless: > .It Bq Er EBADF > The > .Fa fd > -argument is not a valid active descriptor. > +argument is not a valid descriptor. > .It Bq Er EFAULT > The > .Fa cmds > -argument points at invalid address. > +argument points at an invalid address. > .It Bq Er EINVAL > The > .Fa ncmds > @@ -126,7 +126,7 @@ argument is greater than > .Va 256 . > .It Bq Er ENOTCAPABLE > .Fa cmds > -would expand list of allowed > +would expand the list of allowed > .Xr ioctl 2 > commands. > .El > @@ -137,7 +137,7 @@ succeeds unless: > .It Bq Er EBADF > The > .Fa fd > -argument is not a valid active descriptor. > +argument is not a valid descriptor. > .It Bq Er EFAULT > The > .Fa cmds > diff --git a/lib/libc/sys/cap_rights_limit.2 b/lib/libc/sys/cap_rights_li= mit.2 > index 2e83ea0..e2ff134 100644 > --- a/lib/libc/sys/cap_rights_limit.2 > +++ b/lib/libc/sys/cap_rights_limit.2 > @@ -68,7 +68,7 @@ Once capability rights are reduced, operations on the f= ile descriptor will be > limited to those permitted by > .Fa rights . > .Pp > -Bitmask of capability rights assigned to a file descriptor can be obtain= ed with > +A bitmask of capability rights assigned to a file descriptor can be obta= ined with > the > .Fn cap_rights_get > system call. > @@ -178,7 +178,7 @@ Note that only the > and > .Dv F_SETOWN > commands require this capability right. > -Also note that the list of permitted commands can be futher limited with= the > +Also note that the list of permitted commands can be further limited wit= h the > .Xr cap_fcntls_limit 2 > system call. > .It Dv CAP_FLOCK > @@ -254,7 +254,7 @@ Permit > .Xr ioctl 2 . > Be aware that this system call has enormous scope, including potentially > global scope for some objects. > -The list of permitted ioctl commands can be futher limited with the > +The list of permitted ioctl commands can be further limited with the > .Xr cap_ioctls_limit 2 > system call. > .\" XXXPJD: Doesn't exist anymore. > @@ -470,7 +470,7 @@ with the > .Dv O_WRONLY > flag, but without the > .Dv O_APPEND > -flag > +flag, > .Dv CAP_SEEK > is also required. > .El > @@ -503,7 +503,7 @@ argument is not a valid active descriptor. > .It Bq Er EFAULT > The > .Fa rightsp > -argument points at invalid address. > +argument points at an invalid address. > .El > .Sh SEE ALSO > .Xr accept 2 , > diff --git a/sys/sys/capability.h b/sys/sys/capability.h > index 9eed4d1..130d200 100644 > --- a/sys/sys/capability.h > +++ b/sys/sys/capability.h > @@ -267,7 +267,7 @@ int cap_enter(void); > =20 > /* > * Are we sandboxed (in capability mode)? > - * This is libc wrapper around cap_getmode(2) system call. > + * This is a libc wrapper around the cap_getmode(2) system call. > */ > _Bool cap_sandboxed(void); > =20 > @@ -289,7 +289,7 @@ int cap_rights_get(int fd, cap_rights_t *rightsp); > */ > int cap_ioctls_limit(int fd, const unsigned long *cmds, size_t ncmds); > /* > - * Returns array of allowed ioctls for the given descriptors. > + * Returns array of allowed ioctls for the given descriptor. > * If all ioctls are allowed, the cmds array is not populated and > * the function returns INT_MAX. > */ Applied. > From 2b8a154312f692912fc90bc2acd8e55ab1fc4ba4 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 14:00:31 +0100 > Subject: [PATCH 19/24] Sort Xr. >=20 > --- > lib/libc/sys/cap_rights_limit.2 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/lib/libc/sys/cap_rights_limit.2 b/lib/libc/sys/cap_rights_li= mit.2 > index e2ff134..d8d8777 100644 > --- a/lib/libc/sys/cap_rights_limit.2 > +++ b/lib/libc/sys/cap_rights_limit.2 > @@ -546,8 +546,8 @@ argument points at an invalid address. > .Xr mq_open 2 , > .Xr open 2 , > .Xr openat 2 , > -.Xr pdgetpid 2 , > .Xr pdfork 2 , > +.Xr pdgetpid 2 , > .Xr pdkill 2 , > .Xr pdwait4 2 , > .Xr pipe 2 , Applied. > From 42aaaa14c5c1a5d96503a88a381909d6c3254727 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 14:04:36 +0100 > Subject: [PATCH 20/24] Avoid comparing signed with unsigned values. >=20 > --- > sys/fs/nfsserver/nfs_nfsdport.c | 2 +- > sys/kern/sys_capability.c | 7 ++++--- > 2 files changed, 5 insertions(+), 4 deletions(-) >=20 > diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdp= ort.c > index 880f965..69f5629 100644 > --- a/sys/fs/nfsserver/nfs_nfsdport.c > +++ b/sys/fs/nfsserver/nfs_nfsdport.c > @@ -2775,7 +2775,7 @@ fp_getfvp(struct thread *p, int fd, struct file **f= pp, struct vnode **vpp) > int error =3D 0; > =20 > fdp =3D p->td_proc->p_fd; > - if ((u_int)fd >=3D fdp->fd_nfiles || > + if (fd < 0 || fdp->fd_nfiles <=3D fd || > (fp =3D fdp->fd_ofiles[fd].fde_file) =3D=3D NULL) { > error =3D EBADF; > goto out; > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index e87a4e5..e3622b0 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -274,7 +274,7 @@ cap_ioctl_check(struct filedesc *fdp, int fd, u_long = cmd) > { > u_long *cmds; > ssize_t ncmds; > - u_int i; > + ssize_t i; > =20 > FILEDESC_LOCK_ASSERT(fdp); > KASSERT(fd >=3D 0 && fd < fdp->fd_nfiles, > @@ -302,12 +302,13 @@ cap_ioctl_limit_check(struct filedesc *fdp, int fd,= const u_long *cmds, > { > u_long *ocmds; > ssize_t oncmds; > - u_int i, j; > + size_t i; > + ssize_t j; > =20 > oncmds =3D fdp->fd_ofiles[fd].fde_nioctls; > if (oncmds =3D=3D -1) > return (0); > - if (oncmds < ncmds) > + if ((size_t)oncmds < ncmds) > return (ENOTCAPABLE); > =20 > ocmds =3D fdp->fd_ofiles[fd].fde_ioctls; Applied with some minor tweaks. > From a05144e19411d2b3fc8716e72ef2fad7cc9449ae Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 15:09:16 +0100 > Subject: [PATCH 21/24] For readability simplify if (x) y =3D a; else y = =3D b; with > long y to y =3D x ? a : b. >=20 > --- > sys/kern/kern_descrip.c | 10 +++------- > sys/kern/sys_capability.c | 6 ++---- > 2 files changed, 5 insertions(+), 11 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 612d51a..30dac04 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -842,13 +842,9 @@ do_dup(struct thread *td, int flags, int old, int ne= w, > fdp->fd_ofiles[new] =3D fdp->fd_ofiles[old]; > filecaps_copy(&fdp->fd_ofiles[old].fde_caps, > &fdp->fd_ofiles[new].fde_caps); > - if ((flags & DUP_CLOEXEC) !=3D 0) { > - fdp->fd_ofiles[new].fde_flags =3D > - fdp->fd_ofiles[old].fde_flags | UF_EXCLOSE; > - } else { > - fdp->fd_ofiles[new].fde_flags =3D > - fdp->fd_ofiles[old].fde_flags & ~UF_EXCLOSE; > - } > + fdp->fd_ofiles[new].fde_flags =3D flags & DUP_CLOEXEC ? > + fdp->fd_ofiles[old].fde_flags | UF_EXCLOSE : > + fdp->fd_ofiles[old].fde_flags & ~UF_EXCLOSE; > if (new > fdp->fd_lastfile) > fdp->fd_lastfile =3D new; > *retval =3D new; > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index e3622b0..2306811 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -409,10 +409,8 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioc= tls_get_args *uap) > if (error !=3D 0) > goto out; > } > - if (fdep->fde_nioctls =3D=3D -1) > - td->td_retval[0] =3D INT_MAX; > - else > - td->td_retval[0] =3D fdep->fde_nioctls; > + td->td_retval[0] =3D > + fdep->fde_nioctls !=3D -1 ? fdep->fde_nioctls : INT_MAX; > error =3D 0; > =20 > out: Well, IMHO my version is more readable, especially in the first case. > From 1258951ef3cdb8b8624e6a7032036e0e5e0ac8c6 Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 15:23:15 +0100 > Subject: [PATCH 22/24] Unify and simplify bitset inclusion tests. >=20 > - (have | need) !=3D have looks like a typo: shouldn't the | be a &?. Which line exactly? > - some places used (have | need) !=3D have, others (have & need) =3D=3D n= eed. > - (need & ~have) !=3D 0 -- need and not have -- is easier to comprehend. > - Avoid duplication, especially when the duplicated subexpression is long. > --- > sys/kern/kern_descrip.c | 4 ++-- > sys/kern/sys_capability.c | 11 +++++------ > usr.bin/procstat/procstat_files.c | 4 ++-- > 3 files changed, 9 insertions(+), 10 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 30dac04..64176ca 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -1422,9 +1422,9 @@ static void > filecaps_validate(const struct filecaps *fcaps, const char *func) > { > =20 > - KASSERT((fcaps->fc_rights | CAP_MASK_VALID) =3D=3D CAP_MASK_VALID, > + KASSERT((fcaps->fc_rights & ~CAP_MASK_VALID) =3D=3D 0, > ("%s: invalid rights", func)); > - KASSERT((fcaps->fc_fcntls | CAP_FCNTL_ALL) =3D=3D CAP_FCNTL_ALL, > + KASSERT((fcaps->fc_fcntls & ~CAP_FCNTL_ALL) =3D=3D 0, > ("%s: invalid fcntls", func)); > KASSERT(fcaps->fc_fcntls =3D=3D 0 || (fcaps->fc_rights & CAP_FCNTL) != =3D 0, > ("%s: fcntls without CAP_FCNTL", func)); > diff --git a/sys/kern/sys_capability.c b/sys/kern/sys_capability.c > index 2306811..d06918c 100644 > --- a/sys/kern/sys_capability.c > +++ b/sys/kern/sys_capability.c > @@ -148,7 +148,7 @@ static inline int > _cap_check(cap_rights_t have, cap_rights_t need, enum ktr_cap_fail_type = type) > { > =20 > - if ((have | need) !=3D have) { > + if ((need & ~have) !=3D 0) { > #ifdef KTRACE > if (KTRPOINT(curthread, KTR_CAPFAIL)) > ktrcapfail(type, need, have); > @@ -215,7 +215,7 @@ sys_cap_rights_limit(struct thread *td, struct cap_ri= ghts_limit_args *uap) > AUDIT_ARG_FD(fd); > AUDIT_ARG_RIGHTS(rights); > =20 > - if ((rights | CAP_ALL) !=3D CAP_ALL) > + if ((rights & ~CAP_ALL) !=3D 0) > return (EINVAL); > =20 > fdp =3D td->td_proc->p_fd; > @@ -452,7 +452,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fc= ntls_limit_args *uap) > AUDIT_ARG_FD(fd); > AUDIT_ARG_FCNTL_RIGHTS(fcntlrights); > =20 > - if ((fcntlrights | CAP_FCNTL_ALL) !=3D CAP_FCNTL_ALL) > + if ((fcntlrights & ~CAP_FCNTL_ALL) !=3D 0) > return (EINVAL); > =20 > fdp =3D td->td_proc->p_fd; > @@ -463,8 +463,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fc= ntls_limit_args *uap) > return (EBADF); > } > =20 > - if ((fdp->fd_ofiles[fd].fde_fcntls | fcntlrights) !=3D > - fdp->fd_ofiles[fd].fde_fcntls) { > + if ((fcntlrights & ~fdp->fd_ofiles[fd].fde_fcntls) !=3D 0) { > FILEDESC_XUNLOCK(fdp); > return (ENOTCAPABLE); > } > @@ -515,7 +514,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *u= ap) > AUDIT_ARG_FD(fd); > AUDIT_ARG_RIGHTS(rights); > =20 > - if ((rights | CAP_ALL) !=3D CAP_ALL) > + if ((rights & ~CAP_ALL) !=3D 0) > return (EINVAL); > =20 > fdp =3D td->td_proc->p_fd; > diff --git a/usr.bin/procstat/procstat_files.c b/usr.bin/procstat/procsta= t_files.c > index eefc5b7..16dab0b 100644 > --- a/usr.bin/procstat/procstat_files.c > +++ b/usr.bin/procstat/procstat_files.c > @@ -243,7 +243,7 @@ width_capability(cap_rights_t rights) > count =3D 0; > width =3D 0; > for (i =3D 0; i < cap_desc_count; i++) { > - if ((rights & cap_desc[i].cd_right) =3D=3D cap_desc[i].cd_right) { > + if ((cap_desc[i].cd_right & ~rights) =3D=3D 0) { > width +=3D strlen(cap_desc[i].cd_desc); > if (count) > width++; > @@ -267,7 +267,7 @@ print_capability(cap_rights_t rights, u_int capwidth) > printf("-"); > } > for (i =3D 0; i < cap_desc_count; i++) { > - if ((rights & cap_desc[i].cd_right) =3D=3D cap_desc[i].cd_right) { > + if ((cap_desc[i].cd_right & ~rights) =3D=3D 0) { > printf("%s%s", count ? "," : "", cap_desc[i].cd_desc); > width +=3D strlen(cap_desc[i].cd_desc); > if (count) Not applied for now. > From 4d721b7aa909091fc705cc8f822a13da69e1954c Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 15:33:48 +0100 > Subject: [PATCH 23/24] Simplify assertion condition, which contains a > duplicated subexpression. >=20 > --- > sys/kern/kern_descrip.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index 64176ca..c2556e2 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -1428,9 +1428,8 @@ filecaps_validate(const struct filecaps *fcaps, con= st char *func) > ("%s: invalid fcntls", func)); > KASSERT(fcaps->fc_fcntls =3D=3D 0 || (fcaps->fc_rights & CAP_FCNTL) != =3D 0, > ("%s: fcntls without CAP_FCNTL", func)); > - KASSERT(((fcaps->fc_nioctls =3D=3D -1 || fcaps->fc_nioctls =3D=3D 0) && > - fcaps->fc_ioctls =3D=3D NULL) || > - (fcaps->fc_nioctls > 0 && fcaps->fc_ioctls !=3D NULL), > + KASSERT(fcaps->fc_ioctls !=3D NULL ? fcaps->fc_nioctls > 0 : > + (fcaps->fc_nioctls =3D=3D -1 || fcaps->fc_nioctls =3D=3D 0), > ("%s: invalid ioctls", func)); > KASSERT(fcaps->fc_nioctls =3D=3D 0 || (fcaps->fc_rights & CAP_IOCTL) != =3D 0, > ("%s: ioctls without CAP_IOCTL", func)); I think my version is more readable. Skipped. > From 98dd3c8a988e043b4a8f02ed4c19d39d30e1145d Mon Sep 17 00:00:00 2001 > From: Christoph Mallon <christoph.mallon@gmx.de> > Date: Sun, 24 Feb 2013 15:47:53 +0100 > Subject: [PATCH 24/24] Factorise code to free a file descriptor. >=20 > --- > sys/kern/kern_descrip.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) >=20 > diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c > index c2556e2..8256501 100644 > --- a/sys/kern/kern_descrip.c > +++ b/sys/kern/kern_descrip.c > @@ -287,6 +287,19 @@ fdunused(struct filedesc *fdp, int fd) > } > =20 > /* > + * Free a file descriptor. > + */ > +static void > +fdfree(struct filedesc *fdp, int fd) > +{ > + struct filedescent *fde =3D fdp->fd_ofiles[fd]; > + > + filecaps_free(&fde->fde_caps); > + bzero(fde, sizeof(*fde)); > + fdunused(fdp, fd); > +} > + > +/* > * System calls on descriptors. > */ > #ifndef _SYS_SYSPROTO_H_ > @@ -1165,9 +1178,7 @@ kern_close(td, fd) > FILEDESC_XUNLOCK(fdp); > return (EBADF); > } > - filecaps_free(&fdp->fd_ofiles[fd].fde_caps); > - bzero(&fdp->fd_ofiles[fd], sizeof(fdp->fd_ofiles[fd])); > - fdunused(fdp, fd); > + fdfree(fdp, fd); > =20 > /* closefp() drops the FILEDESC lock for us. */ > return (closefp(fdp, fd, fp, td, 1)); > @@ -2035,9 +2046,7 @@ setugidsafety(struct thread *td) > * NULL-out descriptor prior to close to avoid > * a race while close blocks. > */ > - filecaps_free(&fdp->fd_ofiles[i].fde_caps); > - bzero(&fdp->fd_ofiles[i], sizeof(fdp->fd_ofiles[i])); > - fdunused(fdp, i); > + fdfree(fdp, i); > FILEDESC_XUNLOCK(fdp); > (void) closef(fp, td); > FILEDESC_XLOCK(fdp); > @@ -2059,9 +2068,7 @@ fdclose(struct filedesc *fdp, struct file *fp, int = idx, struct thread *td) > =20 > FILEDESC_XLOCK(fdp); > if (fdp->fd_ofiles[idx].fde_file =3D=3D fp) { > - filecaps_free(&fdp->fd_ofiles[idx].fde_caps); > - bzero(&fdp->fd_ofiles[idx], sizeof(fdp->fd_ofiles[idx])); > - fdunused(fdp, idx); > + fdfree(fdp, idx); > FILEDESC_XUNLOCK(fdp); > fdrop(fp, td); > } else > @@ -2094,9 +2101,7 @@ fdcloseexec(struct thread *td) > fp =3D fde->fde_file; > if (fp !=3D NULL && (fp->f_type =3D=3D DTYPE_MQUEUE || > (fde->fde_flags & UF_EXCLOSE))) { > - filecaps_free(&fde->fde_caps); > - bzero(fde, sizeof(*fde)); > - fdunused(fdp, i); > + fdfree(fdp, i); > (void) closefp(fdp, i, fp, td, 0); > /* closefp() drops the FILEDESC lock. */ > FILEDESC_XLOCK(fdp); Applied. Thanks a lot! --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl --yTwVabqJa5IUz21H Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iEYEARECAAYFAlEqqWgACgkQForvXbEpPzToEACg5ZpV0c0ByzYoZ8V3zunuJ6Rt OXkAn0N0kUGsqrNrwhhQV0HL4cPQgfH3 =bm9F -----END PGP SIGNATURE----- --yTwVabqJa5IUz21H--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130224235936.GX1377>