I like constructive feedback.
> * Michael A. Halcrow (mahalcro@private) wrote:
> style nitpick:
>
> case 0:
> case -1:
> default:
> break;
> }
>
> is preferred.
The MPlayer guys once rejected a patch because I had *two* spaces
after a period in one of my sentences in my HTML documentation, rather
than just *one* space. Of course, we all know that whitespace between
word characters has zero impact on the rendered text, but we can't be
having two-space ugliness between the words in the source HTML now,
can we? :-) Their argument had something to do with European
standards and American standards for fixed-length fonts. I averted a
potential flame war and re-submitted the patch with just one space.
And averting a flame war with the MPlayer guys really is an
accomplishment worth noting. At that point, my patch was deemed ``too
complicated''...
> AFAICT, these don't actually justify the new hooks. The CAP_SYS_MODULE
> capable() test could be added above to the seclvl_capable() switch, and
> it would achieve the seclvl protection from module load/unload, right?
Right. We've actually flip-flopped between using CAP_SYS_MODULE and a
custom hook a couple of times. I don't recall exactly why we stayed
with the hook rather than the capability, so I'm changing it back to
the capability (until Serge complains).
> This is racy. How about something that uses the exported kernel api's?
>
> Like:
>
> struct super_block *sb;
> if (S_ISBLK(inode->i_mode) {
> struct block_device *bdev = inode->i_bdev;
> sb = get_super(bdev);
> if (sb) {
> drop_super(sb);
> return 1;
> }
> }
> return 0;
No, I'm afraid that this would *never* work. There are mismatched
parenthesis. :-P Okay, I've put something like this in place of what
was there. Thanks for the suggestion.
> I'm not sure I'd excpect this kind of magic number to persist.
> There are people who push for dynamic and random major minor numbers.
> Is the CAP_SYS_RAWIO check not sufficient? Uhh, wait, does this check
> actually work? These are char devices, not block devices.
I actually have a functional verification script for the Secure Levels
LSM, which would have to go through IBM legal in order to be able to
publish. The script pokes and prods the system to make sure that
seclvl is doing its job. I run it every time I change something in
seclvl, just as a sanity check.
I ripped that check out nonetheless, and it still works the way we
expect; CAP_SYS_RAWIO does seem to be taking care of writes to
/dev/mem and /dev/kmem. We had a lot of redundancy in an earlier
incarnation of seclvl, and this one slipped past our redundancy
checks.
> This latter check could break the case where you set a file for
> mandatory locking. Not sure if that's intentional.
seclvl disallows many actions. As long as there are no actions that
get through that shouldn't, we have no qualms about that. If there is
an elegant way to not break so many things (without adding new hooks,
preferably), we're all ears. Specifically, capabilities are very much
blanket policies, and while they cover Secure Level policies too,
we're happy. In the meantime, anything that doesn't work will be
documented. Which reminds me, I need to write that.
> > +int seclvl_proc_register( void )
> > +{
> > + struct proc_dir_entry* entry;
> > + entry = create_proc_entry( "seclvl", S_IFREG | S_IRUGO, &proc_root );
>
> This is really /proc abuse. We are trying to move away from /proc.
> Perhaps we need to revisit sysfs infrastructure.
That was an artifact from an earlier implementation of the module last
year. We never really questioned its appropriateness as it relates to
recent changes in the kernel (until now).
I have read through kobject.txt and sysfs.txt and am in the process of
revising the module accordingly.
> I'm not sure how safe this is, since it provides an unauthenticated
> backdoor, no? At least just the dentry isn't subject to full all the various
> vfsmnt issues. However, couldn't one simply bind a new file over this
> one, then if you can't umount, this thing is effectively gone...
We'll look into this. My personal preference is to implement a
password-based seclvl reduction rather than to link it to an
executable file (echo "abracadabra" > /sysfs/security/seclvl). Now
that I think about it, this would be a great opportunity to use the
MD5 code that's now in the kernel ;-).
As far as the flooding issue goes, will the ``quiet'' and ``noisy''
verbosity levels be acceptable to address that issue? On my Debian
box (2.6.0-test9 kernel, sysklogd version 1.4.1), duplicate messages
are written to the logs as ``last message repeated 5 times''. What
further measures can be reasonably expected from us to address log
file flooding threats?
Another patch is in the woodworks.
Mike
This archive was generated by hypermail 2b30 : Tue Nov 25 2003 - 12:38:02 PST