Re: Support for multi-position electro-mechanical keylocks

[Christoph Badura <bad%bsd.de@localhost>]

On Sun, Aug 02, 2009 at 11:54:09AM +0200, Marc Balmer wrote:
> - Generic support for keylocks in the kernel.  The number of keylock  
> positions and the current keylock position can be read from the kernel  
> using two functions, userland can access them through the  
> hw.keylock.npos and hw.keylock.pos sysctl variables.

Is that for a single keylock only? Why don't you attach it at 
hw.keylock.0.numpos and  hw.keylock.0.curpos instead and make it 
generic from the start?  I'm also for more descriptive names.

I ask because just this week I've seen boxen with two keylocks at work.

> - secmodel_keylock, a kauth(9) security model that authorizes based on  
> the keylock "closedness".

For me it is more natural to think of the lock as "unlocked" and "locked" (
or perhaps as "engaged").  To me the code would be clearer if it would
test for the lock's state to be "locked" or "unlocked".  Whether there is
a key in the lock is IMHO irrelevant.  Therefore I'd name the variable
"lockstate" not "kstate" which seems to indicate the state of the key to
me.

Could you describe what exactly it authorizes?  I noticed that only a
fraction of the possible kauth actions are controlled by the secmodel.
I think we'd need a specification of what it is supposed to control before
we can talk about the secmodel itself.

> Wheter the rightmost (default) or leftmost  
> position of the keylock means open can be controlled using the  
> security.models.keylock.order sysctl variable (access to which will be  
> protected later).

How is that controlled through that sysctl variable?  I is not clear to
me.

> The enable this, "options KEYLOCK" and "options secmodel_keylock" must  
> be set in the kernel configuration; to use the gpiolock(4) driver att  
> a "gpiolock* at gpio?" line.

This seems awkward to me.  So we need the following lines:
options KEYLOCK
options secmodel_keylock
gpiolock* at gpio?

It looks to me like kern_keylock.c isn't optional from either
secmodel_keylock and gpiolock(4).  Hard to tell, though because there
is no documentation.

I think "options KEYLOCK" is unnecessary.  secmodel_keylock should pull
that file in automatically.  And so should gpiolock.

And we should, of course, get the secmodel_XXX_start() calls out of
secmodel_bsd44_start().  That is a completely ridiculous place for them
to be.

I have two more questions: Why do we want that secmodel in tree?  And why
isn't it a kernel module that gets loaded during system bootstrap?

--chris