Re: [PATCH v2] macsec: dynamically allocate space for sglist
From: "Jason A. Donenfeld"Date: Tue, 25 Apr 2017 19:08:18 +0200 > We call skb_cow_data, which is good anyway to ensure we can actually > modify the skb as such (another error from prior). Now that we have the > number of fragments required, we can safely allocate exactly that amount > of memory. > > Signed-off-by: Jason A. Donenfeld > Cc: Sabrina Dubroca > Cc: secur...@kernel.org > Cc: sta...@vger.kernel.org Applied, thanks.
Re: [PATCH v2] macsec: dynamically allocate space for sglist
From: "Jason A. Donenfeld" Date: Tue, 25 Apr 2017 19:08:18 +0200 > We call skb_cow_data, which is good anyway to ensure we can actually > modify the skb as such (another error from prior). Now that we have the > number of fragments required, we can safely allocate exactly that amount > of memory. > > Signed-off-by: Jason A. Donenfeld > Cc: Sabrina Dubroca > Cc: secur...@kernel.org > Cc: sta...@vger.kernel.org Applied, thanks.
Re: [PATCH v2] macsec: dynamically allocate space for sglist
2017-04-25, 19:08:18 +0200, Jason A. Donenfeld wrote: > We call skb_cow_data, which is good anyway to ensure we can actually > modify the skb as such (another error from prior). Now that we have the > number of fragments required, we can safely allocate exactly that amount > of memory. > > Signed-off-by: Jason A. Donenfeld> Cc: Sabrina Dubroca > Cc: secur...@kernel.org > Cc: sta...@vger.kernel.org Acked-by: Sabrina Dubroca Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Fixes: CVE-2017-7477 David, this fix is essentially equivalent to my patch "macsec: avoid heap overflow in skb_to_sgvec on receive". Feel free to pick my patch if you prefer (it's smaller), but this looks ok to me. Thanks, -- Sabrina
Re: [PATCH v2] macsec: dynamically allocate space for sglist
2017-04-25, 19:08:18 +0200, Jason A. Donenfeld wrote: > We call skb_cow_data, which is good anyway to ensure we can actually > modify the skb as such (another error from prior). Now that we have the > number of fragments required, we can safely allocate exactly that amount > of memory. > > Signed-off-by: Jason A. Donenfeld > Cc: Sabrina Dubroca > Cc: secur...@kernel.org > Cc: sta...@vger.kernel.org Acked-by: Sabrina Dubroca Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver") Fixes: CVE-2017-7477 David, this fix is essentially equivalent to my patch "macsec: avoid heap overflow in skb_to_sgvec on receive". Feel free to pick my patch if you prefer (it's smaller), but this looks ok to me. Thanks, -- Sabrina
[PATCH v2] macsec: dynamically allocate space for sglist
We call skb_cow_data, which is good anyway to ensure we can actually modify the skb as such (another error from prior). Now that we have the number of fragments required, we can safely allocate exactly that amount of memory. Signed-off-by: Jason A. DonenfeldCc: Sabrina Dubroca Cc: secur...@kernel.org Cc: sta...@vger.kernel.org --- Changes v1 -> v2: - sg_init_table now takes the correct argument. drivers/net/macsec.c | 29 ++--- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index dbab05afcdbe..49ce4e9f4a0f 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request *base, int err) static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm, unsigned char **iv, -struct scatterlist **sg) +struct scatterlist **sg, +int num_frags) { size_t size, iv_offset, sg_offset; struct aead_request *req; @@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm, size = ALIGN(size, __alignof__(struct scatterlist)); sg_offset = size; - size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1); + size += sizeof(struct scatterlist) * num_frags; tmp = kmalloc(size, GFP_ATOMIC); if (!tmp) @@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, { int ret; struct scatterlist *sg; + struct sk_buff *trailer; unsigned char *iv; struct ethhdr *eth; struct macsec_eth_header *hh; @@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, return ERR_PTR(-EINVAL); } - req = macsec_alloc_req(tx_sa->key.tfm, , ); + ret = skb_cow_data(skb, 0, ); + if (unlikely(ret < 0)) { + macsec_txsa_put(tx_sa); + kfree_skb(skb); + return ERR_PTR(ret); + } + + req = macsec_alloc_req(tx_sa->key.tfm, , , ret); if (!req) { macsec_txsa_put(tx_sa); kfree_skb(skb); @@ -732,7 +741,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, macsec_fill_iv(iv, secy->sci, pn); - sg_init_table(sg, MAX_SKB_FRAGS + 1); + sg_init_table(sg, ret); skb_to_sgvec(skb, sg, 0, skb->len); if (tx_sc->encrypt) { @@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, { int ret; struct scatterlist *sg; + struct sk_buff *trailer; unsigned char *iv; struct aead_request *req; struct macsec_eth_header *hdr; @@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, if (!skb) return ERR_PTR(-ENOMEM); - req = macsec_alloc_req(rx_sa->key.tfm, , ); + ret = skb_cow_data(skb, 0, ); + if (unlikely(ret < 0)) { + kfree_skb(skb); + return ERR_PTR(ret); + } + req = macsec_alloc_req(rx_sa->key.tfm, , , ret); if (!req) { kfree_skb(skb); return ERR_PTR(-ENOMEM); @@ -936,7 +951,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, hdr = (struct macsec_eth_header *)skb->data; macsec_fill_iv(iv, sci, ntohl(hdr->packet_number)); - sg_init_table(sg, MAX_SKB_FRAGS + 1); + sg_init_table(sg, ret); skb_to_sgvec(skb, sg, 0, skb->len); if (hdr->tci_an & MACSEC_TCI_E) { @@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb, } #define MACSEC_FEATURES \ - (NETIF_F_SG | NETIF_F_HIGHDMA) + (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST) static struct lock_class_key macsec_netdev_addr_lock_key; static int macsec_dev_init(struct net_device *dev) -- 2.12.2
[PATCH v2] macsec: dynamically allocate space for sglist
We call skb_cow_data, which is good anyway to ensure we can actually modify the skb as such (another error from prior). Now that we have the number of fragments required, we can safely allocate exactly that amount of memory. Signed-off-by: Jason A. Donenfeld Cc: Sabrina Dubroca Cc: secur...@kernel.org Cc: sta...@vger.kernel.org --- Changes v1 -> v2: - sg_init_table now takes the correct argument. drivers/net/macsec.c | 29 ++--- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index dbab05afcdbe..49ce4e9f4a0f 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request *base, int err) static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm, unsigned char **iv, -struct scatterlist **sg) +struct scatterlist **sg, +int num_frags) { size_t size, iv_offset, sg_offset; struct aead_request *req; @@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm, size = ALIGN(size, __alignof__(struct scatterlist)); sg_offset = size; - size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1); + size += sizeof(struct scatterlist) * num_frags; tmp = kmalloc(size, GFP_ATOMIC); if (!tmp) @@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, { int ret; struct scatterlist *sg; + struct sk_buff *trailer; unsigned char *iv; struct ethhdr *eth; struct macsec_eth_header *hh; @@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, return ERR_PTR(-EINVAL); } - req = macsec_alloc_req(tx_sa->key.tfm, , ); + ret = skb_cow_data(skb, 0, ); + if (unlikely(ret < 0)) { + macsec_txsa_put(tx_sa); + kfree_skb(skb); + return ERR_PTR(ret); + } + + req = macsec_alloc_req(tx_sa->key.tfm, , , ret); if (!req) { macsec_txsa_put(tx_sa); kfree_skb(skb); @@ -732,7 +741,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, macsec_fill_iv(iv, secy->sci, pn); - sg_init_table(sg, MAX_SKB_FRAGS + 1); + sg_init_table(sg, ret); skb_to_sgvec(skb, sg, 0, skb->len); if (tx_sc->encrypt) { @@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, { int ret; struct scatterlist *sg; + struct sk_buff *trailer; unsigned char *iv; struct aead_request *req; struct macsec_eth_header *hdr; @@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, if (!skb) return ERR_PTR(-ENOMEM); - req = macsec_alloc_req(rx_sa->key.tfm, , ); + ret = skb_cow_data(skb, 0, ); + if (unlikely(ret < 0)) { + kfree_skb(skb); + return ERR_PTR(ret); + } + req = macsec_alloc_req(rx_sa->key.tfm, , , ret); if (!req) { kfree_skb(skb); return ERR_PTR(-ENOMEM); @@ -936,7 +951,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb, hdr = (struct macsec_eth_header *)skb->data; macsec_fill_iv(iv, sci, ntohl(hdr->packet_number)); - sg_init_table(sg, MAX_SKB_FRAGS + 1); + sg_init_table(sg, ret); skb_to_sgvec(skb, sg, 0, skb->len); if (hdr->tci_an & MACSEC_TCI_E) { @@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb, } #define MACSEC_FEATURES \ - (NETIF_F_SG | NETIF_F_HIGHDMA) + (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST) static struct lock_class_key macsec_netdev_addr_lock_key; static int macsec_dev_init(struct net_device *dev) -- 2.12.2