diff mbox series

[net,2/2] ax25: fix potential reference counting leak in ax25_addr_ax25dev

Message ID cb44ea91c0b7084079c3086d6d75e7984505cec7.1714660565.git.duoming@zju.edu.cn (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ax25: fix reference counting issue of ax25_dev | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-05-02--18-00 (tests: 1000)

Commit Message

Duoming Zhou May 2, 2024, 2:43 p.m. UTC 
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(-)

Comments

Lars Kellogg-Stedman May 2, 2024, 6:55 p.m. UTC | #1 
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.
Dan Carpenter May 2, 2024, 7:56 p.m. UTC | #2 
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
diff mbox series

Patch

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;