[Swan-dev] text_said() calls in create_xfrm_migrate_sa() puzzle me

[Andrew Cagney]

On Sat, 19 Jun 2021 at 15:35, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> I don't understand this code but I'm "improving" it, based on symmetry.
> I've hit a snag.  Advice welcome!
>
> There are four calls to text_said.  Here's what they look like, with
> details removed:
>





> I'm a little surprised and puzzled that the second args to
> set_text_said calls go
>         dst
>         src
>         src
>         dst
> My intuition says that these should each be "dst".
>

Here's some more (extracted) context:

        if (endpoint_is_specified(st->st_mobike_local_endpoint)) {
                char *n = jam_str(text_said, SAMIGTOT_BUF, "initiator
migrate kernel SA ");
      } else {
                char *n = jam_str(text_said, SAMIGTOT_BUF, "responder
migrate kernel SA ");

so the code is detailing with a polarity reversal,  But wait there's more:

                        sa.spi = proto_info->our_spi;
                        sa.spi = proto_info->attrs.spi;
                        sa.spi = proto_info->our_spi;
                        sa.spi = proto_info->attrs.spi;

                        sa.dst.new_address =
endpoint_address(st->st_mobike_local_endpoint);
                        sa.src.new_address =
endpoint_address(st->st_mobike_local_endpoint);
                        sa.src.new_address =
endpoint_address(st->st_mobike_remote_endpoint);
                        sa.dst.new_address =
endpoint_address(st->st_mobike_remote_endpoint);

So now we've got a double polarity reversal with an SPI twist.  11 points!

The set_text_said() calls seem to match .new_address, so I suspect it is
logging the old address.


> Even if that's not correct, it feels as if the either the first pair
> or the second pair is reversed.
>
> Note: the text_said only appears in logging so being wrong has no
> serious consequences.
>

Even better, debug logging:

        char reqid_buf[ULTOT_BUF + 32];
        endpoint_buf ra;
        snprintf(reqid_buf, sizeof(reqid_buf), ":%u to %s reqid=%u %s",
                        old_port,
                 str_endpoint(&new_endpoint, &ra),
                        sa.reqid,
                        enum_name(&netkey_sa_dir_names, dir));
        add_str(text_said, SAMIGTOT_BUF, text_said, reqid_buf);
        dbg("%s", text_said);

What about dropping set_text_said() and then dump everything using a single
call?  Hmm, SAMIGTOT_BUF takes a critical hit, and both add_str() and
set_text_said() suffer damage.
Personally, I'd be looking at how create_xfrm_migrate_sa() and
migrate_xfrm_sa() interact.



> Observations:
>
> If !(dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD)
> then dir == XFRM_POLICY_OUT.
>
> Kernel IPSec SA's are unidirectional.
> The SAID includes the destination address of packets carried by the SA.
> I think that in each case, the destination address is "dst".
>
> There are currently three calls to this function, all within one
> statement of the function netlink_migrate_sa:
>         return
>                 create_xfrm_migrate_sa(st, XFRM_POLICY_OUT, &sa, mig_said)
> &&
>                 migrate_xfrm_sa(&sa, st->st_logger) &&
>
>                 create_xfrm_migrate_sa(st, XFRM_POLICY_IN, &sa, mig_said)
> &&
>                 migrate_xfrm_sa(&sa, st->st_logger) &&
>
>                 create_xfrm_migrate_sa(st, XFRM_POLICY_FWD, &sa, mig_said)
> &&
>                 migrate_xfrm_sa(&sa, st->st_logger);
>
>
> dir is the second argument of create_xfrm_migrate_sa.  Clearly it can
> only have one of three values: XFRM_POLICY_OUT, XFRM_POLICY_IN,
> XFRM_POLICY_FWD.  So the test of dir could be simplified to
>         dir != XFRM_POLICY_OUT
>
> Each IPSec SA is unidirectional.  That's why they come in pairs, one
> for each direction.
>
> The SAID is a triple: destination IP, SPI, protocol (eg. AH or ESP).
>
> The test dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD must be
> telling us whether we're dealing with an inbound SA.  If it's true, it
> must be inbound.
> _______________________________________________
> 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/20210621/908e6a3a/attachment.html>