| Message ID | 20201031143619.7086-1-sbauer@blackbox.su |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fix for potential NULL pointer dereference with bare lan743x | expand |
> > Signed-off-by: Sergej Bauer <sbauer@blackbox.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.
On Sun, Nov 01, 2020 at 10:54:38PM +0300, Sergej Bauer wrote: > > > Signed-off-by: Sergej Bauer <sbauer@blackbox.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. > > > @@ -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; -ENETDOWN would be better, it gives a hit that WoL can be configured when the interface is configured up. Andrew
On Sunday, November 1, 2020 11:38:20 PM MSK Andrew Lunn wrote: > On Sun, Nov 01, 2020 at 10:54:38PM +0300, Sergej Bauer wrote: > > > > Signed-off-by: Sergej Bauer <sbauer@blackbox.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. > > > > > > @@ -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; > > -ENETDOWN would be better, it gives a hit that WoL can be configured > when the interface is configured up. > > Andrew Ok, thank you, Andrew! I was doubted in the correctness of returning -EIO. Regards, Sergej.
diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c index dcde496da7fb..ad38fc9e1468 100644 --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c @@ -780,7 +780,9 @@ static void lan743x_ethtool_get_wol(struct net_device *netdev, wol->supported = 0; wol->wolopts = 0; - phy_ethtool_get_wol(netdev->phydev, wol); + + if (netdev->phydev) + phy_ethtool_get_wol(netdev->phydev, wol); wol->supported |= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST | WAKE_MAGIC | WAKE_PHY | WAKE_ARP; @@ -792,6 +794,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) { struct lan743x_adapter *adapter = netdev_priv(netdev); + int ret; adapter->wolopts = 0; if (wol->wolopts & WAKE_UCAST) @@ -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 */
Signed-off-by: Sergej Bauer <sbauer@blackbox.su> --- drivers/net/ethernet/microchip/lan743x_ethtool.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)