Large Capsicum patch for review.
Pawel Jakub Dawidek
pjd at FreeBSD.org
Sun Feb 24 23:58:33 UTC 2013
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.
>
> I placed several patches with suggested changes at http://tron.homeunix.org/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_FSTATAT and CAP_FUTIMESAT are pointless aliases, which provide no benefit, but rather 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 contact, 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, because this is just a big diff with no information about the intention of the 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 at 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().
>
> ---
> lib/libc/gen/cap_sandboxed.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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;
>
> if (cap_getmode(&mode) == -1) {
> assert(errno == ENOSYS);
Applied, although I prefer 'unsigned int' in userland.
> From 7ad09a0cf0ceb9828171133e32f7148ffde62d97 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:03:03 +0100
> Subject: [PATCH 03/24] Use cheaper != 0 tests.
>
> ---
> lib/libc/gen/cap_sandboxed.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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;
>
> - if (cap_getmode(&mode) == -1) {
> + if (cap_getmode(&mode) != 0) {
> assert(errno == ENOSYS);
> return (false);
> }
> assert(mode == 0 || mode == 1);
> - return (mode == 1);
> + return (mode != 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 at gmx.de>
> Date: Sun, 24 Feb 2013 13:03:24 +0100
> Subject: [PATCH 04/24] Add cap_sandboxed to Symbol.map.
>
> ---
> lib/libc/sys/Symbol.map | 1 +
> 1 file changed, 1 insertion(+)
>
> 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 at gmx.de>
> Date: Sun, 24 Feb 2013 13:03:59 +0100
> Subject: [PATCH 05/24] Use standard inline instead of the GNUism __inline.
>
> ---
> sys/kern/sys_capability.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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)
>
> FEATURE(security_capabilities, "Capsicum Capabilities");
>
> -static __inline int
> +static inline int
> _cap_check(cap_rights_t have, cap_rights_t need, enum ktr_cap_fail_type type)
> {
>
Applied.
> From 1d1defdf7dd117afa56330c958daeac5d52be05f Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:04:41 +0100
> Subject: [PATCH 06/24] Avoid polluting the global namespace with stdbool.h.
>
> ---
> sys/sys/capability.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> 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 */
>
> __BEGIN_DECLS
> -#include <stdbool.h>
>
> /*
> * 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);
>
> /*
> * 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 at gmx.de>
> Date: Sun, 24 Feb 2013 13:05:19 +0100
> Subject: [PATCH 07/24] Use ${CC} instead of hardcoding gcc.
>
> ---
> tools/regression/capsicum/syscalls/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/regression/capsicum/syscalls/Makefile b/tools/regression/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:=.t}
> .for SYSCALL in ${SYSCALLS}
>
> ${SYSCALL}: ${SYSCALL}.c misc.c
> - gcc ${CFLAGS} ${@}.c misc.c -o $@
> + ${CC} ${CFLAGS} ${@}.c misc.c -o $@
>
> ${SYSCALL}.t: ${SYSCALL}
> @printf "#!/bin/sh\n\n%s/%s\n" ${.CURDIR} ${@:.t=} > $@
Applied.
> From d2fe7a6f9d8e08943600b1f5d6a2818c6c4aa794 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:31:37 +0100
> Subject: [PATCH 08/24] Use fget_locked() instead of duplicating it.
>
> ---
> sys/kern/uipc_usrreq.c | 3 +--
> sys/netsmb/smb_dev.c | 4 +---
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> 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 thread *td)
> FILEDESC_SLOCK(fdesc);
> for (i = 0; i < oldfds; i++) {
> fd = *fdp++;
> - if (fd < 0 || fd >= fdesc->fd_nfiles ||
> - fdesc->fd_ofiles[fd].fde_file == NULL) {
> + if (fget_locked(fdp, fd) == NULL) {
> FILEDESC_SUNLOCK(fdesc);
> error = 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;
>
> FILEDESC_SLOCK(fdp);
> - if (fd < 0 || fd >= fdp->fd_nfiles ||
> - (fp = fdp->fd_ofiles[fd].fde_file) == NULL ||
> - (fp->f_flag & flag) == 0) {
> + if ((fp = fget_locked(fdp, fd)) == NULL || (fp->f_flag & flag) == 0) {
> FILEDESC_SUNLOCK(fdp);
> return (NULL);
> }
Applied.
> From cc2de42e944828943c17fcba2a821becc634b9a4 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:33:49 +0100
> Subject: [PATCH 09/24] Remove duplicate checks.
>
> fget_locked() just below does the same checks.
> ---
> sys/kern/kern_descrip.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> 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_rights_t need,
> if (td == NULL || (fdp = td->td_proc->p_fd) == NULL)
> return (EBADF);
>
> - FILEDESC_LOCK_ASSERT(fdp);
> -
> - if (fd < 0 || fd >= fdp->fd_nfiles)
> - return (EBADF);
> -
> fp = fget_locked(fdp, fd);
> if (fp == NULL || fp->f_ops == &badfileops)
> return (EBADF);
Applied.
> From 2b9add7396a9485add6a4246071cf041a50161d1 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:34:43 +0100
> Subject: [PATCH 10/24] Make fp_getfvp() static.
>
> ---
> sys/fs/nfs/nfsdport.h | 2 --
> sys/fs/nfsserver/nfs_nfsdport.c | 2 +-
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> 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)
>
> -int fp_getfvp(NFSPROC_T *, int, struct file **, struct vnode **);
> -
> #define NFSNAMEICNDSET(n, c, o, f) do { \
> (n)->cn_cred = (c); \
> (n)->cn_nameiop = (o); \
> diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.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 **vpp)
> {
> struct filedesc *fdp;
Applied.
> From 4645cc6db54f29c1f5ec32c47794fd135f99cf60 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:37:22 +0100
> Subject: [PATCH 11/24] Use simple assignment instead of bcopy() to copy
> structs.
>
> ---
> sys/kern/kern_descrip.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> 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] = fdp->fd_ofiles[old];
> filecaps_copy(&fdp->fd_ofiles[old].fde_caps,
> &fdp->fd_ofiles[new].fde_caps);
> if ((flags & DUP_CLOEXEC) != 0) {
> @@ -1374,7 +1373,7 @@ filecaps_copy(const struct filecaps *src, struct filecaps *dst)
> {
> size_t size;
>
> - bcopy(src, dst, sizeof(*dst));
> + *dst = *src;
> if (src->fc_ioctls != NULL) {
> KASSERT(src->fc_nioctls > 0,
> ("fc_ioctls != NULL, but fc_nioctls=%hd", src->fc_nioctls));
> @@ -1392,7 +1391,7 @@ static void
> filecaps_move(struct filecaps *src, struct filecaps *dst)
> {
>
> - bcopy(src, dst, sizeof(*dst));
> + *dst = *src;
> bzero(src, sizeof(*src));
> }
>
> @@ -1835,7 +1834,7 @@ fdcopy(struct filedesc *fdp)
> (ofde->fde_file->f_ops->fo_flags & DFLAG_PASSABLE) &&
> ofde->fde_file->f_ops != &badfileops) {
> nfde = &newfdp->fd_ofiles[i];
> - bcopy(ofde, nfde, sizeof(*nfde));
> + *nfde = *ofde;
> filecaps_copy(&ofde->fde_caps, &nfde->fde_caps);
> fhold(nfde->fde_file);
> newfdp->fd_lastfile = 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] = 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] = 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 at gmx.de>
> Date: Sun, 24 Feb 2013 13:40:32 +0100
> Subject: [PATCH 12/24] Remove redundant null pointer tests before free().
>
> ---
> sys/kern/kern_descrip.c | 3 +--
> sys/kern/sys_capability.c | 15 +++++----------
> 2 files changed, 6 insertions(+), 12 deletions(-)
>
> 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)
> {
>
> - if (fcaps->fc_ioctls != NULL)
> - free(fcaps->fc_ioctls, M_TEMP);
> + free(fcaps->fc_ioctls, M_TEMP);
> bzero(fcaps, sizeof(*fcaps));
> }
>
> 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_rights_limit_args *uap)
> if (error == 0) {
> fdp->fd_ofiles[fd].fde_rights = rights;
> if ((rights & CAP_IOCTL) == 0) {
> - if (fdp->fd_ofiles[fd].fde_ioctls != NULL) {
> - free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP);
> - fdp->fd_ofiles[fd].fde_ioctls = NULL;
> - }
> + free(fdp->fd_ofiles[fd].fde_ioctls, M_TEMP);
> + fdp->fd_ofiles[fd].fde_ioctls = NULL;
> fdp->fd_ofiles[fd].fde_nioctls = 0;
> }
> if ((rights & CAP_FCNTL) == 0 &&
> @@ -376,8 +374,7 @@ sys_cap_ioctls_limit(struct thread *td, struct cap_ioctls_limit_args *uap)
>
> FILEDESC_XUNLOCK(fdp);
>
> - if (ocmds != NULL)
> - free(ocmds, M_TEMP);
> + free(ocmds, M_TEMP);
>
> return (0);
> }
> @@ -556,10 +553,8 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
> */
> fdp->fd_ofiles[newfd].fde_rights = rights;
> if ((rights & CAP_IOCTL) == 0) {
> - if (fdp->fd_ofiles[newfd].fde_ioctls != NULL) {
> - free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP);
> - fdp->fd_ofiles[newfd].fde_ioctls = NULL;
> - }
> + free(fdp->fd_ofiles[newfd].fde_ioctls, M_TEMP);
> + fdp->fd_ofiles[newfd].fde_ioctls = NULL;
> fdp->fd_ofiles[newfd].fde_nioctls = 0;
> }
> if ((rights & CAP_FCNTL) == 0 && fdp->fd_ofiles[newfd].fde_fcntls != 0)
Applied.
> From 41fc1f39a18331f57e6a03b54f4b78cbbd123cd5 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:41:38 +0100
> Subject: [PATCH 13/24] Remove redundant test.
>
> if needrights is 0, the inner tests will succeed.
> ---
> sys/kern/kern_descrip.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> 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_rights_t needrights,
> return (EBADF);
> #ifdef CAPABILITIES
> haverights = cap_rights(fdp, fd);
> - if (needrights != 0) {
> - error = cap_check(haverights, needrights);
> + error = cap_check(haverights, needrights);
> + if (error != 0)
> + return (error);
> + if ((needrights & CAP_FCNTL) != 0) {
> + error = cap_fcntl_check(fdp, fd, needfcntl);
> if (error != 0)
> return (error);
> - if ((needrights & CAP_FCNTL) != 0) {
> - error = cap_fcntl_check(fdp, fd, needfcntl);
> - if (error != 0)
> - return (error);
> - }
> }
> #endif
> count = fp->f_count;
Applied.
> From d4c8e6bbeb678867b2bd1c0ec09faf60642465d0 Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:43:28 +0100
> Subject: [PATCH 14/24] Avoide double test.
>
> ---
> sys/kern/sys_capability.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> 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,
>
> ocmds = fdp->fd_ofiles[fd].fde_ioctls;
> for (i = 0; i < ncmds; i++) {
> - for (j = 0; j < oncmds; j++) {
> + for (j = 0;; j++) {
> + if (j == oncmds)
> + return (ENOTCAPABLE);
> if (cmds[i] == ocmds[j])
> break;
> }
> - if (j == oncmds)
> - return (ENOTCAPABLE);
> }
>
> 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 at gmx.de>
> Date: Sun, 24 Feb 2013 13:45:09 +0100
> Subject: [PATCH 15/24] Simplify if (x != 0) x = 0; to just x = 0;.
>
> ---
> sys/kern/sys_capability.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> 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_rights_limit_args *uap)
> fdp->fd_ofiles[fd].fde_ioctls = NULL;
> fdp->fd_ofiles[fd].fde_nioctls = 0;
> }
> - if ((rights & CAP_FCNTL) == 0 &&
> - fdp->fd_ofiles[fd].fde_fcntls != 0) {
> + if ((rights & CAP_FCNTL) == 0)
> fdp->fd_ofiles[fd].fde_fcntls = 0;
> - }
> }
> FILEDESC_XUNLOCK(fdp);
> return (error);
> @@ -557,7 +555,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
> fdp->fd_ofiles[newfd].fde_ioctls = NULL;
> fdp->fd_ofiles[newfd].fde_nioctls = 0;
> }
> - if ((rights & CAP_FCNTL) == 0 && fdp->fd_ofiles[newfd].fde_fcntls != 0)
> + if ((rights & CAP_FCNTL) == 0)
> fdp->fd_ofiles[newfd].fde_fcntls = 0;
> FILEDESC_XUNLOCK(fdp);
>
Applied.
> From 38ec55338714e68b845ee45272fac7e7333727ef Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:47:23 +0100
> Subject: [PATCH 16/24] Reduce indentation.
>
> ---
> sys/kern/kern_descrip.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> 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_rights_t need,
> if (error != 0)
> return (error);
>
> - if (fp->f_vnode == NULL) {
> - error = EINVAL;
> - } else {
> - *vpp = fp->f_vnode;
> - vref(*vpp);
> - filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps);
> - }
> + if (fp->f_vnode == NULL)
> + return EINVAL;
>
> - return (error);
> + *vpp = fp->f_vnode;
> + vref(*vpp);
> + filecaps_copy(&fdp->fd_ofiles[fd].fde_caps, havecaps);
> + return (0);
> }
>
> int
Applied.
> From 64e66277c265b3a1221d9e269c8f33345e43332c Mon Sep 17 00:00:00 2001
> From: Christoph Mallon <christoph.mallon at gmx.de>
> Date: Sun, 24 Feb 2013 13:49:37 +0100
> Subject: [PATCH 17/24] Avoid code duplication on error paths.
>
> ---
> sys/kern/sys_capability.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> 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 = malloc(sizeof(cmds[0]) * ncmds, M_TEMP, M_WAITOK);
> error = copyin(uap->cmds, cmds, sizeof(cmds[0]) * ncmds);
> - if (error != 0) {
> - free(cmds, M_TEMP);
> - return (error);
> - }
> + if (error != 0)
> + goto out;
> }
>
> fdp = td->td_proc->p_fd;
> FILEDESC_XLOCK(fdp);
>
> if (fget_locked(fdp, fd) == NULL) {
> - FILEDESC_XUNLOCK(fdp);
> - free(cmds, M_TEMP);
> - return (EBADF);
> + error = EBADF;
> + goto out_locked;
> }
>
> error = cap_ioctl_limit_check(fdp, fd, cmds, ncmds);
> - if (error != 0) {
> - FILEDESC_XUNLOCK(fdp);
> - free(cmds, M_TEMP);
> - return (error);
> - }
> + if (error != 0)
> + goto out_locked;
>
> ocmds = fdp->fd_ofiles[fd].fde_ioctls;
> fdp->fd_ofiles[fd].fde_ioctls = cmds;
> fdp->fd_ofiles[fd].fde_nioctls = ncmds;
> + cmds = ocmds;
>
> +out_locked:
> FILEDESC_XUNLOCK(fdp);
> -
> - free(ocmds, M_TEMP);
> -
> - return (0);
> +out:
> + free(cmds, M_TEMP);
> + return (error);
> }
>
> int
> @@ -396,8 +391,8 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioctls_get_args *uap)
> FILEDESC_SLOCK(fdp);
>
> if (fget_locked(fdp, fd) == NULL) {
> - FILEDESC_SUNLOCK(fdp);
> - return (EBADF);
> + error = EBADF;
> + goto out;
> }
>
> /*
> @@ -410,19 +405,18 @@ sys_cap_ioctls_get(struct thread *td, struct cap_ioctls_get_args *uap)
> if (cmds != NULL && fdep->fde_ioctls != NULL) {
> error = copyout(fdep->fde_ioctls, cmds,
> sizeof(cmds[0]) * MIN(fdep->fde_nioctls, maxcmds));
> - if (error != 0) {
> - FILEDESC_SUNLOCK(fdp);
> - return (error);
> - }
> + if (error != 0)
> + goto out;
> }
> if (fdep->fde_nioctls == -1)
> td->td_retval[0] = INT_MAX;
> else
> td->td_retval[0] = fdep->fde_nioctls;
> + error = 0;
>
> +out:
> FILEDESC_SUNLOCK(fdp);
> -
> - return (0);
> + return (error);
> }
>
> /*
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 at gmx.de>
> Date: Sun, 24 Feb 2013 14:00:01 +0100
> Subject: [PATCH 18/24] (Hopefully) slightly improve manpages and comments.
>
> ---
> 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(-)
>
> 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 checking.
> 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_limit.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 obtained
> +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_limit.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 obtained
> +The list of allowed ioctl commands for a given file descriptor can be obtained
> 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 obtained
> -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_limit.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 file descriptor will be
> limited to those permitted by
> .Fa rights .
> .Pp
> -Bitmask of capability rights assigned to a file descriptor can be obtained with
> +A bitmask of capability rights assigned to a file descriptor can be obtained 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 with 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);
>
> /*
> * 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);
>
> @@ -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 at gmx.de>
> Date: Sun, 24 Feb 2013 14:00:31 +0100
> Subject: [PATCH 19/24] Sort Xr.
>
> ---
> lib/libc/sys/cap_rights_limit.2 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/libc/sys/cap_rights_limit.2 b/lib/libc/sys/cap_rights_limit.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 at gmx.de>
> Date: Sun, 24 Feb 2013 14:04:36 +0100
> Subject: [PATCH 20/24] Avoid comparing signed with unsigned values.
>
> ---
> sys/fs/nfsserver/nfs_nfsdport.c | 2 +-
> sys/kern/sys_capability.c | 7 ++++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.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 **fpp, struct vnode **vpp)
> int error = 0;
>
> fdp = p->td_proc->p_fd;
> - if ((u_int)fd >= fdp->fd_nfiles ||
> + if (fd < 0 || fdp->fd_nfiles <= fd ||
> (fp = fdp->fd_ofiles[fd].fde_file) == NULL) {
> error = 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;
>
> FILEDESC_LOCK_ASSERT(fdp);
> KASSERT(fd >= 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;
>
> oncmds = fdp->fd_ofiles[fd].fde_nioctls;
> if (oncmds == -1)
> return (0);
> - if (oncmds < ncmds)
> + if ((size_t)oncmds < ncmds)
> return (ENOTCAPABLE);
>
> ocmds = 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 at gmx.de>
> Date: Sun, 24 Feb 2013 15:09:16 +0100
> Subject: [PATCH 21/24] For readability simplify if (x) y = a; else y = b; with
> long y to y = x ? a : b.
>
> ---
> sys/kern/kern_descrip.c | 10 +++-------
> sys/kern/sys_capability.c | 6 ++----
> 2 files changed, 5 insertions(+), 11 deletions(-)
>
> 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 new,
> fdp->fd_ofiles[new] = fdp->fd_ofiles[old];
> filecaps_copy(&fdp->fd_ofiles[old].fde_caps,
> &fdp->fd_ofiles[new].fde_caps);
> - if ((flags & DUP_CLOEXEC) != 0) {
> - fdp->fd_ofiles[new].fde_flags =
> - fdp->fd_ofiles[old].fde_flags | UF_EXCLOSE;
> - } else {
> - fdp->fd_ofiles[new].fde_flags =
> - fdp->fd_ofiles[old].fde_flags & ~UF_EXCLOSE;
> - }
> + fdp->fd_ofiles[new].fde_flags = 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 = new;
> *retval = 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_ioctls_get_args *uap)
> if (error != 0)
> goto out;
> }
> - if (fdep->fde_nioctls == -1)
> - td->td_retval[0] = INT_MAX;
> - else
> - td->td_retval[0] = fdep->fde_nioctls;
> + td->td_retval[0] =
> + fdep->fde_nioctls != -1 ? fdep->fde_nioctls : INT_MAX;
> error = 0;
>
> 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 at gmx.de>
> Date: Sun, 24 Feb 2013 15:23:15 +0100
> Subject: [PATCH 22/24] Unify and simplify bitset inclusion tests.
>
> - (have | need) != have looks like a typo: shouldn't the | be a &?.
Which line exactly?
> - some places used (have | need) != have, others (have & need) == need.
> - (need & ~have) != 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(-)
>
> 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)
> {
>
> - KASSERT((fcaps->fc_rights | CAP_MASK_VALID) == CAP_MASK_VALID,
> + KASSERT((fcaps->fc_rights & ~CAP_MASK_VALID) == 0,
> ("%s: invalid rights", func));
> - KASSERT((fcaps->fc_fcntls | CAP_FCNTL_ALL) == CAP_FCNTL_ALL,
> + KASSERT((fcaps->fc_fcntls & ~CAP_FCNTL_ALL) == 0,
> ("%s: invalid fcntls", func));
> KASSERT(fcaps->fc_fcntls == 0 || (fcaps->fc_rights & CAP_FCNTL) != 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)
> {
>
> - if ((have | need) != have) {
> + if ((need & ~have) != 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_rights_limit_args *uap)
> AUDIT_ARG_FD(fd);
> AUDIT_ARG_RIGHTS(rights);
>
> - if ((rights | CAP_ALL) != CAP_ALL)
> + if ((rights & ~CAP_ALL) != 0)
> return (EINVAL);
>
> fdp = td->td_proc->p_fd;
> @@ -452,7 +452,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fcntls_limit_args *uap)
> AUDIT_ARG_FD(fd);
> AUDIT_ARG_FCNTL_RIGHTS(fcntlrights);
>
> - if ((fcntlrights | CAP_FCNTL_ALL) != CAP_FCNTL_ALL)
> + if ((fcntlrights & ~CAP_FCNTL_ALL) != 0)
> return (EINVAL);
>
> fdp = td->td_proc->p_fd;
> @@ -463,8 +463,7 @@ sys_cap_fcntls_limit(struct thread *td, struct cap_fcntls_limit_args *uap)
> return (EBADF);
> }
>
> - if ((fdp->fd_ofiles[fd].fde_fcntls | fcntlrights) !=
> - fdp->fd_ofiles[fd].fde_fcntls) {
> + if ((fcntlrights & ~fdp->fd_ofiles[fd].fde_fcntls) != 0) {
> FILEDESC_XUNLOCK(fdp);
> return (ENOTCAPABLE);
> }
> @@ -515,7 +514,7 @@ sys_cap_new(struct thread *td, struct cap_new_args *uap)
> AUDIT_ARG_FD(fd);
> AUDIT_ARG_RIGHTS(rights);
>
> - if ((rights | CAP_ALL) != CAP_ALL)
> + if ((rights & ~CAP_ALL) != 0)
> return (EINVAL);
>
> fdp = td->td_proc->p_fd;
> diff --git a/usr.bin/procstat/procstat_files.c b/usr.bin/procstat/procstat_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 = 0;
> width = 0;
> for (i = 0; i < cap_desc_count; i++) {
> - if ((rights & cap_desc[i].cd_right) == cap_desc[i].cd_right) {
> + if ((cap_desc[i].cd_right & ~rights) == 0) {
> width += 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 = 0; i < cap_desc_count; i++) {
> - if ((rights & cap_desc[i].cd_right) == cap_desc[i].cd_right) {
> + if ((cap_desc[i].cd_right & ~rights) == 0) {
> printf("%s%s", count ? "," : "", cap_desc[i].cd_desc);
> width += 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 at gmx.de>
> Date: Sun, 24 Feb 2013 15:33:48 +0100
> Subject: [PATCH 23/24] Simplify assertion condition, which contains a
> duplicated subexpression.
>
> ---
> sys/kern/kern_descrip.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> 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, const char *func)
> ("%s: invalid fcntls", func));
> KASSERT(fcaps->fc_fcntls == 0 || (fcaps->fc_rights & CAP_FCNTL) != 0,
> ("%s: fcntls without CAP_FCNTL", func));
> - KASSERT(((fcaps->fc_nioctls == -1 || fcaps->fc_nioctls == 0) &&
> - fcaps->fc_ioctls == NULL) ||
> - (fcaps->fc_nioctls > 0 && fcaps->fc_ioctls != NULL),
> + KASSERT(fcaps->fc_ioctls != NULL ? fcaps->fc_nioctls > 0 :
> + (fcaps->fc_nioctls == -1 || fcaps->fc_nioctls == 0),
> ("%s: invalid ioctls", func));
> KASSERT(fcaps->fc_nioctls == 0 || (fcaps->fc_rights & CAP_IOCTL) != 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 at gmx.de>
> Date: Sun, 24 Feb 2013 15:47:53 +0100
> Subject: [PATCH 24/24] Factorise code to free a file descriptor.
>
> ---
> sys/kern/kern_descrip.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> 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)
> }
>
> /*
> + * Free a file descriptor.
> + */
> +static void
> +fdfree(struct filedesc *fdp, int fd)
> +{
> + struct filedescent *fde = 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);
>
> /* 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)
>
> FILEDESC_XLOCK(fdp);
> if (fdp->fd_ofiles[idx].fde_file == 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 = fde->fde_file;
> if (fp != NULL && (fp->f_type == 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!
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20130225/af380cc4/attachment.sig>
More information about the freebsd-arch
mailing list