* [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
* 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).