Re: [LTP] [PATCH] securebits: add secure_keepcaps testcases

[Serge E. H. <ser...@ca...>]

Quoting Garrett Cooper (yan...@gm...):
> On Mon, Oct 4, 2010 at 7:06 AM, Serge E. Hallyn
> <ser...@ca...> wrote:
> > Quoting Garrett Cooper (yan...@gm...):
> >> Hi Serge,
> >>     Some comments about your provided code.
> >
> > Thanks.
> >
> >> > +AC_DEFUN([LTP_CHECK_SECUREBITS],
> >> > +AC_CHECK_HEADERS(linux/securebits.h,[
> >> > +       LTP_SECUREBITS=yes
> >> > +])
> >> > +)
> >>
> >> Some checks should probably be added for versioning as well as symbols
> >> that get passed to prctl(2) (I'm not sure if checking for the symbols
> >> that get passed to prctl(2) here is the correct way to go about things
> >> though).
> >
> > Not sure how we would check the versioning, bc there is no versioning
> > info in the interface.
> 
> Just checking for the symbols used with an autoconf test would be ok,
> because according to the kernel.org manpage [1] some of these symbols
> have only existed for the past year or two

Right, but before that the header file wouldn't have existed.  The
symbols appeared with the header file's creation.

Of course someone can shoot himself in the foot with older kernel on
newer userspace.  I don't mind doing the extra checks, it'll just take
me a few weeks to get the chance.  The tests aren't going to go stale
in the meantime, so no big whoop.

> (and thus someone like
> Mitani-san will come on the list and say that RHEL 4.x or 5.x compiles
> are broken by the new test :)).

My theory is that this test will suffice for older RHEL :)  but
not for more experimental chaps, I guess.

> > ...
> >
> >> > +       case 3:
> >> > +               ret = prctl(PR_GET_SECUREBITS);
> >>
> >> What if this call fails?
> >
> > It doesn't pass or fail.  The return value is simply the current
> > securebits.
> 
> According to the manpage [1], this syscall can fail.

I don't actually see where the syscall says it can fail (it says that
for CAPBSET_READ, but not for GET_SECUREBITS.  So it can only fail
if the capability module's prctl() isn't called.  I know of no ways
that can happen with current upstream, bc smack, selinux, apparmor
and tomoyo all do not define security_prctl(), which means that the
capability one will be called.

But there's really nothing preventing that situation in the future.
In which case right now we'll cache the error when SET_SECUREBITS
either returns -ENOSYS or returns an error bc of invalid bits.

In any case, an extra check won't hurt.  I just felt the need to
double-check my original thinking :)

> >> > +               ret = prctl(PR_SET_SECUREBITS, ret | SECBIT_KEEP_CAPS);
> >> > +               if (ret == -1) {
> >> > +                       tst_resm(TFAIL|TERRNO, "PR_SET_SECUREBITS failed\n");
> >> > +                       tst_exit();
> >> > +               }
> >
> >> > +#!/bin/sh
> >> > +
> >> > +echo "testing keepcaps"
> >> > +check_keepcaps 1
> >> > +tmp=$?
> >> > +if [ $tmp -ne 0 ]; then
> >> > +       exit_code=$tmp
> >> > +fi
> >> > +check_keepcaps 2
> >> > +tmp=$?
> >> > +if [ $tmp -ne 0 ]; then
> >> > +       exit_code=$tmp
> >> > +fi
> >> > +check_keepcaps 3
> >> > +tmp=$?
> >> > +if [ $tmp -ne 0 ]; then
> >> > +       exit_code=$tmp
> >> > +fi
> >> > +
> >> > +exit $exit_code
> >>
> >> What if (for instance) test 1 fails, and tests 2 or 3 pass?
> >
> > Yeah, I didn't do that right, and maybe it would be best
> > to just shortcut on the first failure anyway.
> 
> That's what I thought. The only thing you lose is coverage potentially
> if one of the tests is broken :/.

Yup, which is probably fine - if any one of these breaks, it'll
be a huge deal imo.

-serge