* [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev
@ 2024-05-02 14:43 Duoming Zhou
2024-05-02 14:43 ` [PATCH net 1/2] ax25: change kfree in ax25_dev_free to ax25_dev_free Duoming Zhou
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Duoming Zhou @ 2024-05-02 14:43 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, jreuter, davem, edumazet, kuba, pabeni,
dan.carpenter, Duoming Zhou
The first patch changes kfree in ax25_dev_free to ax25_dev_free,
because the ax25_dev is managed by reference counting.
The second patch fixes potential reference counting leak issue
in ax25_addr_ax25dev.
Duoming Zhou (2):
ax25: change kfree in ax25_dev_free to ax25_dev_free
ax25: fix potential reference counting leak in ax25_addr_ax25dev
net/ax25/ax25_dev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net 1/2] ax25: change kfree in ax25_dev_free to ax25_dev_free
2024-05-02 14:43 [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Duoming Zhou
@ 2024-05-02 14:43 ` Duoming Zhou
2024-05-02 14:43 ` [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev Duoming Zhou
2024-05-02 20:01 ` [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Markus Elfring
2 siblings, 0 replies; 6+ messages in thread
From: Duoming Zhou @ 2024-05-02 14:43 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, jreuter, davem, edumazet, kuba, pabeni,
dan.carpenter, Duoming Zhou
The ax25_dev is managed by reference counting, so it should not be
deallocated directly by kfree() in ax25_dev_free(), replace it with
ax25_dev_put() instead.
Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
net/ax25/ax25_dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 282ec581c07..07723095c60 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -208,7 +208,7 @@ void __exit ax25_dev_free(void)
s = ax25_dev;
netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev = ax25_dev->next;
- kfree(s);
+ ax25_dev_put(s);
}
ax25_dev_list = NULL;
spin_unlock_bh(&ax25_dev_lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev
2024-05-02 14:43 [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Duoming Zhou
2024-05-02 14:43 ` [PATCH net 1/2] ax25: change kfree in ax25_dev_free to ax25_dev_free Duoming Zhou
@ 2024-05-02 14:43 ` Duoming Zhou
2024-05-02 18:55 ` Lars Kellogg-Stedman
2024-05-02 19:56 ` Dan Carpenter
2024-05-02 20:01 ` [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Markus Elfring
2 siblings, 2 replies; 6+ messages in thread
From: Duoming Zhou @ 2024-05-02 14:43 UTC (permalink / raw)
To: linux-hams
Cc: netdev, linux-kernel, jreuter, davem, edumazet, kuba, pabeni,
dan.carpenter, Duoming Zhou
The reference counting of ax25_dev potentially increase more
than once in ax25_addr_ax25dev(), which will cause memory leak.
In order to fix the above issue, only increase the reference
counting of ax25_dev once, when the res is not null.
Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
net/ax25/ax25_dev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 07723095c60..945af92a7b6 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -37,8 +37,9 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
res = ax25_dev;
- ax25_dev_hold(ax25_dev);
}
+ if (res)
+ ax25_dev_hold(ax25_dev);
spin_unlock_bh(&ax25_dev_lock);
return res;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev
2024-05-02 14:43 ` [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev Duoming Zhou
@ 2024-05-02 18:55 ` Lars Kellogg-Stedman
2024-05-02 19:56 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Lars Kellogg-Stedman @ 2024-05-02 18:55 UTC (permalink / raw)
To: Duoming Zhou
Cc: linux-hams, netdev, linux-kernel, jreuter, davem, edumazet, kuba,
pabeni, dan.carpenter
On Thu, May 02, 2024 at 10:43:38PM +0800, Duoming Zhou wrote:
> The reference counting of ax25_dev potentially increase more
> than once in ax25_addr_ax25dev(), which will cause memory leak.
With this patch applied, I see a kernel panic as soon as something binds
an ax.25 socket (e.g., starting ax25d):
BUG: kernel NULL pointer dereference, address: 0000000000000098
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 0 P4D 0
Oops: 0002 [#1] SMP PTI
CPU: 0 PID: 111 Comm: ax25d Not tainted 6.9.0-rc6-ax25+ #69
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:ax25_addr_ax25dev+0x5b/0xb0
Code: 8b 43 08 4c 89 ef 48 8b b0 d0 03 00 00 e8 6d fb ff ff 85 c0 4c 0f 44 e3 48 8b 1b 48 85 db 75 df 4d 85 e4 74 19 b8 01 00 00 00 <f0> 0f c1 04 25 98 00 00 00 85 c0 74 21 8d 50 01 09 c2 78 2b 48 c7
RSP: 0018:ffffc90000463e00 EFLAGS: 00010282
RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff888101a5f728 RDI: ffffc90000463e6a
RBP: ffffc90000463e18 R08: ffffc90000463e68 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff888101523e40
R13: ffffc90000463e6a R14: ffff88810111f200 R15: ffff8881004e4e00
FS: 00007fb6a6d93b08(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000098 CR3: 0000000101bb4000 CR4: 00000000000006b0
Call Trace:
<TASK>
? show_regs.part.0+0x22/0x30
? __die+0x5b/0x99
? page_fault_oops+0xae/0x220
? search_extable+0x2e/0x40
? ax25_addr_ax25dev+0x5b/0xb0
? kernelmode_fixup_or_oops+0x9f/0x120
? __bad_area_nosemaphore+0x15f/0x1a0
? bad_area_nosemaphore+0x16/0x20
? exc_page_fault+0x2a8/0x6e0
? asm_exc_page_fault+0x2b/0x30
? ax25_addr_ax25dev+0x5b/0xb0
ax25_bind+0x14c/0x260
__sys_bind+0xc0/0xf0
? alloc_file_pseudo+0xae/0xe0
__x64_sys_bind+0x1c/0x30
x64_sys_call+0xfe3/0x1d00
do_syscall_64+0x55/0x120
entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7fb6a6d2cfa4
Code: 00 00 00 ba 01 00 00 00 0f 05 80 e7 08 74 c3 eb b0 48 83 ec 08 89 d2 48 63 ff 45 31 d2 45 31 c0 45 31 c9 b8 31 00 00 00 0f 05 <48> 89 c7 e8 da 3f fe ff 48 83 c4 08 c3 48 83 ec 10 48 89 f0 89 d1
RSP: 002b:00007ffc1a5c7670 EFLAGS: 00000246 ORIG_RAX: 0000000000000031
RAX: ffffffffffffffda RBX: 00007ffc1a5c7799 RCX: 00007fb6a6d2cfa4
RDX: 0000000000000048 RSI: 00007ffc1a5c7750 RDI: 0000000000000005
RBP: 00007fb6a6d94a80 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb6a6cebca0
R13: 0000000000000048 R14: 0000561ff9f9e776 R15: 00007ffc1a5c8b00
</TASK>
CR2: 0000000000000098
---[ end trace 0000000000000000 ]---
RIP: 0010:ax25_addr_ax25dev+0x5b/0xb0
Code: 8b 43 08 4c 89 ef 48 8b b0 d0 03 00 00 e8 6d fb ff ff 85 c0 4c 0f 44 e3 48 8b 1b 48 85 db 75 df 4d 85 e4 74 19 b8 01 00 00 00 <f0> 0f c1 04 25 98 00 00 00 85 c0 74 21 8d 50 01 09 c2 78 2b 48 c7
RSP: 0018:ffffc90000463e00 EFLAGS: 00010282
RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff888101a5f728 RDI: ffffc90000463e6a
RBP: ffffc90000463e18 R08: ffffc90000463e68 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff888101523e40
R13: ffffc90000463e6a R14: ffff88810111f200 R15: ffff8881004e4e00
FS: 00007fb6a6d93b08(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000098 CR3: 0000000101bb4000 CR4: 00000000000006b0
Kernel panic - not syncing: Fatal exception in interrupt
Kernel Offset: disabled
---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
My kernel tree looks like this:
d32d77d2b4a (HEAD -> ham-patches) ax25: fix potential reference counting leak in ax25_addr_ax25dev
742ab0da09d ax25: change kfree in ax25_dev_free to ax25_dev_free
e6357cafe3e ax25: fix reference counting issue of ax25_dev
0106679839f (origin/master, origin/HEAD, master) Merge tag 'regulator-fix-v6.9-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/broonieer
I don't see the kernel panic if I discard the top patch.
--
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/ | N1LKS
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev
2024-05-02 14:43 ` [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev Duoming Zhou
2024-05-02 18:55 ` Lars Kellogg-Stedman
@ 2024-05-02 19:56 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2024-05-02 19:56 UTC (permalink / raw)
To: Duoming Zhou
Cc: linux-hams, netdev, linux-kernel, jreuter, davem, edumazet, kuba,
pabeni, Lars Kellogg-Stedman
On Thu, May 02, 2024 at 10:43:38PM +0800, Duoming Zhou wrote:
> The reference counting of ax25_dev potentially increase more
> than once in ax25_addr_ax25dev(), which will cause memory leak.
>
> In order to fix the above issue, only increase the reference
> counting of ax25_dev once, when the res is not null.
>
> Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> net/ax25/ax25_dev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index 07723095c60..945af92a7b6 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -37,8 +37,9 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
> for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
> if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
> res = ax25_dev;
> - ax25_dev_hold(ax25_dev);
> }
> + if (res)
> + ax25_dev_hold(ax25_dev);
^^^^^^^^
It should be ax25_dev_hold(res);
This is the NULL dereference that Lars saw. Thanks for testing, by the
way Lars.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev
2024-05-02 14:43 [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Duoming Zhou
2024-05-02 14:43 ` [PATCH net 1/2] ax25: change kfree in ax25_dev_free to ax25_dev_free Duoming Zhou
2024-05-02 14:43 ` [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev Duoming Zhou
@ 2024-05-02 20:01 ` Markus Elfring
2 siblings, 0 replies; 6+ messages in thread
From: Markus Elfring @ 2024-05-02 20:01 UTC (permalink / raw)
To: Duoming Zhou, linux-hams, netdev, kernel-janitors,
David S. Miller, Eric Dumazet, Jakub Kicinski, Jörg Reuter,
Paolo Abeni
Cc: LKML, Dan Carpenter, Lars Kellogg-Stedman
> The first patch changes kfree in ax25_dev_free to ax25_dev_free,
I find this description confusing.
Would you like to refer to a ax25_dev_put() call?
> because the ax25_dev is managed by reference counting.
How do you think about to link also to previous change approaches?
Regards,
Markus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-02 20:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-02 14:43 [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Duoming Zhou
2024-05-02 14:43 ` [PATCH net 1/2] ax25: change kfree in ax25_dev_free to ax25_dev_free Duoming Zhou
2024-05-02 14:43 ` [PATCH net 2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev Duoming Zhou
2024-05-02 18:55 ` Lars Kellogg-Stedman
2024-05-02 19:56 ` Dan Carpenter
2024-05-02 20:01 ` [PATCH net 0/2] ax25: fix reference counting issue of ax25_dev Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).