Re: [tipc-discussion] [net-next v2 1/4] tipc: optimize key switching time and logic
From: Tuong Lien
Date: Mon, 31 Aug 2020 15:38:14 +0700
> We reduce the lasting time for a pending TX key to be active as well as
> for a passive RX key to be freed which generally helps speed up the key
> switching. It is not expected to be too fast but should not be too slow
> either. Also the key handling logic is simplified that a pending RX key
> will be removed automatically if it is found not working after a number
> of times; the probing for a pending TX key is now carried on a specific
> message user ('LINK_PROTOCOL' or 'LINK_CONFIG') which is more efficient
> than using a timer on broadcast messages, the timer is reserved for use
> later as needed.
>
> The kernel logs or 'pr***()' are now made as clear as possible to user.
> Some prints are added, removed or changed to the debug-level. The
> 'TIPC_CRYPTO_DEBUG' definition is removed, and the 'pr_debug()' is used
> instead which will be much helpful in runtime.
>
> Besides we also optimize the code in some other places as a preparation
> for later commits.
>
> This commit does not change the en/decryption functionalities.
>
> Acked-by: Jon Maloy
> Signed-off-by: Tuong Lien
Random log messages in response to user config requests are
inappropriate especially with netlink.
Report such informational responses to errors using the
genl_info->extack instead, as is standard practice across
the entire kernel.
Please remove all kernel log messages that get emitted due to
netlink operations and use extack notifications instead.
I also disagree with the commit message stating:
This commit does not change the en/decryption functionalities.
You are changing timer lengths and other aspects of crypto behavior,
so the patch is in fact changing things.
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
Re: [tipc-discussion] [net] tipc: fix using smp_processor_id() in preemptible
> -Original Message-
> From: Eric Dumazet
> Sent: Tuesday, September 1, 2020 8:15 PM
> To: Tuong Tong Lien ; Eric Dumazet
> ; da...@davemloft.net;
> jma...@redhat.com; ma...@donjonn.com; ying@windriver.com;
> net...@vger.kernel.org
> Cc: tipc-discussion@lists.sourceforge.net
> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
>
>
>
> On 9/1/20 5:18 AM, Tuong Tong Lien wrote:
> >
> >
> >> -Original Message-
> >> From: Eric Dumazet
> >> Sent: Monday, August 31, 2020 7:48 PM
> >> To: Tuong Tong Lien ; Eric Dumazet
> >> ; da...@davemloft.net;
> >> jma...@redhat.com; ma...@donjonn.com; ying@windriver.com;
> >> net...@vger.kernel.org
> >> Cc: tipc-discussion@lists.sourceforge.net
> >> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
> >>
> >>
> >>
> >> On 8/31/20 3:05 AM, Tuong Tong Lien wrote:
> >>>
> >>>
> -Original Message-
> From: Eric Dumazet
> Sent: Monday, August 31, 2020 4:48 PM
> To: Tuong Tong Lien ; Eric Dumazet
> ; da...@davemloft.net;
> jma...@redhat.com; ma...@donjonn.com; ying@windriver.com;
> net...@vger.kernel.org
> Cc: tipc-discussion@lists.sourceforge.net
> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
>
>
>
> On 8/31/20 1:33 AM, Tuong Tong Lien wrote:
> > Hi Eric,
> >
> > Thanks for your comments, please see my answers inline.
> >
> >> -Original Message-
> >> From: Eric Dumazet
> >> Sent: Monday, August 31, 2020 3:15 PM
> >> To: Tuong Tong Lien ;
> >> da...@davemloft.net; jma...@redhat.com; ma...@donjonn.com;
> >> ying@windriver.com; net...@vger.kernel.org
> >> Cc: tipc-discussion@lists.sourceforge.net
> >> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
> >>
> >>
> >>
> >> On 8/29/20 12:37 PM, Tuong Lien wrote:
> >>> The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the
> >>> current
> >>> CPU for encryption, however the execution can be preemptible since
> >>> it's
> >>> actually user-space context, so the 'using smp_processor_id() in
> >>> preemptible' has been observed.
> >>>
> >>> We fix the issue by using the 'get/put_cpu_ptr()' API which consists
> >>> of
> >>> a 'preempt_disable()' instead.
> >>>
> >>> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption &
> >>> authentication")
> >>
> >> Have you forgotten ' Reported-by:
> >> syzbot+263f8c0d007dc09b2...@syzkaller.appspotmail.com' ?
> > Well, really I detected the issue during my testing instead, didn't
> > know if it was reported by syzbot too.
> >
> >>
> >>> Acked-by: Jon Maloy
> >>> Signed-off-by: Tuong Lien
> >>> ---
> >>> net/tipc/crypto.c | 12 +---
> >>> 1 file changed, 9 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> >>> index c38babaa4e57..7c523dc81575 100644
> >>> --- a/net/tipc/crypto.c
> >>> +++ b/net/tipc/crypto.c
> >>> @@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp)
> >>> if (aead->cloned) {
> >>> tipc_aead_put(aead->cloned);
> >>> } else {
> >>> - head = *this_cpu_ptr(aead->tfm_entry);
> >>> + head = *get_cpu_ptr(aead->tfm_entry);
> >>> + put_cpu_ptr(aead->tfm_entry);
> >>
> >> Why is this safe ?
> >>
> >> I think that this very unusual construct needs a comment, because this
> >> is not obvious.
> >>
> >> This really looks like an attempt to silence syzbot to me.
> > No, this is not to silence syzbot but really safe.
> > This is because the "aead->tfm_entry" object is "common" between CPUs,
> > there is only its pointer to be the "per_cpu" one. So
> >> just
> trying to lock the process on the current CPU or 'preempt_disable()',
> taking the per-cpu pointer and dereferencing to the actual
> "tfm_entry" object... is enough. Later on, that’s fine to play with the
> actual object without any locking.
>
> Why using per cpu pointers, if they all point to a common object ?
>
> This makes the code really confusing.
> >>> Sorry for making you confused. Yes, the code is a bit ugly and could be
> >>> made in some other ways... The initial idea is to not touch
> or
> >> change the same pointer variable in different CPUs so avoid a penalty with
> >> the cache hits/misses...
> >>
> >> What makes this code interrupt safe ?
> >>
> > Why is it unsafe? Its "parent" object is already managed by RCU mechanism.
> > Also, it is never modified but just "read-only" in all
> cases...
>
> tipc_aead_tfm_next() is _not_ read-only, since it contains :
>
> *tfm_entry = list_next_entry(*tfm_entry, list);
>
> If tipc_aead_tfm_next() can be called both from process context and irq
>
Re: [tipc-discussion] [PATCH] tipc: fix use-after-free in tipc_bcast_get_mode
On 8/25/20 11:52 PM, Hoang Huu Le wrote:
Syzbot has reported those issues as:
==
BUG: KASAN: use-after-free in tipc_bcast_get_mode+0x3ab/0x400
net/tipc/bcast.c:759
Read of size 1 at addr 88805e6b3571 by task kworker/0:6/3850
CPU: 0 PID: 3850 Comm: kworker/0:6 Not tainted 5.8.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
01/01/2011
Workqueue: events tipc_net_finalize_work
Thread 1's call trace:
[...]
kfree+0x103/0x2c0 mm/slab.c:3757 <- bcbase releasing
tipc_bcast_stop+0x1b0/0x2f0 net/tipc/bcast.c:721
tipc_exit_net+0x24/0x270 net/tipc/core.c:112
[...]
Thread 2's call trace:
[...]
tipc_bcast_get_mode+0x3ab/0x400 net/tipc/bcast.c:759 <- bcbase
has already been freed by Thread 1
tipc_node_broadcast+0x9e/0xcc0 net/tipc/node.c:1744
tipc_nametbl_publish+0x60b/0x970 net/tipc/name_table.c:752
tipc_net_finalize net/tipc/net.c:141 [inline]
tipc_net_finalize+0x1fa/0x310 net/tipc/net.c:131
tipc_net_finalize_work+0x55/0x80 net/tipc/net.c:150
[...]
==
BUG: KASAN: use-after-free in tipc_named_reinit+0xef/0x290
net/tipc/name_distr.c:344
Read of size 8 at addr 888052ab2000 by task kworker/0:13/30628
CPU: 0 PID: 30628 Comm: kworker/0:13 Not tainted 5.8.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
01/01/2011
Workqueue: events tipc_net_finalize_work
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1f0/0x31e lib/dump_stack.c:118
print_address_description+0x66/0x5a0 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report+0x132/0x1d0 mm/kasan/report.c:530
tipc_named_reinit+0xef/0x290 net/tipc/name_distr.c:344
tipc_net_finalize+0x85/0xe0 net/tipc/net.c:138
tipc_net_finalize_work+0x50/0x70 net/tipc/net.c:150
process_one_work+0x789/0xfc0 kernel/workqueue.c:2269
worker_thread+0xaa4/0x1460 kernel/workqueue.c:2415
kthread+0x37e/0x3a0 drivers/block/aoe/aoecmd.c:1234
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:293
[...]
Freed by task 14058:
save_stack mm/kasan/common.c:48 [inline]
set_track mm/kasan/common.c:56 [inline]
kasan_set_free_info mm/kasan/common.c:316 [inline]
__kasan_slab_free+0x114/0x170 mm/kasan/common.c:455
__cache_free mm/slab.c:3426 [inline]
kfree+0x10a/0x220 mm/slab.c:3757
tipc_exit_net+0x29/0x50 net/tipc/core.c:113
ops_exit_list net/core/net_namespace.c:186 [inline]
cleanup_net+0x708/0xba0 net/core/net_namespace.c:603
process_one_work+0x789/0xfc0 kernel/workqueue.c:2269
worker_thread+0xaa4/0x1460 kernel/workqueue.c:2415
kthread+0x37e/0x3a0 drivers/block/aoe/aoecmd.c:1234
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:293
Solution 1 (preferer):
Fix it by calling flush_scheduled_work() to make sure the
tipc_net_finalize_work() stopped before releasing bcbase object.
Solution 2:
Fix it by introducing a bit flag and returning if flag is zero
(object had already been freed)
Reported-by: syzbot+6ea1f7a8df64596ef...@syzkaller.appspotmail.com
Reported-by: syzbot+e9cc557752ab126c1...@syzkaller.appspotmail.com
Signed-off-by: Hoang Huu Le
---
net/tipc/bcast.c | 1 +
net/tipc/core.c | 1 +
net/tipc/core.h | 1 +
net/tipc/net.c | 3 +++
4 files changed, 6 insertions(+)
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 940d176e0e87..56b624c8b6d4 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -718,6 +718,7 @@ void tipc_bcast_stop(struct net *net)
struct tipc_net *tn = net_generic(net, tipc_net_id);
synchronize_net();
+ clear_bit_unlock(0, >net_exit_flag);
kfree(tn->bcbase);
kfree(tn->bcl);
}
diff --git a/net/tipc/core.c b/net/tipc/core.c
index 4f6dc74adf45..93ea7dc05bf2 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -60,6 +60,7 @@ static int __net_init tipc_init_net(struct net *net)
tn->trial_addr = 0;
tn->addr_trial_end = 0;
tn->capabilities = TIPC_NODE_CAPABILITIES;
+ test_and_set_bit_lock(0, >net_exit_flag);
memset(tn->node_id, 0, sizeof(tn->node_id));
memset(tn->node_id_string, 0, sizeof(tn->node_id_string));
tn->mon_threshold = TIPC_DEF_MON_THRESHOLD;
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 631d83c9705f..aa75882dd932 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -143,6 +143,7 @@ struct tipc_net {
/* TX crypto handler */
struct tipc_crypto *crypto_tx;
#endif
+ unsigned long net_exit_flag;
};
static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 85400e4242de..0dcbfcff5ad3 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -132,6 +132,9 @@ static void tipc_net_finalize(struct net *net, u32 addr)
{
struct tipc_net *tn = tipc_net(net);
+ if (unlikely(!test_bit(0, >net_exit_flag)))
+ return;
+
Re: [tipc-discussion] [net] tipc: fix using smp_processor_id() in preemptible
> -Original Message-
> From: Eric Dumazet
> Sent: Monday, August 31, 2020 7:48 PM
> To: Tuong Tong Lien ; Eric Dumazet
> ; da...@davemloft.net;
> jma...@redhat.com; ma...@donjonn.com; ying@windriver.com;
> net...@vger.kernel.org
> Cc: tipc-discussion@lists.sourceforge.net
> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
>
>
>
> On 8/31/20 3:05 AM, Tuong Tong Lien wrote:
> >
> >
> >> -Original Message-
> >> From: Eric Dumazet
> >> Sent: Monday, August 31, 2020 4:48 PM
> >> To: Tuong Tong Lien ; Eric Dumazet
> >> ; da...@davemloft.net;
> >> jma...@redhat.com; ma...@donjonn.com; ying@windriver.com;
> >> net...@vger.kernel.org
> >> Cc: tipc-discussion@lists.sourceforge.net
> >> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
> >>
> >>
> >>
> >> On 8/31/20 1:33 AM, Tuong Tong Lien wrote:
> >>> Hi Eric,
> >>>
> >>> Thanks for your comments, please see my answers inline.
> >>>
> -Original Message-
> From: Eric Dumazet
> Sent: Monday, August 31, 2020 3:15 PM
> To: Tuong Tong Lien ; da...@davemloft.net;
> jma...@redhat.com; ma...@donjonn.com;
> ying@windriver.com; net...@vger.kernel.org
> Cc: tipc-discussion@lists.sourceforge.net
> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
>
>
>
> On 8/29/20 12:37 PM, Tuong Lien wrote:
> > The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the current
> > CPU for encryption, however the execution can be preemptible since it's
> > actually user-space context, so the 'using smp_processor_id() in
> > preemptible' has been observed.
> >
> > We fix the issue by using the 'get/put_cpu_ptr()' API which consists of
> > a 'preempt_disable()' instead.
> >
> > Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
>
> Have you forgotten ' Reported-by:
> syzbot+263f8c0d007dc09b2...@syzkaller.appspotmail.com' ?
> >>> Well, really I detected the issue during my testing instead, didn't know
> >>> if it was reported by syzbot too.
> >>>
>
> > Acked-by: Jon Maloy
> > Signed-off-by: Tuong Lien
> > ---
> > net/tipc/crypto.c | 12 +---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> > index c38babaa4e57..7c523dc81575 100644
> > --- a/net/tipc/crypto.c
> > +++ b/net/tipc/crypto.c
> > @@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp)
> > if (aead->cloned) {
> > tipc_aead_put(aead->cloned);
> > } else {
> > - head = *this_cpu_ptr(aead->tfm_entry);
> > + head = *get_cpu_ptr(aead->tfm_entry);
> > + put_cpu_ptr(aead->tfm_entry);
>
> Why is this safe ?
>
> I think that this very unusual construct needs a comment, because this
> is not obvious.
>
> This really looks like an attempt to silence syzbot to me.
> >>> No, this is not to silence syzbot but really safe.
> >>> This is because the "aead->tfm_entry" object is "common" between CPUs,
> >>> there is only its pointer to be the "per_cpu" one. So
> just
> >> trying to lock the process on the current CPU or 'preempt_disable()',
> >> taking the per-cpu pointer and dereferencing to the actual
> >> "tfm_entry" object... is enough. Later on, that’s fine to play with the
> >> actual object without any locking.
> >>
> >> Why using per cpu pointers, if they all point to a common object ?
> >>
> >> This makes the code really confusing.
> > Sorry for making you confused. Yes, the code is a bit ugly and could be
> > made in some other ways... The initial idea is to not touch or
> change the same pointer variable in different CPUs so avoid a penalty with
> the cache hits/misses...
>
> What makes this code interrupt safe ?
>
Why is it unsafe? Its "parent" object is already managed by RCU mechanism.
Also, it is never modified but just "read-only" in all cases...
BR/Tuong
> Having a per-cpu list is not interrupt safe without special care.
>
>
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
[tipc-discussion] [net-next v2 1/4] tipc: optimize key switching time and logic
We reduce the lasting time for a pending TX key to be active as well as
for a passive RX key to be freed which generally helps speed up the key
switching. It is not expected to be too fast but should not be too slow
either. Also the key handling logic is simplified that a pending RX key
will be removed automatically if it is found not working after a number
of times; the probing for a pending TX key is now carried on a specific
message user ('LINK_PROTOCOL' or 'LINK_CONFIG') which is more efficient
than using a timer on broadcast messages, the timer is reserved for use
later as needed.
The kernel logs or 'pr***()' are now made as clear as possible to user.
Some prints are added, removed or changed to the debug-level. The
'TIPC_CRYPTO_DEBUG' definition is removed, and the 'pr_debug()' is used
instead which will be much helpful in runtime.
Besides we also optimize the code in some other places as a preparation
for later commits.
This commit does not change the en/decryption functionalities.
Acked-by: Jon Maloy
Signed-off-by: Tuong Lien
---
net/tipc/crypto.c | 344 +++---
1 file changed, 141 insertions(+), 203 deletions(-)
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 7c523dc81575..53a3b34b3913 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -38,10 +38,10 @@
#include
#include "crypto.h"
-#define TIPC_TX_PROBE_LIM msecs_to_jiffies(1000) /* > 1s */
-#define TIPC_TX_LASTING_LIMmsecs_to_jiffies(12) /* 2 mins */
+#define TIPC_TX_LASTING_TIME msecs_to_jiffies(1) /* 10s */
#define TIPC_RX_ACTIVE_LIM msecs_to_jiffies(3000) /* 3s */
-#define TIPC_RX_PASSIVE_LIMmsecs_to_jiffies(18) /* 3 mins */
+#define TIPC_RX_PASSIVE_LIMmsecs_to_jiffies(15000) /* 15s */
+
#define TIPC_MAX_TFMS_DEF 10
#define TIPC_MAX_TFMS_LIM 1000
@@ -144,7 +144,7 @@ struct tipc_aead {
u32 salt;
u8 authsize;
u8 mode;
- char hint[TIPC_AEAD_HINT_LEN + 1];
+ char hint[2 * TIPC_AEAD_HINT_LEN + 1];
struct rcu_head rcu;
atomic64_t seqno cacheline_aligned;
@@ -168,9 +168,10 @@ struct tipc_crypto_stats {
* @key: the key states
* @working: the crypto is working or not
* @stats: the crypto statistics
+ * @name: the crypto name
* @sndnxt: the per-peer sndnxt (TX)
* @timer1: general timer 1 (jiffies)
- * @timer2: general timer 1 (jiffies)
+ * @timer2: general timer 2 (jiffies)
* @lock: tipc_key lock
*/
struct tipc_crypto {
@@ -181,6 +182,7 @@ struct tipc_crypto {
struct tipc_key key;
u8 working:1;
struct tipc_crypto_stats __percpu *stats;
+ char name[48];
atomic64_t sndnxt cacheline_aligned;
unsigned long timer1;
@@ -239,18 +241,17 @@ static bool tipc_crypto_key_try_align(struct tipc_crypto
*rx, u8 new_pending);
static struct tipc_aead *tipc_crypto_key_pick_tx(struct tipc_crypto *tx,
struct tipc_crypto *rx,
struct sk_buff *skb);
-static void tipc_crypto_key_synch(struct tipc_crypto *rx, u8 new_rx_active,
- struct tipc_msg *hdr);
+static void tipc_crypto_key_synch(struct tipc_crypto *rx, struct sk_buff *skb);
static int tipc_crypto_key_revoke(struct net *net, u8 tx_key);
static void tipc_crypto_rcv_complete(struct net *net, struct tipc_aead *aead,
struct tipc_bearer *b,
struct sk_buff **skb, int err);
static void tipc_crypto_do_cmd(struct net *net, int cmd);
static char *tipc_crypto_key_dump(struct tipc_crypto *c, char *buf);
-#ifdef TIPC_CRYPTO_DEBUG
static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key new,
char *buf);
-#endif
+#define is_tx(crypto) (!(crypto)->node)
+#define is_rx(crypto) (!is_tx(crypto))
#define key_next(cur) ((cur) % KEY_MAX + 1)
@@ -290,7 +291,7 @@ int tipc_aead_key_validate(struct tipc_aead_key *ukey)
if (unlikely(keylen != TIPC_AES_GCM_KEY_SIZE_128 &&
keylen != TIPC_AES_GCM_KEY_SIZE_192 &&
keylen != TIPC_AES_GCM_KEY_SIZE_256))
- return -EINVAL;
+ return -EKEYREJECTED;
return 0;
}
@@ -501,9 +502,9 @@ static int tipc_aead_init(struct tipc_aead **aead, struct
tipc_aead_key *ukey,
return err;
}
- /* Copy some chars from the user key as a hint */
- memcpy(tmp->hint, ukey->key, TIPC_AEAD_HINT_LEN);
- tmp->hint[TIPC_AEAD_HINT_LEN] = '\0';
+ /* Form a hex string of some last bytes as the key's hint */
+ bin2hex(tmp->hint, ukey->key + keylen - TIPC_AEAD_HINT_LEN,
+ TIPC_AEAD_HINT_LEN);
/* Initialize the other data */
tmp->mode = mode;
@@ -663,13 +664,11 @@ static int tipc_aead_encrypt(struct tipc_aead *aead,
struct sk_buff *skb,
* but there is no
[tipc-discussion] [net-next v2 0/4] tipc: add more features to TIPC encryption
This series adds some new features to TIPC encryption:
- Patch 1 ("tipc: optimize key switching time and logic") optimizes the
code and logic in preparation for the following commits.
- Patch 2 ("tipc: introduce encryption master key") introduces support
of 'master key' for authentication of new nodes and key exchange. A
master key can be set/changed by user via netlink (eg. using the same
'tipc node set key' command in iproute2/tipc).
- Patch 3 ("tipc: add automatic session key exchange") allows a session
key to be securely exchanged between nodes as needed.
- Patch 4 ("tipc: add automatic rekeying for encryption key") adds
automatic 'rekeying' of session keys a specific interval. The new key
will be distributed automatically to peer nodes, so become active then.
The rekeying interval is configurable via netlink as well.
v2: update the "tipc: add automatic session key exchange" patch to fix
"implicit declaration" issue when built without "CONFIG_TIPC_CRYPTO".
Tuong Lien (4):
tipc: optimize key switching time and logic
tipc: introduce encryption master key
tipc: add automatic session key exchange
tipc: add automatic rekeying for encryption key
include/uapi/linux/tipc.h | 2 +
include/uapi/linux/tipc_netlink.h | 2 +
net/tipc/crypto.c | 974 ++
net/tipc/crypto.h | 41 +-
net/tipc/link.c | 5 +
net/tipc/msg.h| 8 +-
net/tipc/netlink.c| 2 +
net/tipc/node.c | 91 ++-
net/tipc/node.h | 2 +
net/tipc/sysctl.c | 9 +
10 files changed, 853 insertions(+), 283 deletions(-)
--
2.26.2
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
[tipc-discussion] [net-next v2 2/4] tipc: introduce encryption master key
In addition to the supported cluster & per-node encryption keys for the
en/decryption of TIPC messages, we now introduce one option for user to
set a cluster key as 'master key', which is simply a symmetric key like
the former but has a longer life cycle. It has two purposes:
- Authentication of new member nodes in the cluster. New nodes, having
no knowledge of current session keys in the cluster will still be
able to join the cluster as long as they know the master key. This is
because all neighbor discovery (LINK_CONFIG) messages must be
encrypted with this key.
- Encryption of session encryption keys during automatic exchange and
update of those.This is a feature we will introduce in a later commit
in this series.
We insert the new key into the currently unused slot 0 in the key array
and start using it immediately once the user has set it.
After joining, a node only knowing the master key should be fully
communicable to existing nodes in the cluster, although those nodes may
have their own session keys activated (i.e. not the master one). To
support this, we define a 'grace period', starting from the time a node
itself reports having no RX keys, so the existing nodes will use the
master key for encryption instead. The grace period can be extended but
will automatically stop after e.g. 5 seconds without a new report. This
is also the basis for later key exchanging feature as the new node will
be impossible to decrypt anything without the support from master key.
For user to set a master key, we define a new netlink flag -
'TIPC_NLA_NODE_KEY_MASTER', so it can be added to the current 'set key'
netlink command to specify the setting key to be a master key.
Above all, the traditional cluster/per-node key mechanism is guaranteed
to work when user comes not to use this master key option. This is also
compatible to legacy nodes without the feature supported.
Even this master key can be updated without any interruption of cluster
connectivity but is so is needed, this has to be coordinated and set by
the user.
Acked-by: Jon Maloy
Signed-off-by: Tuong Lien
---
include/uapi/linux/tipc_netlink.h | 1 +
net/tipc/crypto.c | 206 ++
net/tipc/crypto.h | 15 ++-
net/tipc/msg.h| 4 +-
net/tipc/netlink.c| 1 +
net/tipc/node.c | 46 +++
6 files changed, 189 insertions(+), 84 deletions(-)
diff --git a/include/uapi/linux/tipc_netlink.h
b/include/uapi/linux/tipc_netlink.h
index dc0d23a50e69..d484baa9d365 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -165,6 +165,7 @@ enum {
TIPC_NLA_NODE_UP, /* flag */
TIPC_NLA_NODE_ID, /* data */
TIPC_NLA_NODE_KEY, /* data */
+ TIPC_NLA_NODE_KEY_MASTER, /* flag */
__TIPC_NLA_NODE_MAX,
TIPC_NLA_NODE_MAX = __TIPC_NLA_NODE_MAX - 1
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 53a3b34b3913..b75b817441e5 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -38,6 +38,7 @@
#include
#include "crypto.h"
+#define TIPC_TX_GRACE_PERIOD msecs_to_jiffies(5000) /* 5s */
#define TIPC_TX_LASTING_TIME msecs_to_jiffies(1) /* 10s */
#define TIPC_RX_ACTIVE_LIM msecs_to_jiffies(3000) /* 3s */
#define TIPC_RX_PASSIVE_LIMmsecs_to_jiffies(15000) /* 15s */
@@ -49,9 +50,9 @@
* TIPC Key ids
*/
enum {
- KEY_UNUSED = 0,
- KEY_MIN,
- KEY_1 = KEY_MIN,
+ KEY_MASTER = 0,
+ KEY_MIN = KEY_MASTER,
+ KEY_1 = 1,
KEY_2,
KEY_3,
KEY_MAX = KEY_3,
@@ -166,27 +167,36 @@ struct tipc_crypto_stats {
* @aead: array of pointers to AEAD keys for encryption/decryption
* @peer_rx_active: replicated peer RX active key index
* @key: the key states
- * @working: the crypto is working or not
* @stats: the crypto statistics
* @name: the crypto name
* @sndnxt: the per-peer sndnxt (TX)
* @timer1: general timer 1 (jiffies)
* @timer2: general timer 2 (jiffies)
+ * @working: the crypto is working or not
+ * @key_master: flag indicates if master key exists
+ * @legacy_user: flag indicates if a peer joins w/o master key (for bwd comp.)
* @lock: tipc_key lock
*/
struct tipc_crypto {
struct net *net;
struct tipc_node *node;
- struct tipc_aead __rcu *aead[KEY_MAX + 1]; /* key[0] is UNUSED */
+ struct tipc_aead __rcu *aead[KEY_MAX + 1];
atomic_t peer_rx_active;
struct tipc_key key;
- u8 working:1;
struct tipc_crypto_stats __percpu *stats;
char name[48];
atomic64_t sndnxt cacheline_aligned;
unsigned long timer1;
unsigned long timer2;
+ union {
+ struct {
+ u8 working:1;
+ u8 key_master:1;
+ u8 legacy_user:1;
+ };
+ u8 flags;
+ };
[tipc-discussion] [net-next v2 3/4] tipc: add automatic session key exchange
With support from the master key option in the previous commit, it
becomes easy to make frequent updates/exchanges of session keys between
authenticated cluster nodes.
Basically, there are two situations where the key exchange will take in
place:
- When a new node joins the cluster (with the master key), it will need
to get its peer's TX key, so that be able to decrypt further messages
from that peer.
- When a new session key is generated (by either user manual setting or
later automatic rekeying feature), the key will be distributed to all
peer nodes in the cluster.
A key to be exchanged is encapsulated in the data part of a 'MSG_CRYPTO
/KEY_DISTR_MSG' TIPC v2 message, then xmit-ed as usual and encrypted by
using the master key before sending out. Upon receipt of the message it
will be decrypted in the same way as regular messages, then attached as
the sender's RX key in the receiver node.
In this way, the key exchange is reliable by the link layer, as well as
security, integrity and authenticity by the crypto layer.
Also, the forward security will be easily achieved by user changing the
master key actively but this should not be required very frequently.
The key exchange feature is independent on the presence of a master key
Note however that the master key still is needed for new nodes to be
able to join the cluster. It is also optional, and can be turned off/on
via the sysfs: 'net/tipc/key_exchange_enabled' [default 1: enabled].
Backward compatibility is guaranteed because for nodes that do not have
master key support, key exchange using master key ie. tx_key = 0 if any
will be shortly discarded at the message validation step. In other
words, the key exchange feature will be automatically disabled to those
nodes.
v2: fix the "implicit declaration of function 'tipc_crypto_key_flush'"
error in node.c. The function only exists when built with the TIPC
"CONFIG_TIPC_CRYPTO" option.
Reported-by: kernel test robot
Acked-by: Jon Maloy
Signed-off-by: Tuong Lien
---
net/tipc/crypto.c | 359 +++---
net/tipc/crypto.h | 24
net/tipc/link.c | 5 +
net/tipc/msg.h| 4 +
net/tipc/node.c | 21 ++-
net/tipc/node.h | 2 +
net/tipc/sysctl.c | 9 ++
7 files changed, 404 insertions(+), 20 deletions(-)
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index b75b817441e5..d29266a9d2ee 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -37,6 +37,8 @@
#include
#include
#include "crypto.h"
+#include "msg.h"
+#include "bcast.h"
#define TIPC_TX_GRACE_PERIOD msecs_to_jiffies(5000) /* 5s */
#define TIPC_TX_LASTING_TIME msecs_to_jiffies(1) /* 10s */
@@ -82,6 +84,8 @@ static const char *hstats[MAX_STATS] = {"ok", "nok", "async",
"async_ok",
/* Max TFMs number per key */
int sysctl_tipc_max_tfms __read_mostly = TIPC_MAX_TFMS_DEF;
+/* Key exchange switch, default: on */
+int sysctl_tipc_key_exchange_enabled __read_mostly = 1;
/**
* struct tipc_key - TIPC keys' status indicator
@@ -133,6 +137,8 @@ struct tipc_tfm {
* @mode: crypto mode is applied to the key
* @hint[]: a hint for user key
* @rcu: struct rcu_head
+ * @key: the aead key
+ * @gen: the key's generation
* @seqno: the key seqno (cluster scope)
* @refcnt: the key reference counter
*/
@@ -147,6 +153,8 @@ struct tipc_aead {
u8 mode;
char hint[2 * TIPC_AEAD_HINT_LEN + 1];
struct rcu_head rcu;
+ struct tipc_aead_key *key;
+ u16 gen;
atomic64_t seqno cacheline_aligned;
refcount_t refcnt cacheline_aligned;
@@ -166,7 +174,13 @@ struct tipc_crypto_stats {
* @node: TIPC node (RX)
* @aead: array of pointers to AEAD keys for encryption/decryption
* @peer_rx_active: replicated peer RX active key index
+ * @key_gen: TX/RX key generation
* @key: the key states
+ * @skey_mode: session key's mode
+ * @skey: received session key
+ * @wq: common workqueue on TX crypto
+ * @work: delayed work sched for TX/RX
+ * @key_distr: key distributing state
* @stats: the crypto statistics
* @name: the crypto name
* @sndnxt: the per-peer sndnxt (TX)
@@ -175,6 +189,7 @@ struct tipc_crypto_stats {
* @working: the crypto is working or not
* @key_master: flag indicates if master key exists
* @legacy_user: flag indicates if a peer joins w/o master key (for bwd comp.)
+ * @nokey: no key indication
* @lock: tipc_key lock
*/
struct tipc_crypto {
@@ -182,7 +197,16 @@ struct tipc_crypto {
struct tipc_node *node;
struct tipc_aead __rcu *aead[KEY_MAX + 1];
atomic_t peer_rx_active;
+ u16 key_gen;
struct tipc_key key;
+ u8 skey_mode;
+ struct tipc_aead_key *skey;
+ struct workqueue_struct *wq;
+ struct delayed_work work;
+#define KEY_DISTR_SCHED1
+#define KEY_DISTR_COMPL2
+ atomic_t key_distr;
+
struct tipc_crypto_stats __percpu *stats;
char name[48];
@@ -194,6 +218,7 @@ struct tipc_crypto
[tipc-discussion] [net-next v2 4/4] tipc: add automatic rekeying for encryption key
Rekeying is required for security since a key is less secure when using
for a long time. Also, key will be detached when its nonce value (or
seqno ...) is exhausted. We now make the rekeying process automatic and
configurable by user.
Basically, TIPC will at a specific interval generate a new key by using
the kernel 'Random Number Generator' cipher, then attach it as the node
TX key and securely distribute to others in the cluster as RX keys (-
the key exchange). The automatic key switching will then take over, and
make the new key active shortly. Afterwards, the traffic from this node
will be encrypted with the new session key. The same can happen in peer
nodes but not necessarily at the same time.
For simplicity, the automatically generated key will be initiated as a
per node key. It is not too hard to also support a cluster key rekeying
(e.g. a given node will generate a unique cluster key and update to the
others in the cluster...), but that doesn't bring much benefit, while a
per-node key is even more secure.
We also enable user to force a rekeying or change the rekeying interval
via netlink, the new 'set key' command option: 'TIPC_NLA_NODE_REKEYING'
is added for these purposes as follows:
- A value >= 1 will be set as the rekeying interval (in minutes);
- A value of 0 will disable the rekeying;
- A value of 'TIPC_REKEYING_NOW' (~0) will force an immediate rekeying;
The default rekeying interval is (60 * 24) minutes i.e. done every day.
There isn't any restriction for the value but user shouldn't set it too
small or too large which results in an "ineffective" rekeying (thats ok
for testing though).
Acked-by: Jon Maloy
Signed-off-by: Tuong Lien
---
include/uapi/linux/tipc.h | 2 +
include/uapi/linux/tipc_netlink.h | 1 +
net/tipc/crypto.c | 115 +-
net/tipc/crypto.h | 2 +
net/tipc/netlink.c| 1 +
net/tipc/node.c | 28 +++-
6 files changed, 146 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/tipc.h b/include/uapi/linux/tipc.h
index add01db1daef..80ea15e12113 100644
--- a/include/uapi/linux/tipc.h
+++ b/include/uapi/linux/tipc.h
@@ -254,6 +254,8 @@ static inline int tipc_aead_key_size(struct tipc_aead_key
*key)
return sizeof(*key) + key->keylen;
}
+#define TIPC_REKEYING_NOW (~0U)
+
/* The macros and functions below are deprecated:
*/
diff --git a/include/uapi/linux/tipc_netlink.h
b/include/uapi/linux/tipc_netlink.h
index d484baa9d365..d847dd671d79 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -166,6 +166,7 @@ enum {
TIPC_NLA_NODE_ID, /* data */
TIPC_NLA_NODE_KEY, /* data */
TIPC_NLA_NODE_KEY_MASTER, /* flag */
+ TIPC_NLA_NODE_REKEYING, /* u32 */
__TIPC_NLA_NODE_MAX,
TIPC_NLA_NODE_MAX = __TIPC_NLA_NODE_MAX - 1
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index d29266a9d2ee..9d4ad832572f 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -36,6 +36,7 @@
#include
#include
+#include
#include "crypto.h"
#include "msg.h"
#include "bcast.h"
@@ -48,6 +49,8 @@
#define TIPC_MAX_TFMS_DEF 10
#define TIPC_MAX_TFMS_LIM 1000
+#define TIPC_REKEYING_INTV_DEF (60 * 24) /* default: 1 day */
+
/**
* TIPC Key ids
*/
@@ -181,6 +184,7 @@ struct tipc_crypto_stats {
* @wq: common workqueue on TX crypto
* @work: delayed work sched for TX/RX
* @key_distr: key distributing state
+ * @rekeying_intv: rekeying interval (in minutes)
* @stats: the crypto statistics
* @name: the crypto name
* @sndnxt: the per-peer sndnxt (TX)
@@ -206,6 +210,7 @@ struct tipc_crypto {
#define KEY_DISTR_SCHED1
#define KEY_DISTR_COMPL2
atomic_t key_distr;
+ u32 rekeying_intv;
struct tipc_crypto_stats __percpu *stats;
char name[48];
@@ -294,7 +299,9 @@ static char *tipc_key_change_dump(struct tipc_key old,
struct tipc_key new,
static int tipc_crypto_key_xmit(struct net *net, struct tipc_aead_key *skey,
u16 gen, u8 mode, u32 dnode);
static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr);
+static void tipc_crypto_work_tx(struct work_struct *work);
static void tipc_crypto_work_rx(struct work_struct *work);
+static int tipc_aead_key_generate(struct tipc_aead_key *skey);
#define is_tx(crypto) (!(crypto)->node)
#define is_rx(crypto) (!is_tx(crypto))
@@ -342,6 +349,27 @@ int tipc_aead_key_validate(struct tipc_aead_key *ukey)
return 0;
}
+/**
+ * tipc_aead_key_generate - Generate new session key
+ * @skey: input/output key with new content
+ *
+ * Return: 0 in case of success, otherwise < 0
+ */
+static int tipc_aead_key_generate(struct tipc_aead_key *skey)
+{
+ int rc = 0;
+
+ /* Fill the key's content with a random value via RNG cipher */
+ rc =
