* [PATCH] fix for potential NULL pointer dereference with bare lan743x @ 2020-10-29 0:28 Sergej Bauer 2020-10-31 0:03 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Sergej Bauer @ 2020-10-29 0:28 UTC (permalink / raw) Cc: sbauer, Bryan Whitehead, Microchip Linux Driver Support, David S. Miller, Jakub Kicinski, netdev, linux-kernel This is just a minor fix which prevents a kernel NULL pointer dereference when using phy-less lan743x. Signed-off-by: Sergej Bauer <sbauer@blackbox.su> --- drivers/net/ethernet/microchip/lan743x_ethtool.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c index dcde496da7fb..354d72d550f2 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; @@ -793,6 +795,9 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev, { struct lan743x_adapter *adapter = netdev_priv(netdev); + if (!netdev->phydev) + return -EIO; + adapter->wolopts = 0; if (wol->wolopts & WAKE_UCAST) adapter->wolopts |= WAKE_UCAST; -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fix for potential NULL pointer dereference with bare lan743x 2020-10-29 0:28 [PATCH] fix for potential NULL pointer dereference with bare lan743x Sergej Bauer @ 2020-10-31 0:03 ` Jakub Kicinski 2020-10-31 14:36 ` [PATCH v2] " Sergej Bauer 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2020-10-31 0:03 UTC (permalink / raw) To: Sergej Bauer Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller, netdev, linux-kernel On Thu, 29 Oct 2020 03:28:45 +0300 Sergej Bauer wrote: > This is just a minor fix which prevents a kernel NULL pointer > dereference when using phy-less lan743x. > > Signed-off-by: Sergej Bauer <sbauer@blackbox.su> I take you mean when the device is down netdev->phydev will be NULL? > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c > index dcde496da7fb..354d72d550f2 100644 > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c > @@ -793,6 +795,9 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev, > { > struct lan743x_adapter *adapter = netdev_priv(netdev); > > + if (!netdev->phydev) > + return -EIO; Does it make sense to just skip the phy_ethtool_set_wol() call instead? Also doesn't the wol configuration of the PHY get lost across an netdev up/down cycle in this driver? Should it be re-applied after phy is connected back? > adapter->wolopts = 0; > if (wol->wolopts & WAKE_UCAST) > adapter->wolopts |= WAKE_UCAST; ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] fix for potential NULL pointer dereference with bare lan743x 2020-10-31 0:03 ` Jakub Kicinski @ 2020-10-31 14:36 ` Sergej Bauer [not found] ` <dabea6fc-2f2d-7864-721b-3c950265f764@web.de> 0 siblings, 1 reply; 6+ messages in thread From: Sergej Bauer @ 2020-10-31 14:36 UTC (permalink / raw) To: kuba Cc: sbauer, Bryan Whitehead, Microchip Linux Driver Support, David S. Miller, netdev, linux-kernel Signed-off-by: Sergej Bauer <sbauer@blackbox.su> --- drivers/net/ethernet/microchip/lan743x_ethtool.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) 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 */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <dabea6fc-2f2d-7864-721b-3c950265f764@web.de>]
* Re: [PATCH v2] lan743x: Fix for potential null pointer dereference [not found] ` <dabea6fc-2f2d-7864-721b-3c950265f764@web.de> @ 2020-11-01 19:54 ` Sergej Bauer 2020-11-01 20:38 ` Andrew Lunn 0 siblings, 1 reply; 6+ messages in thread From: Sergej Bauer @ 2020-11-01 19:54 UTC (permalink / raw) To: Markus Elfring, netdev Cc: UNGLinuxDriver, linux-kernel, Bryan Whitehead, David S. Miller > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lan743x: Fix for potential null pointer dereference 2020-11-01 19:54 ` [PATCH v2] lan743x: Fix for potential null pointer dereference Sergej Bauer @ 2020-11-01 20:38 ` Andrew Lunn 2020-11-01 20:54 ` Sergej Bauer 0 siblings, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2020-11-01 20:38 UTC (permalink / raw) To: Sergej Bauer Cc: Markus Elfring, netdev, UNGLinuxDriver, linux-kernel, Bryan Whitehead, David S. Miller 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lan743x: Fix for potential null pointer dereference 2020-11-01 20:38 ` Andrew Lunn @ 2020-11-01 20:54 ` Sergej Bauer 0 siblings, 0 replies; 6+ messages in thread From: Sergej Bauer @ 2020-11-01 20:54 UTC (permalink / raw) To: Andrew Lunn Cc: Markus Elfring, netdev, UNGLinuxDriver, linux-kernel, Bryan Whitehead, David S. Miller 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-01 20:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-29 0:28 [PATCH] fix for potential NULL pointer dereference with bare lan743x Sergej Bauer
2020-10-31 0:03 ` Jakub Kicinski
2020-10-31 14:36 ` [PATCH v2] " Sergej Bauer
[not found] ` <dabea6fc-2f2d-7864-721b-3c950265f764@web.de>
2020-11-01 19:54 ` [PATCH v2] lan743x: Fix for potential null pointer dereference Sergej Bauer
2020-11-01 20:38 ` Andrew Lunn
2020-11-01 20:54 ` Sergej Bauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).