linux-kernel - Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Mar 2022 13:53:44 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Duoming Zhou <duoming@....edu.cn>
Cc:     linux-hams@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, jreuter@...na.de, kuba@...nel.org,
        davem@...emloft.net, ralf@...ux-mips.org, thomas@...erried.de
Subject: Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del()

On Fri, Mar 11, 2022 at 09:46:24AM +0800, Duoming Zhou wrote:
> This patch adds a flag in ax25_dev in order to prevent reference count
> leaks. If the above condition happens, the "test_bit" condition check
> in ax25_kill_by_device() could pass and the refcounts could be
> decreased properly.
>

Why are you using test_bit()?  Just use booleans.

I am not a networking person but this whole thing feel like adding on
layers of cruft.  If refcounting needs a lot of additional tracking
information then probably the reference counts are not being done in
the correct location.

> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 6bd09718077..fc564b87acc 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -86,6 +86,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>  again:
>  	ax25_for_each(s, &ax25_list) {
>  		if (s->ax25_dev == ax25_dev) {
> +			set_bit(AX25_DEV_KILL, &ax25_dev->flag);
>  			sk = s->sk;
>  			if (!sk) {
>  				spin_unlock_bh(&ax25_list_lock);

Why can this not be a local variable.

	bool found = false;

	ax25_for_each(s, &ax25_list) {
		if (s->ax25_dev == ax25_dev) {
			found = true;
			...

> @@ -115,6 +116,10 @@ static void ax25_kill_by_device(struct net_device *dev)
>  		}
>  	}
>  	spin_unlock_bh(&ax25_list_lock);
> +	if (!test_bit(AX25_DEV_KILL, &ax25_dev->flag) && test_bit(AX25_DEV_BIND, &ax25_dev->flag)) {

The comments for ax25_kill_by_device() say:

/*
 *      Kill all bound sockets on a dropped device.
 */

So how can test_bit(AX25_DEV_BIND, &ax25_dev->flag) ever be false at
this location?

	if (!found) {
		dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
		ax25_dev_put(ax25_dev);
	}

But even here, my instinct is that if the refcounting is were done in
the correct place we would not need any additional variables.  Is there
no simpler solution?

> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index d2a244e1c26..9b04d74a1be 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -77,6 +77,7 @@ void ax25_dev_device_up(struct net_device *dev)
>  	ax25_dev->values[AX25_VALUES_PACLEN]	= AX25_DEF_PACLEN;
>  	ax25_dev->values[AX25_VALUES_PROTOCOL]  = AX25_DEF_PROTOCOL;
>  	ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT;
> +	ax25_dev->flag = AX25_DEV_INIT;

There is no need for this, it's allocated with kzalloc().

>  
>  #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER)
>  	ax25_ds_setup_timer(ax25_dev);

regards,
dan carpenter