[Swan-dev] state m/c 2of3: State machine cleanups

Andrew Cagney andrew.cagney at gmail.com
Wed Mar 4 03:56:54 EET 2015

On 3 March 2015 at 16:42, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> | From: Andrew Cagney <andrew.cagney at gmail.com>
> (I didn't write the centralized V2 code driven by the microcode but it
> was kind of copied from what I did in V1.)
> | - I earlier posted questions related to ikev2_process_payloads() - it
> | is, to me, doing more than it should.
> | By moving its search logic into the main search-for-state-transition
> | loop things get more transparent, and SMF2_CONTINUE_MATCH (which
> | scares me) can also be deleted.
> | In addition to checking the clear payload, SMF2_UNPACK_SK indicates
> | that the SK (encrypted) payload can be checked - less stuff for my
> | rekey states to deal with
> The following is based on my imperfect memory.  With no checking.
> Beware.
> I hacked in the SMF2_CONTINUE_MATCH code.  Why?
> - decryption was not done by the central routine: the per-transition
>   routines do it.
> - in some cases, decryption exposes information that affects microcode
>   selection.  So a second go at matching was required

It sounds like the code in this area has evolved then.  Thanks for the

Currently it is only used to differentiate between two I1 transitions -
expected INIT reply and some other INIT reply such as INVALID_KE.  Both of
these cases are deal with clear text.

> Some thoughts:
> - I think that it would be better to have the centralized routine do
>   the decryption before calling the appropriate per-transition
>   routine.

I did the change in stages:

- first I split ikev2_process_payload into several parts:
-> code to decode the payload and record the results
-> code to check the results match expected
-> code to log unexpected results
the code looping over SMF2_CONTINUE_MATCH could then be moved into
process_v2_packet so that contained separate loops

- then I merged the two loops

- finally I added stuff to continue to the SK payload if the applicable bit
is set; it just re-used the above (along with a tweaked decrypt payload

> - I think that there are some cases where a bit of transition-specific
>   code is needed before the keying material for decryption is
>   available.  (So it was easier to hack in what I did rather than
>   making larger changes.)

Yes. In pluto for IKEv2, the original responder only completes the DH
calculation when the AUTH request arrives (it's a smart trick).  In V1 it
is done in the background.  This is why I added a flag to mark that things
could be decrypted (and allow smother migration).

> - ikev2_process_payloads gets called twice on an encrypted message:
>   the first gobbles just the single E payload; the second gobbles the many
>   payloads within the decrypted E payload.

> - I think that it would be clearer if the E payload were special-cased
>   and that ikev2_process_payload were only called once
>   and that the payload checking only be done once
>   and only then would the per-transition function be called.
>   But the proof would be if the code were  simplified by this.

See above, it looks something like this:

        for (svm = v2_state_microcode_table; svm->state != STATE_IKEv2_ROOF;
             svm++) {
                if (svm->state != from_state)
                if (svm->recv_type != ix)
                 * Does the original initiator flag match?
                if ((svm->flags & SMF2_IKE_I_SET) && !ike_i)
                if ((svm->flags & SMF2_IKE_I_CLEAR) && ike_i)
                 * Does the message reply flag match?
                if ((svm->flags & SMF2_MSG_R_SET) && !msg_r)
                if ((svm->flags & SMF2_MSG_R_CLEAR) && msg_r)
                 * Since there's a state that, at least, looks like it
                 * will accept the packet, unpack the clear payload
                 * and continue matching.
                if (clear_payload_summary.status != STF_OK) {
                        DBG(DBG_CONTROL, DBG_log("Unpacking clear payload
for svm: %s", svm->story));
                        clear_payload_summary = ikev2_decode_payloads(md,

                        if (clear_payload_summary.status != STF_OK) {
                struct ikev2_payload_errors clear_payload_errors
                        = ikev2_verify_payloads(clear_payload_summary, svm,
                if (clear_payload_errors.status != STF_OK) {
                        /* Save this failure for later logging. */
                        clear_payload_status = clear_payload_errors;
                 * Continue checking by unpacking the SK payload but
                 * only its ok and there's one to unpack.  Otherwise
                 * assume its a correct match.
                if (!(svm->flags & SMF2_UNPACK_SK)) {
                if (!(svm->req_clear_payloads &
                if (enc_payload_summary.status != STF_OK) {
                            DBG_log("Unpacking encrypted payload for svm:
                        stf_status status =
                        if (status != STF_OK) {
                                complete_v2_state_transition(mdp, status);
                        unsigned np =
                        enc_payload_summary = ikev2_decode_payloads(md,
&md->clr_pbs, np);
                        if (enc_payload_summary.status != STF_OK) {
                struct ikev2_payload_errors enc_payload_errors
                        = ikev2_verify_payloads(enc_payload_summary, svm,
                if (enc_payload_errors.status != STF_OK) {
                        /* Save this failure for later logging. */
                        enc_payload_status = enc_payload_errors;
                /* must be the right state machine entry */

(inside verify_and_decrypt_payload there's a check that crypto is ready, I
should probably make that explicit here)
SMF2_UNPACK_SK provides a migration path and addresses the original
responder trick.

> My feeling is that a significant amount of code that is replicated in
> several per-transition functions could be moved into a central routine
> and gated by microcode.  That was the motivation for the microcode
> construct in V1.

Events, such as timeouts, and even request initiation should get dispatched
by the state machine.  Make this state machine behave like a state machine.

> An example of centralizing code is moving the timeout-setting logic out of
> the per-transition functions.  I think that Antony has done (most of?
> all of?) this.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20150303/7226d156/attachment-0001.html>

More information about the Swan-dev mailing list