Re: [PATCH 0/3] fix a couple of atm->phy_data related issues

From: Tong Zhang
Date: Mon Mar 08 2021 - 13:20:55 EST

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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 8, 2021 at 12:39 AM Tong Zhang <ztong0001@xxxxxxxxx> 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?