netdev - Re: [PATCH v2] lan743x: Fix for potential null pointer dereference

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]

Date:   Sun, 01 Nov 2020 22:54:38 +0300
From:   Sergej Bauer <sbauer@...ckbox.su>
To:     Markus Elfring <Markus.Elfring@....de>, netdev@...r.kernel.org
Cc:     UNGLinuxDriver@...rochip.com, linux-kernel@...r.kernel.org,
        Bryan Whitehead <bryan.whitehead@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2] lan743x: Fix for potential null pointer dereference

> > Signed-off-by: Sergej Bauer <sbauer@...ckbox.su>
> 
> * I miss a change description here.
The reason for the fix is when the device is down netdev->phydev will be NULL 
and there is no checking for this situation. So 'ethtool ethN' leads to kernel 
panic.

$ sudo ethtool eth7

[  103.510336] BUG: kernel NULL pointer dereference, address: 0000000000000340
[  103.510454] #PF: supervisor read access in kernel mode
[  103.510530] #PF: error_code(0x0000) - not-present page
[  103.510600] PGD 0 P4D 0 
[  103.510635] Oops: 0000 [#1] SMP PTI
[  103.510675] CPU: 1 PID: 7182 Comm: ethtool Not tainted 5.9.0upstream+ #5
[  103.510737] Hardware name: Gigabyte Technology Co., Ltd. H110-D3/H110-D3-
CF, BIOS F24 04/11/2018
[  103.510836] RIP: 0010:phy_ethtool_get_wol+0x5/0x30 [libphy]
[  103.510892] Code: 00 48 85 c0 74 11 48 8b 80 40 01 00 00 48 85 c0 74 05 e9 
8e 7a 6f dd b8 a1 ff ff ff c3 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 <48> 8b 87 
40 03 00 00 48 85 c0 74 11 48 8b 80 48 01 00 00 48 85 c0
[  103.511054] RSP: 0018:ffffb6cd85123cf0 EFLAGS: 00010286
[  103.511106] RAX: ffffffffc03f0d00 RBX: ffffb6cd85123d90 RCX: ffffffff9e6fdd20
[  103.511171] RDX: 0000000000000001 RSI: ffffb6cd85123d90 RDI: 0000000000000000
[  103.511237] RBP: ffff946f811b4000 R08: 0000000000001000 R09: 0000000000000000
[  103.511302] R10: 0000000000000000 R11: 0000000000000089 R12: 
00007ffde92be040
[  103.511367] R13: 0000000000000005 R14: ffff946f811b4000 R15: 0000000000000000
[  103.511434] FS:  00007f54a9bc7740(0000) GS:ffff9470b6c80000(0000) knlGS:
0000000000000000
[  103.511508] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  103.511564] CR2: 0000000000000340 CR3: 000000011d366001 CR4: 
00000000003706e0
[  103.511629] Call Trace:
[  103.511666]  lan743x_ethtool_get_wol+0x21/0x40 [lan743x]
[  103.511724]  dev_ethtool+0x1507/0x29d0
[  103.511769]  ? avc_has_extended_perms+0x17f/0x440
[  103.511820]  ? tomoyo_init_request_info+0x84/0x90
[  103.511870]  ? tomoyo_path_number_perm+0x68/0x1e0
[  103.511919]  ? tty_insert_flip_string_fixed_flag+0x82/0xe0
[  103.511973]  ? inet_ioctl+0x187/0x1d0
[  103.512016]  dev_ioctl+0xb5/0x560
[  103.512055]  sock_do_ioctl+0xa0/0x140
[  103.512098]  sock_ioctl+0x2cb/0x3c0
[  103.512139]  __x64_sys_ioctl+0x84/0xc0
[  103.512183]  do_syscall_64+0x33/0x80
[  103.512224]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  103.512274] RIP: 0033:0x7f54a9cba427
[  103.512313] Code: 00 00 90 48 8b 05 69 aa 0c 00 64 c7 00 26 00 00 00 48 c7 
c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 
ff ff 73 01 c3 48 8b 0d 39 aa 0c 00 f7 d8 64 89 01 48
...
---
So changes - is just to check a pointer for NULL;

> * Should a prefix be specified in the patch subject?
> 
as far as I understand subject should be "[PATCH v2] lan743x: fix for potential 
NULL pointer dereference with bare lan743x"?

ok, I've got it.

> 
> …
> 
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> 
> …
> 
> > @@ -809,9 +812,12 @@ static int lan743x_ethtool_set_wol(struct net_device
> > *netdev,> 
> >  	device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
> > 
> > -	phy_ethtool_set_wol(netdev->phydev, wol);
> > +	if (netdev->phydev)
> > +		ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > +	else
> > +		ret = -EIO;
> > 
> > -	return 0;
> > +	return ret;
> > 
> >  }
> >  #endif /* CONFIG_PM */
> 
> How do you think about to use the following code variant?
> 
> +	return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol) : -EIO;
> 
It will be quite shorter, thanks.

> Regards,
> Markus

                Regards.
                        Sergej.