Re: [PATCH ipsec-next v8 14/14] xfrm: add documentation for XFRM_MSG_MIGRATE_STATE

[Sabrina Dubroca <sd@queasysnail.net>]

Overall a very document, thanks. Some comments:

2026-05-05, 06:34:55 +0200, Antony Antony wrote:
> +    struct xfrm_user_migrate_state {
> +        struct xfrm_usersa_id  id;       /* spi, daddr, proto, family */
> +        xfrm_address_t         new_daddr;
> +        xfrm_address_t         new_saddr;
> +        struct xfrm_mark       old_mark; /* SA lookup: key = v & m */
> +        struct xfrm_selector   new_sel;  /* new selector (see Flags) */
> +        __u32                  new_reqid;
> +        __u32                  flags;    /* XFRM_MIGRATE_STATE_* */
> +        __u16                  new_family;
> +        __u16                  reserved;
> +    };

Thinking about the UAPI a bit more, maybe this would be a good time to
start introducing a "proper" netlink API for XFRM? Instead of having
the main properties in a fixed struct, and a few attributes as an
afterthought, use attributes as the main way to exchange information?

Then we can start adding the attributes as alternative to the fixed
headers in some other ops, and later start deprecating the current
API?

(for some reason this thought only popped up when I had the html
rendering in front of me, sorry it's so late in the process)

[...]
> +Flags
> +=====
> +
> +The ``flags`` field in ``xfrm_user_migrate_state`` controls optional
> +migration behaviour. Unknown flag bits are ignored.

Maybe better to reject unknown flag bits (as well as unknown
attributes beyond XFRMA_MAX), so that we can fully control their
behavior, and not risk incorrectly migrating an SA if a "too-recent"
userspace passes attributes we don't know (then if we fail to handle
them in the kernel, the SA may not handle the traffic).

(Steffen and you will have a much better understanding of the security
risks here than me)



> +Migration Steps
> +===============

maybe add:

Userspace is expected to:

> +#. Install a block policy to drop traffic on the affected selector.
> +#. Remove the old policy.
> +#. Call ``XFRM_MSG_MIGRATE_STATE`` for each SA.
> +#. Reinstall the policies.
> +#. Remove the block policy.
> +
> +Block Policy and IV Safety
> +--------------------------
> +
> +Installing a block policy before migration is required to prevent
> +traffic leaks and IV reuse in counter mode.
> +
> +AES-GCM IV uniqueness is critical: reusing a (key, IV) pair allows
> +an attacker to recover the authentication subkey and forge
> +authentication tags, breaking both confidentiality and integrity.
> +
> +``XFRM_MSG_MIGRATE_STATE`` atomically copies the sequence number and
> +replay window from the old SA to the new SA and deletes the old SA.
> +The block policy ensures no outgoing packets are sent in the migration
> +window, preventing IV reuse under the same key.

Does it matter that the copy is done atomically if we expect userspace
to install a block policy? (without the block, I'll have to recheck to
convince myself whether it would be safe, and TBH I'm still a bit
confused by patch 8)

> +Feature Detection
> +=================
> +
> +Userspace can probe for kernel support by sending a minimal
> +``XFRM_MSG_MIGRATE_STATE`` message with a non-existent SPI:
> +
> +- ``-ENOPROTOOPT``: not supported (``CONFIG_XFRM_MIGRATE`` not enabled)
> +- any other error: supported


xfrm_user_rcv_msg

if (type > XFRM_MSG_MAX)
        return -EINVAL;


Userspace will hit that on a kernel that may have migrate but not
XFRM_MSG_MIGRATE_STATE, no?


[...]
> +Error Handling
> +==============
> +
> +If the target SA tuple (daddr, SPI, proto, family) is occupied by an existing
> +unrelated SA, the operation returns ``-EEXIST``.

All this happens under xfrm_cfg_mutex, so if we did an initial lookup
for the new SA before starting the operation, we could ensure there's
no dupe, and the mutex would guarantee no insertion. No?

-- 
Sabrina