LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Navid Emamdoost <navid.emamdoost@gmail.com>
Cc: emamd001@umn.edu, smccaman@umn.edu, kjlu@umn.edu,
	"David S. Miller" <davem@davemloft.net>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	John Hurley <john.hurley@netronome.com>,
	Colin Ian King <colin.king@canonical.com>,
	oss-drivers@netronome.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
Date: Wed, 4 Dec 2019 10:39:55 -0800
Message-ID: <20191204103955.63c4d9af@cakuba.netronome.com> (raw)
In-Reply-To: <20190927015157.20070-1-navid.emamdoost@gmail.com>

On Thu, 26 Sep 2019 20:51:46 -0500, Navid Emamdoost wrote:
> In nfp_abm_u32_knode_replace if the allocation for match fails it should
> go to the error handling instead of returning. Updated other gotos to
> have correct errno returned, too.
> 
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	- Reused err variable for erorr value returning.
> Changes in v3:
> 	- Fix the err value in the first goto, and fix subject prefix.

Ugh damn this. Apparently this "fix" has made the news:

https://news.softpedia.com/news/canonical-releases-major-kernel-security-update-for-ubuntu-19-10-and-18-04-lts-528433.shtml

https://nvd.nist.gov/vuln/detail/CVE-2019-19076

and (a) it would be a damn control path, root-only memory leak, but
also (b) upon closer inspection there is no leak here at all!

We don't need to delete the entry if we failed to allocate it...
The delete path is in case the entry for the handle is changed, but 
if we're trying to allocate one anew there can't be any on the list.

Congratulations to whoever classified this as a security fix.

I will send a revert, and go ask for the CVE to be marked invalid.
What a waste of time. I should have paid more attention :/

> diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> index 23ebddfb9532..9f8a1f69c0c4 100644
> --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
> +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> @@ -176,8 +176,10 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
>  	u8 mask, val;
>  	int err;
>  
> -	if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack))
> +	if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) {
> +		err = -EOPNOTSUPP;
>  		goto err_delete;
> +	}
>  
>  	tos_off = proto == htons(ETH_P_IP) ? 16 : 20;
>  
> @@ -198,14 +200,18 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
>  		if ((iter->val & cmask) == (val & cmask) &&
>  		    iter->band != knode->res->classid) {
>  			NL_SET_ERR_MSG_MOD(extack, "conflict with already offloaded filter");
> +			err = -EOPNOTSUPP;
>  			goto err_delete;
>  		}
>  	}
>  
>  	if (!match) {
>  		match = kzalloc(sizeof(*match), GFP_KERNEL);
> -		if (!match)
> -			return -ENOMEM;
> +		if (!match) {
> +			err = -ENOMEM;
> +			goto err_delete;
> +		}
> +
>  		list_add(&match->list, &alink->dscp_map);
>  	}
>  	match->handle = knode->handle;
> @@ -221,7 +227,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
>  
>  err_delete:
>  	nfp_abm_u32_knode_delete(alink, knode);
> -	return -EOPNOTSUPP;
> +	return err;
>  }
>  
>  static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type,


  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 18:34 [PATCH] net: flow_offload: " Navid Emamdoost
2019-09-26  1:28 ` Jakub Kicinski
2019-09-26  2:22   ` [PATCH v2] " Navid Emamdoost
2019-09-26  4:53     ` Jakub Kicinski
2019-09-27  1:51       ` [PATCH v3] nfp: abm: " Navid Emamdoost
2019-09-27 12:12         ` Markus Elfring
2019-09-27 21:42           ` Jakub Kicinski
2019-09-28  5:55             ` Markus Elfring
2019-09-27 18:56         ` [PATCH v3] " David Miller
2019-12-04 18:39         ` Jakub Kicinski [this message]
2019-09-27  2:26       ` [PATCH v2] net: flow_offload: " Navid Emamdoost

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191204103955.63c4d9af@cakuba.netronome.com \
    --to=jakub.kicinski@netronome.com \
    --cc=colin.king@canonical.com \
    --cc=davem@davemloft.net \
    --cc=emamd001@umn.edu \
    --cc=john.hurley@netronome.com \
    --cc=kjlu@umn.edu \
    --cc=linux-kernel@vger.kernel.org \
    --cc=navid.emamdoost@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=oss-drivers@netronome.com \
    --cc=pablo@netfilter.org \
    --cc=smccaman@umn.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git