macsec: a few fixes
Subsystems (3)
Sabrina Dubroca
maintainerv2 cover macsec: a few fixes
Some small fixes for the macsec driver:
- possible NULL pointer dereferences
- netlink dumps fixes: RTNL locking, consistent dumps
- a reference counting bug
- wrong name for uapi constant
- a few memory leaks
Patches 1 to 5 are the same as in v1, patches 6 to 9 are new. Patch 6 fixes the memleak that Lance spotted in v1.
Sabrina Dubroca (9):
macsec: add missing NULL check after kmalloc
macsec: take rtnl lock before for_each_netdev
macsec: don't put a NULL rxsa
macsec: fix rx_sa refcounting with decrypt callback
macsec: add consistency check to netlink dumps
macsec: fix memory leaks around rx_handler (un)registration
macsec: fix SA leak if initialization fails
macsec: add missing macsec prefix in uapi
macsec: fix netlink attribute validation
drivers/net/macsec.c | 65 +++++++++++++++++++++++++++--------------- include/uapi/linux/if_macsec.h | 4 +-- 2 files changed, 44 insertions(+), 25 deletions(-)
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 1/9 macsec: add missing NULL check after kmalloc
--- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/macsec.c +2 -2
@@ -1622,8 +1622,8 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) } rx_sa =kmalloc (sizeof (*rx_sa),GFP_KERNEL ); -if (init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, - secy->icv_len)) { +if (!rx_sa ||init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), + secy->key_len, secy->icv_len)) {rtnl_unlock ();return -ENOMEM ; }
Signature · 1 line
2.8.0
Re: [PATCH net v2 1/9] macsec: add missing NULL check after kmalloc
Quoted 13 lines · Sabrina Dubroca
Fixes c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")Reported-by Dan Carpenter <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/net/macsec.c +0
-0
drivers/net/macsec.c static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) · line 1629
Quoted 10 lines
drivers/net/macsec.c +2
-2@@ -1622,8 +1622,8 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) } rx_sa =kmalloc (sizeof (*rx_sa),GFP_KERNEL ); -if (init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, - secy->icv_len)) { +if (!rx_sa ||init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), + secy->key_len, secy->icv_len)) {rtnl_unlock ();return -ENOMEM ; }
In case that kmalloc was successful and init_rx_sa failed, the allocated memory should be freed, shouldnt it?.
Regards,
Lino
Sabrina Dubroca
maintainerRe: [PATCH net v2 1/9] macsec: add missing NULL check after kmalloc
Quoted 18 lines · Lino Sanfilippo
Quoted 15 lines · Sabrina Dubroca
Fixes c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")Reported-by Dan Carpenter <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/net/macsec.c +0
-0@@ -1622,8 +1622,8 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) }
Quoted 11 lines
drivers/net/macsec.c static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) · line 1629
Quoted 8 lines
drivers/net/macsec.c +2
-2rx_sa =kmalloc (sizeof (*rx_sa),GFP_KERNEL ); -if (init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, - secy->icv_len)) { +if (!rx_sa ||init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), + secy->key_len, secy->icv_len)) {rtnl_unlock ();return -ENOMEM ; }In case that kmalloc was successful and
init_rx_safailed, the allocated memory should be freed, shouldnt it?.
Yep, Lance pointed that out in v1:
http://marc.info/?l=linux-netdev&m=146108796406155
But since we have the same code with init_tx_sa, I decided to fix both in a separate patch (7/9 in this set).
Thanks,
Signature · 1 line
Sabrina
Re: [PATCH net v2 1/9] macsec: add missing NULL check after kmalloc
Quoted 31 lines · Sabrina Dubroca
Quoted 18 lines · Lino Sanfilippo
Quoted 15 lines · Sabrina Dubroca
Fixes c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")Reported-by Dan Carpenter <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/net/macsec.c +0
-0@@ -1622,8 +1622,8 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) }Quoted 11 lines
drivers/net/macsec.c static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) · line 1629
Quoted 8 lines
drivers/net/macsec.c +2
-2rx_sa =kmalloc (sizeof (*rx_sa),GFP_KERNEL ); -if (init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, - secy->icv_len)) { +if (!rx_sa ||init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), + secy->key_len, secy->icv_len)) {rtnl_unlock ();return -ENOMEM ; }In case that kmalloc was successful and
init_rx_safailed, the allocated memory should be freed, shouldnt it?.
Quoted 6 lines
Yep, Lance pointed that out in v1:
http://marc.info/?l=linux-netdev&m=146108796406155But since we have the same code with
init_tx_sa, I decided to fix both in a separate patch (7/9 in this set).
Ah ok, sorry about the noise then :)
Regards,
Lino
Sabrina Dubroca
maintainerPATCH v2 2/9 macsec: take rtnl lock before for_each_netdev
In reply to Sabrina Dubroca--- drivers/net/macsec.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/net/macsec.c +2 -5
@@ -2268,8 +2268,6 @@ static int dump_secy(struct macsec_secy *secy, struct net_device *dev,if (!hdr)return -EMSGSIZE ; -rtnl_lock (); -if (nla_put_u32 (skb,MACSEC_ATTR_IFINDEX , dev->ifindex)) goto nla_put_failure;
@@ -2429,14 +2427,11 @@ static int dump_secy(struct macsec_secy *secy, struct net_device *dev,nla_nest_end (skb, rxsc_list); -rtnl_unlock (); -genlmsg_end (skb, hdr);return 0 ; nla_put_failure: -rtnl_unlock ();genlmsg_cancel (skb, hdr);return -EMSGSIZE ; }
@@ -2450,6 +2445,7 @@ static int macsec_dump_txsc(struct sk_buff *skb, struct netlink_callback *cb) dev_idx = cb->args[0 ]; d =0 ; +rtnl_lock ();for_each_netdev (net, dev) {struct macsec_secy *secy;
@@ -2467,6 +2463,7 @@ next: } done: +rtnl_unlock (); cb->args[0 ] = d;return skb->len; }
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 3/9 macsec: don't put a NULL rxsa
In reply to Sabrina DubrocaThe "deliver:" path of macsec_handle_frame can be called with rx_sa == NULL. Check rx_sa != NULL before calling macsec_rxsa_put().
--- drivers/net/macsec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/macsec.c +2 -1
@@ -1161,7 +1161,8 @@ deliver: macsec_extra_len(macsec_skb_cb(skb)->has_sci));macsec_reset_skb (skb, secy->netdev); -macsec_rxsa_put (rx_sa); +if (rx_sa) +macsec_rxsa_put (rx_sa);count_rx (dev, skb->len);rcu_read_unlock ();
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 4/9 macsec: fix rx_sa refcounting with decrypt callback
In reply to Sabrina DubrocaThe decrypt callback macsec_decrypt_done needs a reference on the rx_sa and releases it before returning, but macsec_handle_frame already put that reference after macsec_decrypt returned NULL.
Set rx_sa to NULL when the decrypt callback runs so that macsec_handle_frame knows it must not release the reference.
--- drivers/net/macsec.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/net/macsec.c +9 -7
@@ -880,12 +880,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,macsec_skb_cb (skb)->valid = false; skb =skb_share_check (skb,GFP_ATOMIC );if (!skb) -return NULL ; +return ERR_PTR (-ENOMEM ); req =aead_request_alloc (rx_sa->key.tfm,GFP_ATOMIC );if (!req) {kfree_skb (skb); -return NULL ; +return ERR_PTR (-ENOMEM ); } hdr = (struct macsec_eth_header *)skb->data;
@@ -905,7 +905,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, skb =skb_unshare (skb,GFP_ATOMIC );if (!skb) {aead_request_free (req); -return NULL ; +return ERR_PTR (-ENOMEM ); } }else {/* integrity only: all headers + data authenticated */
@@ -921,14 +921,14 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,dev_hold (dev); ret =crypto_aead_decrypt (req);if (ret == -EINPROGRESS ) { -return NULL ; +return ERR_PTR (ret); }else if (ret !=0 ) {/* decryption/authentication failed * 10.6 if validateFrames is disabled, deliver anyway */ if (ret != -EBADMSG ) {kfree_skb (skb); - skb =NULL ; + skb =ERR_PTR (ret); } }else {macsec_skb_cb (skb)->valid = true;
@@ -1146,8 +1146,10 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb) secy->validate_frames != MACSEC_VALIDATE_DISABLED) skb =macsec_decrypt (skb, dev, rx_sa, sci, secy); -if (!skb) { -macsec_rxsa_put (rx_sa); +if (IS_ERR (skb)) { +/* the decrypt callback needs the reference */ +if (PTR_ERR (skb) != -EINPROGRESS ) +macsec_rxsa_put (rx_sa);rcu_read_unlock (); *pskb =NULL ;return RX_HANDLER_CONSUMED ;
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 5/9 macsec: add consistency check to netlink dumps
In reply to Sabrina DubrocaUse genl_dump_check_consistent in dump_secy.
--- drivers/net/macsec.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/net/macsec.c +11 -0
@@ -2271,6 +2271,8 @@ static int dump_secy(struct macsec_secy *secy, struct net_device *dev,if (!hdr)return -EMSGSIZE ; +genl_dump_check_consistent (cb, hdr, &macsec_fam); +if (nla_put_u32 (skb,MACSEC_ATTR_IFINDEX , dev->ifindex)) goto nla_put_failure;
@@ -2439,6 +2441,8 @@ nla_put_failure:return -EMSGSIZE ; } +static int macsec_generation =1 ;/* protected by RTNL */ +static int macsec_dump_txsc (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);
@@ -2449,6 +2453,9 @@ static int macsec_dump_txsc(struct sk_buff *skb, struct netlink_callback *cb) d =0 ;rtnl_lock (); + + cb->seq = macsec_generation; +for_each_netdev (net, dev) {struct macsec_secy *secy;
@@ -2920,6 +2927,8 @@ static void macsec_dellink(struct net_device *dev, struct list_head *head)struct net_device *real_dev = macsec->real_dev;struct macsec_rxh_data *rxd =macsec_data_rtnl (real_dev); + macsec_generation++; +unregister_netdevice_queue (dev, head);list_del_rcu (&macsec->secys);if (list_empty (&rxd->secys))
@@ -3066,6 +3075,8 @@ static int macsec_newlink(struct net *net, struct net_device *dev,if (err <0 ) goto del_dev; + macsec_generation++; +dev_hold (real_dev);return 0 ;
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 6/9 macsec: fix memory leaks around rx_handler (un)registration
In reply to Sabrina DubrocaWe leak a struct macsec_rxh_data when we unregister the rx_handler in macsec_dellink.
We also leak a struct macsec_rxh_data in register_macsec_dev if we fail to register the rx_handler.
--- drivers/net/macsec.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/net/macsec.c +6 -2
@@ -2931,8 +2931,10 @@ static void macsec_dellink(struct net_device *dev, struct list_head *head)unregister_netdevice_queue (dev, head);list_del_rcu (&macsec->secys); -if (list_empty (&rxd->secys)) +if (list_empty (&rxd->secys)) {netdev_rx_handler_unregister (real_dev); +kfree (rxd); + }macsec_del_dev (macsec); }
@@ -2954,8 +2956,10 @@ static int register_macsec_dev(struct net_device *real_dev, err =netdev_rx_handler_register (real_dev, macsec_handle_frame, rxd); -if (err <0 ) +if (err <0 ) { +kfree (rxd);return err; + } }list_add_tail_rcu (&macsec->secys, &rxd->secys);
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 7/9 macsec: fix SA leak if initialization fails
In reply to Sabrina Dubroca--- drivers/net/macsec.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/net/macsec.c +2 -0
@@ -1627,6 +1627,7 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info) rx_sa =kmalloc (sizeof (*rx_sa),GFP_KERNEL );if (!rx_sa ||init_rx_sa (rx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, secy->icv_len)) { +kfree (rx_sa);rtnl_unlock ();return -ENOMEM ; }
@@ -1771,6 +1772,7 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info) tx_sa =kmalloc (sizeof (*tx_sa),GFP_KERNEL );if (!tx_sa ||init_tx_sa (tx_sa,nla_data (tb_sa[MACSEC_SA_ATTR_KEY ]), secy->key_len, secy->icv_len)) { +kfree (tx_sa);rtnl_unlock ();return -ENOMEM ; }
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 8/9 macsec: add missing macsec prefix in uapi
In reply to Sabrina DubrocaI accidentally forgot some MACSEC_ prefixes in if_macsec.h.
--- drivers/net/macsec.c | 12 +++++++----- include/uapi/linux/if_macsec.h | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-)
drivers/net/macsec.c +7 -5
@@ -2232,7 +2232,8 @@ static int nla_put_secy(struct macsec_secy *secy, struct sk_buff *skb)return 1 ; if (nla_put_sci(skb, MACSEC_SECY_ATTR_SCI, secy->sci) || - nla_put_u64(skb, MACSEC_SECY_ATTR_CIPHER_SUITE, DEFAULT_CIPHER_ID) || + nla_put_u64(skb, MACSEC_SECY_ATTR_CIPHER_SUITE, + MACSEC_DEFAULT_CIPHER_ID) || nla_put_u8(skb, MACSEC_SECY_ATTR_ICV_LEN, secy->icv_len) || nla_put_u8(skb, MACSEC_SECY_ATTR_OPER, secy->operational) || nla_put_u8(skb, MACSEC_SECY_ATTR_PROTECT, secy->protect_frames) ||
@@ -3096,7 +3097,7 @@ unregister:static int macsec_validate_attr (struct nlattr *tb[],struct nlattr *data[]) { -u64 csid =DEFAULT_CIPHER_ID ; +u64 csid =MACSEC_DEFAULT_CIPHER_ID ;u8 icv_len =DEFAULT_ICV_LEN ;int flag;bool es, scb, sci;
@@ -3111,8 +3112,8 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[]) icv_len =nla_get_u8 (data[IFLA_MACSEC_ICV_LEN ]);switch (csid) { -case DEFAULT_CIPHER_ID : -case DEFAULT_CIPHER_ALT : +case MACSEC_DEFAULT_CIPHER_ID : +case MACSEC_DEFAULT_CIPHER_ALT :if (icv_len <MACSEC_MIN_ICV_LEN || icv_len >MACSEC_MAX_ICV_LEN )return -EINVAL ;
@@ -3185,7 +3186,8 @@ static int macsec_fill_info(struct sk_buff *skb, if (nla_put_sci(skb, IFLA_MACSEC_SCI, secy->sci) || nla_put_u8(skb, IFLA_MACSEC_ICV_LEN, secy->icv_len) || - nla_put_u64(skb, IFLA_MACSEC_CIPHER_SUITE, DEFAULT_CIPHER_ID) || + nla_put_u64(skb, IFLA_MACSEC_CIPHER_SUITE, + MACSEC_DEFAULT_CIPHER_ID) || nla_put_u8(skb, IFLA_MACSEC_ENCODING_SA, tx_sc->encoding_sa) || nla_put_u8(skb, IFLA_MACSEC_ENCRYPT, tx_sc->encrypt) || nla_put_u8(skb, IFLA_MACSEC_PROTECT, secy->protect_frames) ||
include/uapi/linux/if_macsec.h +2 -2
@@ -19,8 +19,8 @@#define MACSEC_MAX_KEY_LEN 128 -#define DEFAULT_CIPHER_ID 0x0080020001000001ULL -#define DEFAULT_CIPHER_ALT 0x0080C20001000001ULL +#define MACSEC_DEFAULT_CIPHER_ID 0x0080020001000001ULL +#define MACSEC_DEFAULT_CIPHER_ALT 0x0080C20001000001ULL#define MACSEC_MIN_ICV_LEN 8#define MACSEC_MAX_ICV_LEN 32
Signature · 1 line
2.8.0
Sabrina Dubroca
maintainerPATCH v2 9/9 macsec: fix netlink attribute validation
In reply to Sabrina Dubrocamacsec_validate_attr should check IFLA_MACSEC_REPLAY_PROTECT (not IFLA_MACSEC_PROTECT) to verify that the replay protection and replay window arguments are correct.
--- drivers/net/macsec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/macsec.c +2 -2
@@ -3147,8 +3147,8 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[]) nla_get_u8(data[IFLA_MACSEC_VALIDATION]) > MACSEC_VALIDATE_MAX)return -EINVAL ; -if ((data[IFLA_MACSEC_PROTECT ] && -nla_get_u8 (data[IFLA_MACSEC_PROTECT ])) && +if ((data[IFLA_MACSEC_REPLAY_PROTECT ] && +nla_get_u8 (data[IFLA_MACSEC_REPLAY_PROTECT ])) && !data[IFLA_MACSEC_WINDOW ])return -EINVAL ;
Signature · 1 line
2.8.0
David Miller
maintainerFrom: Sabrina Dubroca <[email protected]>
Date: Fri, 22 Apr 2016 11:28:00 +0200
Quoted 9 lines
Some small fixes for the macsec driver:
- possible NULL pointer dereferences
- netlink dumps fixes: RTNL locking, consistent dumps
- a reference counting bug
- wrong name for uapi constant
- a few memory leaksPatches 1 to 5 are the same as in v1, patches 6 to 9 are new. Patch 6 fixes the memleak that Lance spotted in v1.
Series applied, thanks Sabrina.