Message-ID: <CAA5qM4Cp6LxgfF+3pgFoRO25QGtjDiEF5LyLtJEqYLXKm-W0jQ@mail.gmail.com>
Date:   Mon, 8 Mar 2021 13:20:05 -0500
From:   Tong Zhang <ztong0001@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Chas Williams <3chas3@...il.com>,
        linux-atm-general@...ts.sourceforge.net,
        Netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] fix a couple of atm->phy_data related issues

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

Powered by blists - more mailing lists