lkml.org 
[lkml]   [2020]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v2] lan743x: Fix for potential null pointer dereference
Date
> > 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.




\
 
 \ /
  Last update: 2020-11-01 20:55    [W:0.077 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site