linux-kernel - e1000e: can TIMINCA register be zero?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <57160BF8.6020001@redhat.com>
Date:	Tue, 19 Apr 2016 12:44:08 +0200
From:	Denys Vlasenko <dvlasenk@...hat.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Shannon Nelson <shannon.nelson@...el.com>,
	Carolyn Wyborny <carolyn.wyborny@...el.com>,
	Don Skidmore <donald.c.skidmore@...el.com>,
	Bruce Allan <bruce.w.allan@...el.com>,
	John Ronciak <john.ronciak@...el.com>,
	Mitch Williams <mitch.a.williams@...el.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: e1000e: can TIMINCA register be zero?

Hello,

I have a user report of division by zero in e1000e_cyclecounter_read+0xd9/0x100
at modprobe:

 [<ffffffff810b3c24>] timecounter_init+0x24/0x40
 [<ffffffffa048db34>] e1000e_config_hwtstamp+0x1c4/0x2e0 [e1000e]
 [<ffffffffa048ee55>] e1000e_reset+0x1c5/0x7a0 [e1000e]
 [<ffffffffa0496228>] e1000_probe+0xa2f/0xc7e [e1000e]
 [<ffffffff812befc7>] local_pci_probe+0x17/0x20
 [<ffffffff812c01b1>] pci_device_probe+0x101/0x120
 [<ffffffff81380c22>] ? driver_sysfs_add+0x62/0x90
 [<ffffffff81380eca>] driver_probe_device+0xaa/0x3a0
 [<ffffffff8138126b>] __driver_attach+0xab/0xb0
 [<ffffffff813811c0>] ? __driver_attach+0x0/0xb0
 [<ffffffff813800b4>] bus_for_each_dev+0x64/0x90
 [<ffffffff81380b5e>] driver_attach+0x1e/0x20
 [<ffffffff8137f8c8>] bus_add_driver+0x1e8/0x2b0
 [<ffffffff8138147f>] driver_register+0x5f/0xe0
 [<ffffffff812c0416>] __pci_register_driver+0x56/0xd0
 [<ffffffffa04ad000>] ? e1000_init_module+0x0/0x43 [e1000e]
 [<ffffffffa04ad041>] e1000_init_module+0x41/0x43 [e1000e]
 [<ffffffff810020d0>] do_one_initcall+0xc0/0x280
 [<ffffffff810c85d1>] sys_init_module+0xe1/0x250
 [<ffffffff8100b0d2>] system_call_fastpath+0x16/0x1b

User says it happens on hotplug.

On code inspection, this is clearly a case of
er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK == 0:

                /* errata for 82574/82583 possible bad bits read from SYSTIMH/L
                 * check to see that the time is incrementing at a reasonable
                 * rate and is a multiple of incvalue
                 */
==>             incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
                for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
                        /* latch SYSTIMH on read of SYSTIML */
                        systim_next = (cycle_t)er32(SYSTIML);
                        systim_next |= (cycle_t)er32(SYSTIMH) << 32;

                        time_delta = systim_next - systim;
                        temp = time_delta;
====>                   rem = do_div(temp, incvalue);

                        systim = systim_next;

                        if ((time_delta < E1000_82574_SYSTIM_EPSILON) &&
                            (rem == 0))
                                break;
                }

Knowing nothing about e1000e, I can easily slap on a quick fix here:

		rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);


However, I would like to alert you guys that this was seen.


Would zero counter increment in er32(TIMINCA) cause problems elsewhere?
In 1000e_config_hwtstamp(), it is initialized before timecounter_init():

        /* Get and set the System Time Register SYSTIM base frequency */
        ret_val = e1000e_get_base_timinca(adapter, &regval);
        if (ret_val)
                return ret_val;
==>     ew32(TIMINCA, regval);

        /* reset the ns time counter */
==>     timecounter_init(&adapter->tc, &adapter->cc,
                         ktime_to_ns(ktime_get_real()));

By code inspection, e1000e_get_base_timinca() either returns -EINVAL
and we don't do timecounter_init() and the division/0 location
is not reached, or e1000e_get_base_timinca(&regval) sets
nonzero regval. Then we set TIMINCA to this nonzero value.

Isn't it fishy that then timecounter_init() -> e1000e_cyclecounter_read()
-> er32(TIMINCA) sees zero there?