[PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[Michał Lowas-Rzechonek]

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);
+
 		/* Figure out the key refresh phase */
 		if (kr_transition) {
 			l_debug("Beacon based KR phase change");
-- 
2.22.0

Re: [PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[Michał Lowas-Rzechonek]

Hi Brian,

On 09/09, Michał Lowas-Rzechonek wrote:
> This patch ensures that Sequence Number is reset only when IV Index used
> for outgoing messages increases.

Did you have a chance to take a look at this?

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[Michał Lowas-Rzechonek]

Hi Brian,

On 09/17, Michał Lowas-Rzechonek wrote:
> On 09/09, Michał Lowas-Rzechonek wrote:
> > This patch ensures that Sequence Number is reset only when IV Index used
> > for outgoing messages increases.
> 
> Did you have a chance to take a look at this?

Ping?

-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND

Re: [PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[Gix, Brian]

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");

Re: [PATCH BlueZ v2] mesh: Fix IV Recovery procedure when IV Update is in progress

[michal.lowas-rzechonek]

Hi Brian,

On 10/01, Gix, Brian wrote:
> 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.

Agreed. The IV Recovery flag should be tracked separately from IV
Update state.

FYI:

We ended up implementing two separate state machines to process IV
Update procedure: one for 'not synced' and one for 'synced' operation.

Node starts in 'not synced' machine and moves to 'synced' when it
receives its first SNB. The move is one-way, so it's not possible to
move from 'synced' back to 'not synced' without resetting the node.

Both machines are fairly simple and nearly identical... but moving from
'not synced' to 'synced' is far from trivial.

In the end, the whole machinery is rather complex. Reflecting this
approach in BlueZ would require quite an invasive patchset, essentially
reworking the whole IV Update logic.

I'm going to try to document the whole solution and get back to you.

regards
-- 
Michał Lowas-Rzechonek <michal.lowas-rzechonek@silvair.com>
Silvair http://silvair.com
Jasnogórska 44, 31-358 Krakow, POLAND