[PATCH v5] ax25: Fix refcount imbalance on inbound connections

[PATCH v5] ax25: Fix refcount imbalance on inbound connections

[lars]

From: Lars Kellogg-Stedman <lars@oddbit.com>

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 will start with one of the
following errors:

    refcount_t: decrement hit 0; leaking memory.
    refcount_t: underflow; use-after-free.

And will then have a trace like:

    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(). This makes the
logic leading to ax25_accept() match the logic for ax25_bind(): in both
cases we increment the refcount, which is ultimately decremented in
ax25_release().

Fixes: 9fd75b66b8f6 ("ax25: Fix refcount leaks caused by ax25_cb_del()")
Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
Tested-by: Duoming Zhou <duoming@zju.edu.cn>
Tested-by: Dan Cross <crossd@gmail.com>
Tested-by: Chris Maness <christopher.maness@gmail.com>
---
 net/ax25/af_ax25.c | 6 ++++++
 1 file changed, 6 insertions(+)

v3:
- Address naveenm's comments regarding the ordering of variable declarations
  (https://lore.kernel.org/netdev/SJ2PR18MB5635B7ADC7339BEDB79B183DA2EA2@SJ2PR18MB5635.namprd18.prod.outlook.com/)

v4:
- Respond to kuba's comments regarding the Fixes: tag
  (https://lore.kernel.org/netdev/20240522100701.4d9edf99@kernel.org/)

v5:
- Respond to pabeni's comments regarding the Fixes: tag and running
  checkpatch.pl
  (https://lore.kernel.org/netdev/8e9a1c59f78a7774268bb6defed46df6f3771cbc.camel@redhat.com/)
- Respond to dan.carpenter's request about rewording the commit message
  (https://lore.kernel.org/netdev/962afcda-8f67-400f-b3eb-951bf2e46fb7@moroto.mountain/)
- Accept duoming's suggestion for the Fixes: tag
  (https://lore.kernel.org/netdev/3cf699c4.20d18.18fc4df304a.Coremail.duoming@zju.edu.cn/)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 8077cf2ee44..d6f9fae06a9 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1378,8 +1378,10 @@ static int ax25_accept(struct socket *sock, struct socket *newsock,
 {
 	struct sk_buff *skb;
 	struct sock *newsk;
+	ax25_dev *ax25_dev;
 	DEFINE_WAIT(wait);
 	struct sock *sk;
+	ax25_cb *ax25;
 	int err = 0;
 
 	if (sock->state != SS_UNCONNECTED)
@@ -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

Re: [PATCH v5] ax25: Fix refcount imbalance on inbound connections

[Dan Carpenter]

On Wed, May 29, 2024 at 05:02:43PM -0400, lars@oddbit.com wrote:
> From: Lars Kellogg-Stedman <lars@oddbit.com>
> 
> 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 will start with one of the
> following errors:
> 
>     refcount_t: decrement hit 0; leaking memory.
>     refcount_t: underflow; use-after-free.
> 
> And will then have a trace like:
> 
>     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(). This makes the
> logic leading to ax25_accept() match the logic for ax25_bind(): in both
> cases we increment the refcount, which is ultimately decremented in
> ax25_release().
> 
> Fixes: 9fd75b66b8f6 ("ax25: Fix refcount leaks caused by ax25_cb_del()")
> Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
> Tested-by: Duoming Zhou <duoming@zju.edu.cn>
> Tested-by: Dan Cross <crossd@gmail.com>
> Tested-by: Chris Maness <christopher.maness@gmail.com>
> ---

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v5] ax25: Fix refcount imbalance on inbound connections

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 May 2024 17:02:43 -0400 you wrote:
> From: Lars Kellogg-Stedman <lars@oddbit.com>
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [v5] ax25: Fix refcount imbalance on inbound connections
    https://git.kernel.org/netdev/net/c/3c34fb0bd4a4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v5] ax25: Fix refcount imbalance on inbound connections

[Chris Maness]

Is this the only patch to get the last stable branch off of the
mainline (6.9) up to date?

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 8077cf2ee4480..d6f9fae06a9d8 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1378,8 +1378,10 @@ static int ax25_accept(struct socket *sock,
struct socket *newsock,
{
struct sk_buff *skb;
struct sock *newsk;
+ ax25_dev *ax25_dev;
DEFINE_WAIT(wait);
struct sock *sk;
+ ax25_cb *ax25;
int err = 0;
if (sock->state != SS_UNCONNECTED)
@@ -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);

#########################################

and I am going to guess that the next stable fork will have this
commit already applied?

Thanks in advance,
Chris KQ6UP

On Sat, Jun 1, 2024 at 4:10 PM Chris Maness
<christopher.maness@gmail.com> wrote:
>
> Awesome!
>
> Thanks,
> Chris Maness
> -Sent from my iPhone
>
>
> On Sat, Jun 1, 2024 at 4:00 PM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>
>> Hello:
>>
>> This patch was applied to netdev/net.git (main)
>> by Jakub Kicinski <kuba@kernel.org>:
>>
>> On Wed, 29 May 2024 17:02:43 -0400 you wrote:
>> > From: Lars Kellogg-Stedman <lars@oddbit.com>
>> >
>> > 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.
>> >
>> > [...]
>>
>> Here is the summary with links:
>>   - [v5] ax25: Fix refcount imbalance on inbound connections
>>     https://git.kernel.org/netdev/net/c/3c34fb0bd4a4
>>
>> You are awesome, thank you!
>> --
>> Deet-doot-dot, I am a bot.
>> https://korg.docs.kernel.org/patchwork/pwbot.html
>>
>>


-- 
Thanks,
Chris Maness