Hi Michał,
On Mon, 2019-09-09 at 21:25 +0200, Michał Lowas-Rzechonek wrote:
> This patch ensures that Sequence Number is reset only when IV Index used
> for outgoing messages increases.
>
> This fixes erroneously cleared sequence number when node performs IV
> Recovery procedure on startup in a following scenario:
> - node has IV Index set to <N>
> - node starts in IV_UPD_INIT state
> - node receives a Secure Network Beacon with IV Index <N>+1 and IV
> Update flag set
>
> Upon reception, the node shall:
> - increase its IV Index to <N>+1
> - enter IV_UPD_UPDATING state
>
> This means that the node keeps transmitting messages using IV Index
> equal to <N>, therefore it shall *not* reset its Sequence Number before IV
> Update procedure completes.
>
> If, on the other hand, SNB contains either:
> - IV Index <N>+2 (regardless of IV Update flag)
> - IV Index <N>+1 and IV Update flag *not* set
> the node shall reset the Sequence Number right away.
> ---
> mesh/net.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/mesh/net.c b/mesh/net.c
> index 7c4049e0e..ba7bb32fd 100644
> --- a/mesh/net.c
> +++ b/mesh/net.c
> @@ -2735,8 +2735,6 @@ static void update_iv_kr_state(struct mesh_subnet *subnet, uint32_t iv_index,
> }
>
> if (net->iv_upd_state == IV_UPD_INIT) {
> - if (iv_index > net->iv_index)
> - mesh_net_set_seq_num(net, 0);
> net->iv_index = iv_index;
>
> if (iv_update) {
> @@ -2757,6 +2755,12 @@ static void update_iv_kr_state(struct mesh_subnet *subnet, uint32_t iv_index,
> mesh_config_write_iv_index(node_config_get(net->node), iv_index,
> net->iv_upd_state);
>
> + /* Reset seq num if iv index used for *outgoing* messages has
> + * just been increased
> + */
> + if (mesh_net_get_iv_index(net) > local_iv_index)
> + mesh_net_set_seq_num(net, 0);
> +
I think there may be a more subtle problem at work here... Are we losing
the local state of IVU when we power off/reboot?
I think this will fail to set the Sequence to Zero in the following case:
1. Node is operating in the following state:
IV_Index = 5, IVU = 1; (Outgoing messages sent with IV_Index == 4)
2. Node powers off.
3. Node power on --> Here is the problem
Internal IV_Index still = 5,
IVU *should* still = 1, but it may have gotten lost
if we start up in IV_UPD_INIT
Outgoing messages *Should* still sent with IV_Index == 4,
but may already start sending with IV_Index == 5 if IV_UPD_INIT
4. SNB received with IV_Index = 5, IVU = 0
Outgoing messages now sent with IV_Index = 5, but Seq # never set to Zero.
I like the idea of using the change in value of the result
of mesh_net_get_iv_index(net), but I am no longer certain if
it is correct if we powered down in the IV_UPD_UPDATING
state. This is a real problem if we send any messages before
we get a SNB.
With this current patch, the danger may be low... The Seq would not be
reset to Zero if we powered off during IV Update procedure, and powered
on after it was over, but it would have fewer Sequence values to use on
the new IV_Index, and it could accumulate over time unless it is present
and powered up for a normal, complete procedure.
The problem, I think, is using the iv_upd_state (which we need to do
IV Index recovery) as a proxy for the actual state of the IVU bit.
> /* Figure out the key refresh phase */
> if (kr_transition) {
> l_debug("Beacon based KR phase change");
