Large Capsicum patch for review.
Pawel Jakub Dawidek
pjd at FreeBSD.org
Mon Feb 25 21:49:04 UTC 2013
On Mon, Feb 25, 2013 at 11:32:27AM +0100, Christoph Mallon wrote:
> On 25.02.2013 00:59, Pawel Jakub Dawidek wrote:
> > On Sun, Feb 24, 2013 at 04:07:12PM +0100, Christoph Mallon wrote:
> >> On 24.02.2013 07:05, Christoph Mallon wrote:
> >> - 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>
>
> A nice name is a good step.
> Still, I would prefer, if consistently only one type (size_t) would be used instead of two/three (ssize_t, size_t and int for the constant).
Using three types here is not inconsistent, it is a common practise.
Check out even read(2) and write(2) - they take size as size_t, as there
is no need for a negative size, but they return ssize_t, because they
can return -1 if an error occured. This is exactly what I do in
cap_ioctls_get().
I use size_t as this is preferred type for size, but I don't need size_t
for iterator, because I know the value will never need more than 16
bits, so I use int as a more CPU-friendly type.
> >> - 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.
>
> This deserves a comment in the code.
I agree. Not my code, though.
> >> - 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.
>
> So effectively the history is lost.
The entire history is there, nothing is lost:
http://p4db.freebsd.org/
> > 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.
>
> In a branch in svn one could reread these steps on every commit.
> Or even better, use git(-svn), which can automatically create a nice patch series (like the one I provided).
Your changes were pretty simple, so they look nice as separate commits.
The patch you review is a result of ~2 months of work and exporting
every single change would not create such nice output. If those changes
were implemented totally separated, I could generate several independent
patches, but those changes were implemented together and trying to
separate them now would be, if possible, very time consuming. So
eventhough I agree it would be easier to review them separately, I only
have this single patch.
> >> --- 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.
>
> cap_getmode() is declared with u_int, so I used it.
Hmm, I wanted to change cap_getmode(2) argument to 'unsigned int', but
many syscalls (at least as defined in syscalls.master) use u_int/u_long,
eventhough they are sometimes documented differently, eg. ioctl(2).
> >> --- 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.
>
> It is always suspicious, if a magic number besides 0 is used.
Not really. Majority of syscalls returns exactly -1 on error.
The manual page says -1 on error, not something other than 0, although
on the other hand it only returns 0 on success. I agree that in most
cases it is better to check against 0 - if a syscall will suddenly start
to return something other than 0 and -1, it is better to assume failure.
In this very case we want to return true only if we are very sure the
process is in sandbox in case it wants to execute some risky code based
on the fact if it is in sandbox or not, so something like this would be
best:
if (cap_getmode(&mode) != 0) {
assert(errno == ENOSYS);
return (false);
}
assert(mode == 0 || mode == 1);
return (mode == 1);
We return 'false' if cap_getmode() returns -1 or any unexpected value
(your version), but we then return 'false' when 'mode' is either 0 or
any unexpected number (my version).
> >> --- 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.
>
> The manpage says, you need to include stdbool.h yourself.
I'm waiting for an opinion on this from one person still.
> >> --- 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.
>
> The duplicate tests do not even match!
> You have to carefully read the code to verify, that if indeed only triggers, when the for loop terminated without break.
> Code duplication is almost always bad, especially if the duplicated code does not fully match.
I don't agree, sorry. Your loop looks like endless loop at first. If the
loop would be more complex, it would be hard to tell at first glance if
the loop even terminates. For me it looks awkward - there is a place in
'for ()' for a check, which defines when the loop should end; IMHO it is
there for a reason. The construction I use is widely used, at least in
FreeBSD code and looks obvious to me.
> >> --- 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);
> >
> > Applied, but I removed first goto and now out label is placed before
> > unlock.
>
> This leaves code duplication in place, which is error prone.
But eliminates extra goto label that is used for exactly one case.
Once we grow at least one more error condition that doesn't need
unlocking and I'm fine with adding it.
> >> --- 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.
>
> Why did you mismatch the types of i and ncmds as well as j and oncmds?
I use 'int' to compare with 'ssize_t' and 'u_int' to compare with
'size_t'. I tried to explain this at the begining of my e-mail.
> >> --- 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.
>
> I had to read these lines very carefully to be sure that they actually assign to the same variables.
> Code duplication is really hurts readability.
Trying to squeeze as much as possible into one line and then breaking
the line into three doesn't cure readability either, IMHO.
It's probably a matter of taste, but my version is more readable for me.
> Maybe &fdp->fd_ofiles[old] and &fdp->fd_ofiles[new] should be placed in local variables.
Good idea, see:
http://p4db.freebsd.org/changeView.cgi?CH=222366
> >> 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?
>
> I did /not/ say, it is a typo.
> I said, it /looks like/ a typo due to the | instead of the expected & in a bit test.
> Therefore I replaced the test by more conventional ones, which also feature less code duplication.
To be honest I was a bit confused by it initially too. This is something
that was there when I came and I decided to left it that way, but I
think you convinced me to use more common construct. I applied your
changes.
> >> 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.
>
> You have to read this long and split up expression very carefully, to realize, that the cases are mutually exclusive.
> ?: makes this immediately clear: Check two different cases, depending on whether it is a null pointer or not.
Ok, I applied your change, after looking at it more it definiately is
simpler, although I don't think ?:'s result is boolean, but that's not
a big deal.
I updated the patch:
http://people.freebsd.org/~pjd/patches/capkern.diff
--
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/fdf37eb2/attachment.sig>
More information about the freebsd-arch
mailing list