netdev - RE: [PATCH v2] ax25: Fix refcount imbalance on inbound connections

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]

Message-ID: 
 <SJ2PR18MB5635B7ADC7339BEDB79B183DA2EA2@SJ2PR18MB5635.namprd18.prod.outlook.com>
Date: Tue, 21 May 2024 17:21:40 +0000
From: Naveen Mamindlapalli <naveenm@...vell.com>
To: Lars Kellogg-Stedman <lars@...bit.com>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-hams@...r.kernel.org"
	<linux-hams@...r.kernel.org>
Subject: RE: [PATCH v2] ax25: Fix refcount imbalance on inbound connections


> -----Original Message-----
> From: Lars Kellogg-Stedman <lars@...bit.com>
> Sent: Tuesday, May 21, 2024 7:19 PM
> To: netdev@...r.kernel.org; linux-hams@...r.kernel.org
> Subject: [PATCH v2] ax25: Fix refcount imbalance on inbound
> connections
> 
> The first version of this patch was posted only to the linux-hams mailing list. It
> has been difficult to get the patch reviewed, but the patch has now been tested
> successfully by three people (that includes
> me) who have all verified that it prevents the crashes that were previously
> plaguing inbound ax.25 connections.
> 
> Related discussions:
> 
> - https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-
> 2Dhams-26m-3D171629285223248-26w-
> 3D2&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6mQ8K9wIpqwFO8y
> jikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzfKSd1iQ3b4biO_v8e
> 0R8Llg9vHP8lSQ3tGo5sXr4&s=-
> MpoDvDpxv4ixHLeXbhPn1j9UnlUz6cJ29sdjMqsA6I&e=
> - https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-
> 2Dhams-26m-3D171270115728031-26w-
> 3D2&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6mQ8K9wIpqwFO8y
> jikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzfKSd1iQ3b4biO_v8e
> 0R8Llg9vHP8lSQ3tGo5sXr4&s=3ZUXfkS5ChKuooKI5BBasvYQeJMH-
> FYqbeskhJKJdOI&e=
> 
> >8------------------------------------------------------8<
> 
> When releasing a socket in ax25_release(), we call netdev_put() to decrease the
> refcount on the associated ax.25 device. However, the execution path for
> accepting an incoming connection never calls netdev_hold(). This imbalance
> leads to refcount errors, and ultimately to kernel crashes.
> 
> A typical call trace for the above situation looks like this:
> 
>     Call Trace:
>     <TASK>
>     ? show_regs+0x64/0x70
>     ? __warn+0x83/0x120
>     ? refcount_warn_saturate+0xb2/0x100
>     ? report_bug+0x158/0x190
>     ? prb_read_valid+0x20/0x30
>     ? handle_bug+0x3e/0x70
>     ? exc_invalid_op+0x1c/0x70
>     ? asm_exc_invalid_op+0x1f/0x30
>     ? refcount_warn_saturate+0xb2/0x100
>     ? refcount_warn_saturate+0xb2/0x100
>     ax25_release+0x2ad/0x360
>     __sock_release+0x35/0xa0
>     sock_close+0x19/0x20
>     [...]
> 
> On reboot (or any attempt to remove the interface), the kernel gets stuck in an
> infinite loop:
> 
>     unregister_netdevice: waiting for ax0 to become free. Usage count = 0
> 
> This patch corrects these issues by ensuring that we call netdev_hold() and
> ax25_dev_hold() for new connections in ax25_accept(), balancing the calls to
> netdev_put() and ax25_dev_put() in ax25_release.
> 
> Fixes: 7d8a3a477b
> Signed-off-by: Lars Kellogg-Stedman <lars@...bit.com>
> ---
>  net/ax25/af_ax25.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index
> 8077cf2ee44..ff921272d40 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1381,6 +1381,8 @@ static int ax25_accept(struct socket *sock, struct
> socket *newsock,
>  	DEFINE_WAIT(wait);
>  	struct sock *sk;
>  	int err = 0;
> +	ax25_cb *ax25;
> +	ax25_dev *ax25_dev;

nit: Please follow reverse Christmas tree.

> 
>  	if (sock->state != SS_UNCONNECTED)
>  		return -EINVAL;
> @@ -1434,6 +1436,10 @@ static int ax25_accept(struct socket *sock, struct
> socket *newsock,
>  	kfree_skb(skb);
>  	sk_acceptq_removed(sk);
>  	newsock->state = SS_CONNECTED;
> +	ax25 = sk_to_ax25(newsk);
> +	ax25_dev = ax25->ax25_dev;
> +	netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC);
> +	ax25_dev_hold(ax25_dev);
> 
>  out:
>  	release_sock(sk);
> --
> 2.45.1
> 
> --
> Lars Kellogg-Stedman <lars@...bit.com> | larsks @ {irc,twitter,github}
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.oddbit.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6
> mQ8K9wIpqwFO8yjikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzf
> KSd1iQ3b4biO_v8e0R8Llg9vHP8lSQ3tGo5sXr4&s=8ug-soRMsh7y1vLY4tO8OSx-
> bKun8bXlZ7uAPiQnePU&e=                 | N1LKS