From 022e9d6090599c0593c78e87dc9ba98a290e6bc4 Mon Sep 17 00:00:00 2001 From: Taehee Yoo Date: Thu, 9 Apr 2020 14:08:08 +0000 Subject: net: macsec: fix using wrong structure in macsec_changelink() In the macsec_changelink(), "struct macsec_tx_sa tx_sc" is used to store "macsec_secy.tx_sc". But, the struct type of tx_sc is macsec_tx_sc, not macsec_tx_sa. So, the macsec_tx_sc should be used instead. Test commands: ip link add dummy0 type dummy ip link add macsec0 link dummy0 type macsec ip link set macsec0 type macsec encrypt off Splat looks like: [61119.963483][ T9335] ================================================================== [61119.964709][ T9335] BUG: KASAN: slab-out-of-bounds in macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.965787][ T9335] Read of size 160 at addr ffff888020d69c68 by task ip/9335 [61119.966699][ T9335] [61119.966979][ T9335] CPU: 0 PID: 9335 Comm: ip Not tainted 5.6.0+ #503 [61119.967791][ T9335] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [61119.968914][ T9335] Call Trace: [61119.969324][ T9335] dump_stack+0x96/0xdb [61119.969809][ T9335] ? macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.970554][ T9335] print_address_description.constprop.5+0x1be/0x360 [61119.971294][ T9335] ? macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.971973][ T9335] ? macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.972703][ T9335] __kasan_report+0x12a/0x170 [61119.973323][ T9335] ? macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.973942][ T9335] kasan_report+0xe/0x20 [61119.974397][ T9335] check_memory_region+0x149/0x1a0 [61119.974866][ T9335] memcpy+0x1f/0x50 [61119.975209][ T9335] macsec_changelink.part.34+0xb6/0x200 [macsec] [61119.975825][ T9335] ? macsec_get_stats64+0x3e0/0x3e0 [macsec] [61119.976451][ T9335] ? kernel_text_address+0x111/0x120 [61119.976990][ T9335] ? pskb_expand_head+0x25f/0xe10 [61119.977503][ T9335] ? stack_trace_save+0x82/0xb0 [61119.977986][ T9335] ? memset+0x1f/0x40 [61119.978397][ T9335] ? __nla_validate_parse+0x98/0x1ab0 [61119.978936][ T9335] ? macsec_alloc_tfm+0x90/0x90 [macsec] [61119.979511][ T9335] ? __kasan_slab_free+0x111/0x150 [61119.980021][ T9335] ? kfree+0xce/0x2f0 [61119.980700][ T9335] ? netlink_trim+0x196/0x1f0 [61119.981420][ T9335] ? nla_memcpy+0x90/0x90 [61119.982036][ T9335] ? register_lock_class+0x19e0/0x19e0 [61119.982776][ T9335] ? memcpy+0x34/0x50 [61119.983327][ T9335] __rtnl_newlink+0x922/0x1270 [ ... ] Fixes: 3cf3227a21d1 ("net: macsec: hardware offloading infrastructure") Signed-off-by: Taehee Yoo Signed-off-by: David S. Miller --- drivers/net/macsec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/net/macsec.c') diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 0d580d81d910..a183250ff66a 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -3809,7 +3809,7 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], struct netlink_ext_ack *extack) { struct macsec_dev *macsec = macsec_priv(dev); - struct macsec_tx_sa tx_sc; + struct macsec_tx_sc tx_sc; struct macsec_secy secy; int ret; -- cgit v1.2.3 From 7f327080364abccf923fa5a5b24e038eb0ba1407 Mon Sep 17 00:00:00 2001 From: Taehee Yoo Date: Thu, 23 Apr 2020 13:40:47 +0000 Subject: macsec: avoid to set wrong mtu When a macsec interface is created, the mtu is calculated with the lower interface's mtu value. If the mtu of lower interface is lower than the length, which is needed by macsec interface, macsec's mtu value will be overflowed. So, if the lower interface's mtu is too low, macsec interface's mtu should be set to 0. Test commands: ip link add dummy0 mtu 10 type dummy ip link add macsec0 link dummy0 type macsec ip link show macsec0 Before: 11: macsec0@dummy0: mtu 4294967274 After: 11: macsec0@dummy0: mtu 0 Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Signed-off-by: Taehee Yoo Signed-off-by: David S. Miller --- drivers/net/macsec.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'drivers/net/macsec.c') diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index a183250ff66a..758baf7cb8a1 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -4002,11 +4002,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev, struct netlink_ext_ack *extack) { struct macsec_dev *macsec = macsec_priv(dev); + rx_handler_func_t *rx_handler; + u8 icv_len = DEFAULT_ICV_LEN; struct net_device *real_dev; - int err; + int err, mtu; sci_t sci; - u8 icv_len = DEFAULT_ICV_LEN; - rx_handler_func_t *rx_handler; if (!tb[IFLA_LINK]) return -EINVAL; @@ -4033,7 +4033,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev, if (data && data[IFLA_MACSEC_ICV_LEN]) icv_len = nla_get_u8(data[IFLA_MACSEC_ICV_LEN]); - dev->mtu = real_dev->mtu - icv_len - macsec_extra_len(true); + mtu = real_dev->mtu - icv_len - macsec_extra_len(true); + if (mtu < 0) + dev->mtu = 0; + else + dev->mtu = mtu; rx_handler = rtnl_dereference(real_dev->rx_handler); if (rx_handler && rx_handler != macsec_handle_frame) -- cgit v1.2.3 From ab046a5d4be4c90a3952a0eae75617b49c0cb01b Mon Sep 17 00:00:00 2001 From: Scott Dial Date: Fri, 24 Apr 2020 18:51:08 -0400 Subject: net: macsec: preserve ingress frame ordering MACsec decryption always occurs in a softirq context. Since the FPU may not be usable in the softirq context, the call to decrypt may be scheduled on the cryptd work queue. The cryptd work queue does not provide ordering guarantees. Therefore, preserving order requires masking out ASYNC implementations of gcm(aes). For instance, an Intel CPU with AES-NI makes available the generic-gcm-aesni driver from the aesni_intel module to implement gcm(aes). However, this implementation requires the FPU, so it is not always available to use from a softirq context, and will fallback to the cryptd work queue, which does not preserve frame ordering. With this change, such a system would select gcm_base(ctr(aes-aesni),ghash-generic). While the aes-aesni implementation prefers to use the FPU, it will fallback to the aes-asm implementation if unavailable. By using a synchronous version of gcm(aes), the decryption will complete before returning from crypto_aead_decrypt(). Therefore, the macsec_decrypt_done() callback will be called before returning from macsec_decrypt(). Thus, the order of calls to macsec_post_decrypt() for the frames is preserved. While it's presumable that the pure AES-NI version of gcm(aes) is more performant, the hybrid solution is capable of gigabit speeds on modest hardware. Regardless, preserving the order of frames is paramount for many network protocols (e.g., triggering TCP retries). Within the MACsec driver itself, the replay protection is tripped by the out-of-order frames, and can cause frames to be dropped. This bug has been present in this code since it was added in v4.6, however it may not have been noticed since not all CPUs have FPU offload available. Additionally, the bug manifests as occasional out-of-order packets that are easily misattributed to other network phenomena. When this code was added in v4.6, the crypto/gcm.c code did not restrict selection of the ghash function based on the ASYNC flag. For instance, x86 CPUs with PCLMULQDQ would select the ghash-clmulni driver instead of ghash-generic, which submits to the cryptd work queue if the FPU is busy. However, this bug was was corrected in v4.8 by commit b30bdfa86431afbafe15284a3ad5ac19b49b88e3, and was backported all the way back to the v3.14 stable branch, so this patch should be applicable back to the v4.6 stable branch. Signed-off-by: Scott Dial Signed-off-by: David S. Miller --- drivers/net/macsec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/net/macsec.c') diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 758baf7cb8a1..d4034025c87c 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -1305,7 +1305,8 @@ static struct crypto_aead *macsec_alloc_tfm(char *key, int key_len, int icv_len) struct crypto_aead *tfm; int ret; - tfm = crypto_alloc_aead("gcm(aes)", 0, 0); + /* Pick a sync gcm(aes) cipher to ensure order is preserved. */ + tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC); if (IS_ERR(tfm)) return tfm; -- cgit v1.2.3 From 29ca3cdfe13b2792b8624e6f769777e8cb387f9c Mon Sep 17 00:00:00 2001 From: Antoine Tenart Date: Wed, 6 May 2020 15:58:30 +0200 Subject: net: macsec: fix rtnl locking issue netdev_update_features() must be called with the rtnl lock taken. Not doing so triggers a warning, as ASSERT_RTNL() is used in __netdev_update_features(), the first function called by netdev_update_features(). Fix this. Fixes: c850240b6c41 ("net: macsec: report real_dev features when HW offloading is enabled") Signed-off-by: Antoine Tenart Signed-off-by: David S. Miller --- drivers/net/macsec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/net/macsec.c') diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index d4034025c87c..d0d31cb99180 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -2641,11 +2641,12 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) if (ret) goto rollback; - rtnl_unlock(); /* Force features update, since they are different for SW MACSec and * HW offloading cases. */ netdev_update_features(dev); + + rtnl_unlock(); return 0; rollback: -- cgit v1.2.3