| Message ID | 20210308032529.435224-1-ztong0001@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix a couple of atm->phy_data related issues | expand |
On Mon, Mar 8, 2021 at 12:39 AM Tong Zhang <ztong0001@gmail.com> wrote: > > there are two drivers(zatm and idt77252) using PRIV() (i.e. atm->phy_data) > to store private data, but the driver happens to populate wrong > pointers: atm->dev_data. which actually cause null-ptr-dereference in > following PRIV(dev). This patch series attemps to fix those two issues > along with a typo in atm struct. > > Tong Zhang (3): > atm: fix a typo in the struct description > atm: uPD98402: fix incorrect allocation > atm: idt77252: fix null-ptr-dereference > > drivers/atm/idt77105.c | 4 ++-- > drivers/atm/uPD98402.c | 2 +- > include/linux/atmdev.h | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) For the 2 phys you actually seen null pointer dereferences or are your changes based on just code review? I ask because it seems like this code has been this way since 2005 and in the case of uPD98402_start the code doesn't seem like it should function the way it was as PRIV is phy_data and there being issues seems pretty obvious since the initialization of things happens immediately after the allocation. I'm just wondering if it might make more sense to drop the code if it hasn't been run in 15+ years rather than updating it?
Hi Alex, attached is the kernel log for zatm(uPD98402) -- I also have idt77252's log -- which is similar to this one -- I think it makes sense to drop if no one is actually using it -- - Tong [ 5.740774] BUG: KASAN: null-ptr-deref in uPD98402_start+0x5e/0x219 [uPD98402] [ 5.741179] Write of size 4 at addr 000000000000002c by task modprobe/96 [ 5.741548] [ 5.741637] CPU: 0 PID: 96 Comm: modprobe Not tainted 5.12.0-rc2-dirty #71 [ 5.742017] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 [ 5.742635] Call Trace: [ 5.742775] dump_stack+0x8a/0xb5 [ 5.742966] kasan_report.cold+0x10f/0x111 [ 5.743197] ? uPD98402_start+0x5e/0x219 [uPD98402] [ 5.743473] uPD98402_start+0x5e/0x219 [uPD98402] [ 5.743739] zatm_init_one+0x10b5/0x1311 [zatm] [ 5.743998] ? zatm_int.cold+0x30/0x30 [zatm] [ 5.744246] ? _raw_write_lock_irqsave+0xd0/0xd0 [ 5.744507] ? __mutex_lock_slowpath+0x10/0x10 [ 5.744757] ? _raw_spin_unlock_irqrestore+0xd/0x20 [ 5.745030] ? zatm_int.cold+0x30/0x30 [zatm] [ 5.745278] local_pci_probe+0x6f/0xb0 [ 5.745492] pci_device_probe+0x171/0x240 [ 5.745718] ? pci_device_remove+0xe0/0xe0 [ 5.745949] ? kernfs_create_link+0xb6/0x110 [ 5.746190] ? sysfs_do_create_link_sd.isra.0+0x76/0xe0 [ 5.746482] really_probe+0x161/0x420 [ 5.746691] driver_probe_device+0x6d/0xd0 [ 5.746923] device_driver_attach+0x82/0x90 [ 5.747158] ? device_driver_attach+0x90/0x90 [ 5.747402] __driver_attach+0x60/0x100 [ 5.747621] ? device_driver_attach+0x90/0x90 [ 5.747864] bus_for_each_dev+0xe1/0x140 [ 5.748075] ? subsys_dev_iter_exit+0x10/0x10 [ 5.748320] ? klist_node_init+0x61/0x80 [ 5.748542] bus_add_driver+0x254/0x2a0 [ 5.748760] driver_register+0xd3/0x150 [ 5.748977] ? 0xffffffffc0030000 [ 5.749163] do_one_initcall+0x84/0x250 [ 5.749380] ? trace_event_raw_event_initcall_finish+0x150/0x150 [ 5.749714] ? _raw_spin_unlock_irqrestore+0xd/0x20 [ 5.749987] ? create_object+0x395/0x510 [ 5.750210] ? kasan_unpoison+0x21/0x50 [ 5.750427] do_init_module+0xf8/0x350 [ 5.750640] load_module+0x40c5/0x4410 [ 5.750854] ? module_frob_arch_sections+0x20/0x20 [ 5.751123] ? kernel_read_file+0x1cd/0x3e0 [ 5.751364] ? __do_sys_finit_module+0x108/0x170 [ 5.751628] __do_sys_finit_module+0x108/0x170 [ 5.751879] ? __ia32_sys_init_module+0x40/0x40 [ 5.752126] ? file_open_root+0x200/0x200 [ 5.752353] ? do_sys_open+0x85/0xe0 [ 5.752556] ? filp_open+0x50/0x50 [ 5.752750] ? fpregs_assert_state_consistent+0x4d/0x60 [ 5.753042] ? exit_to_user_mode_prepare+0x2f/0x130 [ 5.753316] do_syscall_64+0x33/0x40 [ 5.753519] entry_SYSCALL_64_after_hwframe+0x44/0xae [ 5.753802] RIP: 0033:0x7ff64032dcf7 ff c3 48 c7 c6 01 00 00 00 e9 a1 [ 5.755029] RSP: 002b:00007ffd250ea358 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 5.755449] RAX: ffffffffffffffda RBX: 0000000001093a70 RCX: 00007ff64032dcf7 [ 5.755847] RDX: 0000000000000000 RSI: 00000000010929e0 RDI: 0000000000000003 [ 5.756242] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000001 [ 5.756635] R10: 00007ff640391300 R11: 0000000000000246 R12: 00000000010929e0 [ 5.757029] R13: 0000000000000000 R14: 0000000001092dd0 R15: 0000000000000001 On Mon, Mar 8, 2021 at 12:47 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Mon, Mar 8, 2021 at 12:39 AM Tong Zhang <ztong0001@gmail.com> wrote: > > > > there are two drivers(zatm and idt77252) using PRIV() (i.e. atm->phy_data) > > to store private data, but the driver happens to populate wrong > > pointers: atm->dev_data. which actually cause null-ptr-dereference in > > following PRIV(dev). This patch series attemps to fix those two issues > > along with a typo in atm struct. > > > > Tong Zhang (3): > > atm: fix a typo in the struct description > > atm: uPD98402: fix incorrect allocation > > atm: idt77252: fix null-ptr-dereference > > > > drivers/atm/idt77105.c | 4 ++-- > > drivers/atm/uPD98402.c | 2 +- > > include/linux/atmdev.h | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > For the 2 phys you actually seen null pointer dereferences or are your > changes based on just code review? > > I ask because it seems like this code has been this way since 2005 and > in the case of uPD98402_start the code doesn't seem like it should > function the way it was as PRIV is phy_data and there being issues > seems pretty obvious since the initialization of things happens > immediately after the allocation. > > I'm just wondering if it might make more sense to drop the code if it > hasn't been run in 15+ years rather than updating it?
Hi Tong, Is this direct-assigned hardware or is QEMU being used to emulate the hardware here? Admittedly I don't know that much about ATM, so I am not sure when/if those phys would have gone out of production. However since the code dates back to 2005 I am guessing it is on the old side. Ultimately the decision is up to Chas. However if there has been code in place for this long that would trigger this kind of null pointer dereference then it kind of points to the fact that those phys have probably not been in use since at least back when Linus switched over to git in 2005. Thanks, - Alex On Mon, Mar 8, 2021 at 9:55 AM Tong Zhang <ztong0001@gmail.com> wrote: > > Hi Alex, > attached is the kernel log for zatm(uPD98402) -- I also have > idt77252's log -- which is similar to this one -- > I think it makes sense to drop if no one is actually using it -- > - Tong > > [ 5.740774] BUG: KASAN: null-ptr-deref in uPD98402_start+0x5e/0x219 > [uPD98402] > [ 5.741179] Write of size 4 at addr 000000000000002c by task modprobe/96 > [ 5.741548] > [ 5.741637] CPU: 0 PID: 96 Comm: modprobe Not tainted 5.12.0-rc2-dirty #71 > [ 5.742017] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), > BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 > [ 5.742635] Call Trace: > [ 5.742775] dump_stack+0x8a/0xb5 > [ 5.742966] kasan_report.cold+0x10f/0x111 > [ 5.743197] ? uPD98402_start+0x5e/0x219 [uPD98402] > [ 5.743473] uPD98402_start+0x5e/0x219 [uPD98402] > [ 5.743739] zatm_init_one+0x10b5/0x1311 [zatm] > [ 5.743998] ? zatm_int.cold+0x30/0x30 [zatm] > [ 5.744246] ? _raw_write_lock_irqsave+0xd0/0xd0 > [ 5.744507] ? __mutex_lock_slowpath+0x10/0x10 > [ 5.744757] ? _raw_spin_unlock_irqrestore+0xd/0x20 > [ 5.745030] ? zatm_int.cold+0x30/0x30 [zatm] > [ 5.745278] local_pci_probe+0x6f/0xb0 > [ 5.745492] pci_device_probe+0x171/0x240 > [ 5.745718] ? pci_device_remove+0xe0/0xe0 > [ 5.745949] ? kernfs_create_link+0xb6/0x110 > [ 5.746190] ? sysfs_do_create_link_sd.isra.0+0x76/0xe0 > [ 5.746482] really_probe+0x161/0x420 > [ 5.746691] driver_probe_device+0x6d/0xd0 > [ 5.746923] device_driver_attach+0x82/0x90 > [ 5.747158] ? device_driver_attach+0x90/0x90 > [ 5.747402] __driver_attach+0x60/0x100 > [ 5.747621] ? device_driver_attach+0x90/0x90 > [ 5.747864] bus_for_each_dev+0xe1/0x140 > [ 5.748075] ? subsys_dev_iter_exit+0x10/0x10 > [ 5.748320] ? klist_node_init+0x61/0x80 > [ 5.748542] bus_add_driver+0x254/0x2a0 > [ 5.748760] driver_register+0xd3/0x150 > [ 5.748977] ? 0xffffffffc0030000 > [ 5.749163] do_one_initcall+0x84/0x250 > [ 5.749380] ? trace_event_raw_event_initcall_finish+0x150/0x150 > [ 5.749714] ? _raw_spin_unlock_irqrestore+0xd/0x20 > [ 5.749987] ? create_object+0x395/0x510 > [ 5.750210] ? kasan_unpoison+0x21/0x50 > [ 5.750427] do_init_module+0xf8/0x350 > [ 5.750640] load_module+0x40c5/0x4410 > [ 5.750854] ? module_frob_arch_sections+0x20/0x20 > [ 5.751123] ? kernel_read_file+0x1cd/0x3e0 > [ 5.751364] ? __do_sys_finit_module+0x108/0x170 > [ 5.751628] __do_sys_finit_module+0x108/0x170 > [ 5.751879] ? __ia32_sys_init_module+0x40/0x40 > [ 5.752126] ? file_open_root+0x200/0x200 > [ 5.752353] ? do_sys_open+0x85/0xe0 > [ 5.752556] ? filp_open+0x50/0x50 > [ 5.752750] ? fpregs_assert_state_consistent+0x4d/0x60 > [ 5.753042] ? exit_to_user_mode_prepare+0x2f/0x130 > [ 5.753316] do_syscall_64+0x33/0x40 > [ 5.753519] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 5.753802] RIP: 0033:0x7ff64032dcf7 > ff c3 48 c7 c6 01 00 00 00 e9 a1 > [ 5.755029] RSP: 002b:00007ffd250ea358 EFLAGS: 00000246 ORIG_RAX: > 0000000000000139 > [ 5.755449] RAX: ffffffffffffffda RBX: 0000000001093a70 RCX: 00007ff64032dcf7 > [ 5.755847] RDX: 0000000000000000 RSI: 00000000010929e0 RDI: 0000000000000003 > [ 5.756242] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000001 > [ 5.756635] R10: 00007ff640391300 R11: 0000000000000246 R12: 00000000010929e0 > [ 5.757029] R13: 0000000000000000 R14: 0000000001092dd0 R15: 0000000000000001 > > On Mon, Mar 8, 2021 at 12:47 PM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Mon, Mar 8, 2021 at 12:39 AM Tong Zhang <ztong0001@gmail.com> wrote: > > > > > > there are two drivers(zatm and idt77252) using PRIV() (i.e. atm->phy_data) > > > to store private data, but the driver happens to populate wrong > > > pointers: atm->dev_data. which actually cause null-ptr-dereference in > > > following PRIV(dev). This patch series attemps to fix those two issues > > > along with a typo in atm struct. > > > > > > Tong Zhang (3): > > > atm: fix a typo in the struct description > > > atm: uPD98402: fix incorrect allocation > > > atm: idt77252: fix null-ptr-dereference > > > > > > drivers/atm/idt77105.c | 4 ++-- > > > drivers/atm/uPD98402.c | 2 +- > > > include/linux/atmdev.h | 2 +- > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > For the 2 phys you actually seen null pointer dereferences or are your > > changes based on just code review? > > > > I ask because it seems like this code has been this way since 2005 and > > in the case of uPD98402_start the code doesn't seem like it should > > function the way it was as PRIV is phy_data and there being issues > > seems pretty obvious since the initialization of things happens > > immediately after the allocation. > > > > I'm just wondering if it might make more sense to drop the code if it > > hasn't been run in 15+ years rather than updating it?
I have this emulated device in QEMU, -- and I agree with you that probably no one has been using it for a while IMHO, given the quality of the driver it also make sense to drop the support completely or we at least need to fix some obvious issues here. Best, - Tong On Mon, Mar 8, 2021 at 1:06 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > Hi Tong, > > Is this direct-assigned hardware or is QEMU being used to emulate the > hardware here? Admittedly I don't know that much about ATM, so I am > not sure when/if those phys would have gone out of production. However > since the code dates back to 2005 I am guessing it is on the old side. > > Ultimately the decision is up to Chas. However if there has been code > in place for this long that would trigger this kind of null pointer > dereference then it kind of points to the fact that those phys have > probably not been in use since at least back when Linus switched over > to git in 2005. > > Thanks, > > - Alex > > On Mon, Mar 8, 2021 at 9:55 AM Tong Zhang <ztong0001@gmail.com> wrote: > > > > Hi Alex, > > attached is the kernel log for zatm(uPD98402) -- I also have > > idt77252's log -- which is similar to this one -- > > I think it makes sense to drop if no one is actually using it -- > > - Tong > > > > [ 5.740774] BUG: KASAN: null-ptr-deref in uPD98402_start+0x5e/0x219 > > [uPD98402] > > [ 5.741179] Write of size 4 at addr 000000000000002c by task modprobe/96 > > [ 5.741548] > > [ 5.741637] CPU: 0 PID: 96 Comm: modprobe Not tainted 5.12.0-rc2-dirty #71 > > [ 5.742017] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), > > BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 > > [ 5.742635] Call Trace: > > [ 5.742775] dump_stack+0x8a/0xb5 > > [ 5.742966] kasan_report.cold+0x10f/0x111 > > [ 5.743197] ? uPD98402_start+0x5e/0x219 [uPD98402] > > [ 5.743473] uPD98402_start+0x5e/0x219 [uPD98402] > > [ 5.743739] zatm_init_one+0x10b5/0x1311 [zatm] > > [ 5.743998] ? zatm_int.cold+0x30/0x30 [zatm] > > [ 5.744246] ? _raw_write_lock_irqsave+0xd0/0xd0 > > [ 5.744507] ? __mutex_lock_slowpath+0x10/0x10 > > [ 5.744757] ? _raw_spin_unlock_irqrestore+0xd/0x20 > > [ 5.745030] ? zatm_int.cold+0x30/0x30 [zatm] > > [ 5.745278] local_pci_probe+0x6f/0xb0 > > [ 5.745492] pci_device_probe+0x171/0x240 > > [ 5.745718] ? pci_device_remove+0xe0/0xe0 > > [ 5.745949] ? kernfs_create_link+0xb6/0x110 > > [ 5.746190] ? sysfs_do_create_link_sd.isra.0+0x76/0xe0 > > [ 5.746482] really_probe+0x161/0x420 > > [ 5.746691] driver_probe_device+0x6d/0xd0 > > [ 5.746923] device_driver_attach+0x82/0x90 > > [ 5.747158] ? device_driver_attach+0x90/0x90 > > [ 5.747402] __driver_attach+0x60/0x100 > > [ 5.747621] ? device_driver_attach+0x90/0x90 > > [ 5.747864] bus_for_each_dev+0xe1/0x140 > > [ 5.748075] ? subsys_dev_iter_exit+0x10/0x10 > > [ 5.748320] ? klist_node_init+0x61/0x80 > > [ 5.748542] bus_add_driver+0x254/0x2a0 > > [ 5.748760] driver_register+0xd3/0x150 > > [ 5.748977] ? 0xffffffffc0030000 > > [ 5.749163] do_one_initcall+0x84/0x250 > > [ 5.749380] ? trace_event_raw_event_initcall_finish+0x150/0x150 > > [ 5.749714] ? _raw_spin_unlock_irqrestore+0xd/0x20 > > [ 5.749987] ? create_object+0x395/0x510 > > [ 5.750210] ? kasan_unpoison+0x21/0x50 > > [ 5.750427] do_init_module+0xf8/0x350 > > [ 5.750640] load_module+0x40c5/0x4410 > > [ 5.750854] ? module_frob_arch_sections+0x20/0x20 > > [ 5.751123] ? kernel_read_file+0x1cd/0x3e0 > > [ 5.751364] ? __do_sys_finit_module+0x108/0x170 > > [ 5.751628] __do_sys_finit_module+0x108/0x170 > > [ 5.751879] ? __ia32_sys_init_module+0x40/0x40 > > [ 5.752126] ? file_open_root+0x200/0x200 > > [ 5.752353] ? do_sys_open+0x85/0xe0 > > [ 5.752556] ? filp_open+0x50/0x50 > > [ 5.752750] ? fpregs_assert_state_consistent+0x4d/0x60 > > [ 5.753042] ? exit_to_user_mode_prepare+0x2f/0x130 > > [ 5.753316] do_syscall_64+0x33/0x40 > > [ 5.753519] entry_SYSCALL_64_after_hwframe+0x44/0xae > > [ 5.753802] RIP: 0033:0x7ff64032dcf7 > > ff c3 48 c7 c6 01 00 00 00 e9 a1 > > [ 5.755029] RSP: 002b:00007ffd250ea358 EFLAGS: 00000246 ORIG_RAX: > > 0000000000000139 > > [ 5.755449] RAX: ffffffffffffffda RBX: 0000000001093a70 RCX: 00007ff64032dcf7 > > [ 5.755847] RDX: 0000000000000000 RSI: 00000000010929e0 RDI: 0000000000000003 > > [ 5.756242] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000001 > > [ 5.756635] R10: 00007ff640391300 R11: 0000000000000246 R12: 00000000010929e0 > > [ 5.757029] R13: 0000000000000000 R14: 0000000001092dd0 R15: 0000000000000001 > > > > On Mon, Mar 8, 2021 at 12:47 PM Alexander Duyck > > <alexander.duyck@gmail.com> wrote: > > > > > > On Mon, Mar 8, 2021 at 12:39 AM Tong Zhang <ztong0001@gmail.com> wrote: > > > > > > > > there are two drivers(zatm and idt77252) using PRIV() (i.e. atm->phy_data) > > > > to store private data, but the driver happens to populate wrong > > > > pointers: atm->dev_data. which actually cause null-ptr-dereference in > > > > following PRIV(dev). This patch series attemps to fix those two issues > > > > along with a typo in atm struct. > > > > > > > > Tong Zhang (3): > > > > atm: fix a typo in the struct description > > > > atm: uPD98402: fix incorrect allocation > > > > atm: idt77252: fix null-ptr-dereference > > > > > > > > drivers/atm/idt77105.c | 4 ++-- > > > > drivers/atm/uPD98402.c | 2 +- > > > > include/linux/atmdev.h | 2 +- > > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > > > For the 2 phys you actually seen null pointer dereferences or are your > > > changes based on just code review? > > > > > > I ask because it seems like this code has been this way since 2005 and > > > in the case of uPD98402_start the code doesn't seem like it should > > > function the way it was as PRIV is phy_data and there being issues > > > seems pretty obvious since the initialization of things happens > > > immediately after the allocation. > > > > > > I'm just wondering if it might make more sense to drop the code if it > > > hasn't been run in 15+ years rather than updating it?
Hello: This series was applied to netdev/net.git (refs/heads/master): On Sun, 7 Mar 2021 22:25:27 -0500 you wrote: > there are two drivers(zatm and idt77252) using PRIV() (i.e. atm->phy_data) > to store private data, but the driver happens to populate wrong > pointers: atm->dev_data. which actually cause null-ptr-dereference in > following PRIV(dev). This patch series attemps to fix those two issues > along with a typo in atm struct. > > Tong Zhang (3): > atm: fix a typo in the struct description > atm: uPD98402: fix incorrect allocation > atm: idt77252: fix null-ptr-dereference > > [...] Here is the summary with links: - [1/3] atm: fix a typo in the struct description https://git.kernel.org/netdev/net/c/1019d7923d9d - [2/3] atm: uPD98402: fix incorrect allocation https://git.kernel.org/netdev/net/c/3153724fc084 - [3/3] atm: idt77252: fix null-ptr-dereference https://git.kernel.org/netdev/net/c/4416e98594dc You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html