[illumos-Developer] webrev: removal of closed kcfd

[Garrett D'Amore]

On Mon, 2010-09-06 at 19:40 -0400, Richard Lowe wrote:
> Garrett D'Amore wrote:
> > http://mexico.purplecow.org/gdamore/webrev/nokcfd/
> 
> general:
> 
>   - Is there any worth in leaving FIPS support sufficient to lock out
>     non-FIPS algorithms and modes, run the self-tests, and just not do
>     module verification?  I appreciate that's not compliant, but is it
>     useful?
> 
>     You and I had discussed this offline, and mostly thought "No", since
>     the value of FIPS is solely the certification.

Right.  Generally, any new effort to redo FIPS would need to consider
many many issues, and might wind up taking a substantially different
path depending on what their goals are (what FIPS level, what crypto
boundary, etc.)

I think it would be very bad to offer any kind of "pseudo" FIPS, since
it clearly would not even fit the most basic design tenets of FIPS 140-2
without some kind of verified cryptographic infrastructure.  Without
module verification, quasi FIPS support is utterly pointless, IMO.


>   - Is the FIPS removal a simple backout of its introduction (and
>     subsequent).  Or done some other way?

It was done the "hard way", since I was also removing the module signing
at the same time, and there were shared bits.

> 
>     (I'm looking for an indication it's suitably complete)

In fact, I left some of the fips stuff (the "self-test" algorithm
support) in, because they are likely to be useful to future efforts.
However, I'm willing to remove that if other folks want to see the
currently-not-used code removed.

> 
>   - Did you test that priocntl, pbind, etc, function on your process?

Good question.  Answer: No.  In fact, I haven't really tested the
process logic at all.  All I've done is verify that the lwps are present
in PS. :-)  I still need to test the actual kernel crypto code.  I would
very much welcome concrete testing suggestions or help.

> 
> usr/src/cmd/cmd-crypto/cryptoadm/cryptoadm.c:1211,1212:
> 
>       nit: Self evident.

Go back and reread the comment.  It says "used to" .. as in, not any
more.

> 
> usr/src/cmd/cmd-crypto/cryptoadm/cryptoadm.h:40
> 
>       Should no longer be necessary

Accepted.

> 
> usr/src/uts/common/crypto/core/kcf.c:95
> 
>       Because we can do this at this point, with your changes, rather
>       than when kcfd shows us the door as before, right?

Right.

> 
> usr/src/uts/common/crypto/core/kcf_sched.c:206
> 
>       nit: A period is one ., an ellipsis is three.  You can't have
>       two.

Accept.  Its a period now.

> 
> usr/src/uts/common/crypto/core/kcf_sched.c:1027
> 
>       cv_reltimedwait() should never return 0, only -1 on timeout,
>       and > 0 when signalled (cv_signal), no?

Yes.  I left the explicit case for 0 in case someone decides that
signaling kcfpoold is a good idea, and to illustrate that I thought
about it.  Technically I can remove the 0 case if someone is offended by
it.

> 
> usr/src/uts/common/crypto/core/kcf_sched.c:1395,1396
> 
>       nit: process names don't agree, "kcfpool" v. "kcfpoold"

I fixed the comment.  Its kcfpoold.

> 
> usr/src/uts/common/sys/crypto/sched_impl.h:379
> 
>       "... also protects the three fileds above"
> 
>       This is probably not true, as you deleted 5 fields preceding
>       this comment, if it is still true, please fix the spelling of
>       fields.

Accept.  It doesn't protect any fields, so I removed the incorrect
statement.

> 
> usr/src/uts/common/sys/crypto/spi.h:48
> usr/src/uts/common/sys/crypto/spi.h:493,500
> usr/src/uts/common/sys/crypto/spi.h:536,548
> usr/src/uts/common/sys/crypto/spi.h:566
> 
>       co_fips140_ops is useless, after your changes.  Can it be safely
>       removed, given that the API is versioned?  Or does it make more
>       sense to leave CRYPTO_SPI_VERSION_4, so the API versioning
>       continues to line up.

I prefer the latter.  It will make future merges less painful, if they
ever occur, and I hate the idea of a mixup even being *possible* with
someone mixing current V4 code against some future V4 or later.

> 
>       Either way, I think you should remove the implementations of this
>       entry point from the VERSION_4 providers, since the code will
>       never be called.

This goes to the question about dead code.  I think I'm coming to agree
with you on this, although it will be seriously annoying if ever anyone
tries to reinstate these.

I'm going to submit a new webrev soon, nightly found a few lint
complaints, and I gutted the crypto support from nightly as well.

Stay tuned.

	-- Garrett

> 
> -- Rich