* [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops
@ 2018-09-14 23:01 Florian Fainelli
2018-09-15 7:32 ` Russell King - ARM Linux
0 siblings, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2018-09-14 23:01 UTC (permalink / raw)
To: netdev
Cc: rmk+kernel, Florian Fainelli, Andrew Lunn, David S. Miller, open list
In case we have specified a SFP and an I2C phandle in the Device Tree,
but we somehow failed to look up the I2C adapter (e.g: the driver is not
enabled), we will leave dangling socket_ops, and the sfp_bus will still
have been registered. We can then observe NULL pointer dereferences
while doing ethtool -m:
[ 20.218613] Unable to handle kernel NULL pointer dereference at virtual address 00000008
[ 20.226977] pgd = (ptrval)
[ 20.229785] [00000008] *pgd=7c400831, *pte=00000000, *ppte=00000000
[ 20.236274] Internal error: Oops: 17 [#1] SMP ARM
[ 20.241122] Modules linked in:
[ 20.244278] CPU: 0 PID: 1480 Comm: ethtool Not tainted 4.19.0-rc3 #138
[ 20.251013] Hardware name: Broadcom Northstar Plus SoC
[ 20.256316] PC is at sfp_get_module_info+0x8/0x10
[ 20.261172] LR is at dev_ethtool+0x218c/0x2afc
Specifically guard against that.
Fixes: ce0aa27ff3f6 ("sfp: add sfp-bus to bridge between network devices and sfp cages")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/sfp-bus.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 740655261e5b..9465be501372 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -378,6 +378,9 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
*/
int sfp_get_module_info(struct sfp_bus *bus, struct ethtool_modinfo *modinfo)
{
+ if (!bus->socket_ops)
+ return -EOPNOTSUPP;
+
return bus->socket_ops->module_info(bus->sfp, modinfo);
}
EXPORT_SYMBOL_GPL(sfp_get_module_info);
@@ -396,6 +399,9 @@ EXPORT_SYMBOL_GPL(sfp_get_module_info);
int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
u8 *data)
{
+ if (!bus->socket_ops)
+ return -EOPNOTSUPP;
+
return bus->socket_ops->module_eeprom(bus->sfp, ee, data);
}
EXPORT_SYMBOL_GPL(sfp_get_module_eeprom);
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops
2018-09-14 23:01 [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops Florian Fainelli
@ 2018-09-15 7:32 ` Russell King - ARM Linux
2018-09-17 17:30 ` Florian Fainelli
0 siblings, 1 reply; 4+ messages in thread
From: Russell King - ARM Linux @ 2018-09-15 7:32 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, Andrew Lunn, David S. Miller, open list
On Fri, Sep 14, 2018 at 04:01:31PM -0700, Florian Fainelli wrote:
> In case we have specified a SFP and an I2C phandle in the Device Tree,
> but we somehow failed to look up the I2C adapter (e.g: the driver is not
> enabled), we will leave dangling socket_ops, and the sfp_bus will still
> have been registered. We can then observe NULL pointer dereferences
> while doing ethtool -m:
>
> [ 20.218613] Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [ 20.226977] pgd = (ptrval)
> [ 20.229785] [00000008] *pgd=7c400831, *pte=00000000, *ppte=00000000
> [ 20.236274] Internal error: Oops: 17 [#1] SMP ARM
> [ 20.241122] Modules linked in:
> [ 20.244278] CPU: 0 PID: 1480 Comm: ethtool Not tainted 4.19.0-rc3 #138
> [ 20.251013] Hardware name: Broadcom Northstar Plus SoC
> [ 20.256316] PC is at sfp_get_module_info+0x8/0x10
> [ 20.261172] LR is at dev_ethtool+0x218c/0x2afc
>
> Specifically guard against that.
The other alternative to this would be to only set ndev->sfp_bus when
the bus moves to registered state, which would probably be a saner
alternative than to add an additional layer of tests. IOW:
drivers/net/phy/sfp-bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 48ca980f5c96..c4c2b5269216 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -362,6 +362,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
}
if (bus->started)
bus->socket_ops->start(bus->sfp);
+ bus->netdev->sfp_bus = bus;
bus->registered = true;
return 0;
}
@@ -370,6 +371,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
{
const struct sfp_upstream_ops *ops = bus->upstream_ops;
+ bus->netdev->sfp_bus = NULL;
if (bus->registered) {
if (bus->started)
bus->socket_ops->stop(bus->sfp);
@@ -451,7 +453,6 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
{
bus->upstream_ops = NULL;
bus->upstream = NULL;
- bus->netdev->sfp_bus = NULL;
bus->netdev = NULL;
}
@@ -480,7 +481,6 @@ struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
bus->upstream_ops = ops;
bus->upstream = upstream;
bus->netdev = ndev;
- ndev->sfp_bus = bus;
if (bus->sfp) {
ret = sfp_register_bus(bus);
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops
2018-09-15 7:32 ` Russell King - ARM Linux
@ 2018-09-17 17:30 ` Florian Fainelli
2018-09-17 23:13 ` Russell King - ARM Linux
0 siblings, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2018-09-17 17:30 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: netdev, Andrew Lunn, David S. Miller, open list
On 09/15/2018 12:32 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 14, 2018 at 04:01:31PM -0700, Florian Fainelli wrote:
>> In case we have specified a SFP and an I2C phandle in the Device Tree,
>> but we somehow failed to look up the I2C adapter (e.g: the driver is not
>> enabled), we will leave dangling socket_ops, and the sfp_bus will still
>> have been registered. We can then observe NULL pointer dereferences
>> while doing ethtool -m:
>>
>> [ 20.218613] Unable to handle kernel NULL pointer dereference at virtual address 00000008
>> [ 20.226977] pgd = (ptrval)
>> [ 20.229785] [00000008] *pgd=7c400831, *pte=00000000, *ppte=00000000
>> [ 20.236274] Internal error: Oops: 17 [#1] SMP ARM
>> [ 20.241122] Modules linked in:
>> [ 20.244278] CPU: 0 PID: 1480 Comm: ethtool Not tainted 4.19.0-rc3 #138
>> [ 20.251013] Hardware name: Broadcom Northstar Plus SoC
>> [ 20.256316] PC is at sfp_get_module_info+0x8/0x10
>> [ 20.261172] LR is at dev_ethtool+0x218c/0x2afc
>>
>> Specifically guard against that.
>
> The other alternative to this would be to only set ndev->sfp_bus when
> the bus moves to registered state, which would probably be a saner
> alternative than to add an additional layer of tests. IOW:
Indeed, your patch works for me:
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
>
> drivers/net/phy/sfp-bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 48ca980f5c96..c4c2b5269216 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -362,6 +362,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
> }
> if (bus->started)
> bus->socket_ops->start(bus->sfp);
> + bus->netdev->sfp_bus = bus;
> bus->registered = true;
> return 0;
> }
> @@ -370,6 +371,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
> {
> const struct sfp_upstream_ops *ops = bus->upstream_ops;
>
> + bus->netdev->sfp_bus = NULL;
> if (bus->registered) {
> if (bus->started)
> bus->socket_ops->stop(bus->sfp);
> @@ -451,7 +453,6 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
> {
> bus->upstream_ops = NULL;
> bus->upstream = NULL;
> - bus->netdev->sfp_bus = NULL;
> bus->netdev = NULL;
> }
>
> @@ -480,7 +481,6 @@ struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
> bus->upstream_ops = ops;
> bus->upstream = upstream;
> bus->netdev = ndev;
> - ndev->sfp_bus = bus;
>
> if (bus->sfp) {
> ret = sfp_register_bus(bus);
>
--
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops
2018-09-17 17:30 ` Florian Fainelli
@ 2018-09-17 23:13 ` Russell King - ARM Linux
0 siblings, 0 replies; 4+ messages in thread
From: Russell King - ARM Linux @ 2018-09-17 23:13 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, Andrew Lunn, David S. Miller, open list
On Mon, Sep 17, 2018 at 10:30:52AM -0700, Florian Fainelli wrote:
> On 09/15/2018 12:32 AM, Russell King - ARM Linux wrote:
> > On Fri, Sep 14, 2018 at 04:01:31PM -0700, Florian Fainelli wrote:
> >> In case we have specified a SFP and an I2C phandle in the Device Tree,
> >> but we somehow failed to look up the I2C adapter (e.g: the driver is not
> >> enabled), we will leave dangling socket_ops, and the sfp_bus will still
> >> have been registered. We can then observe NULL pointer dereferences
> >> while doing ethtool -m:
> >>
> >> [ 20.218613] Unable to handle kernel NULL pointer dereference at virtual address 00000008
> >> [ 20.226977] pgd = (ptrval)
> >> [ 20.229785] [00000008] *pgd=7c400831, *pte=00000000, *ppte=00000000
> >> [ 20.236274] Internal error: Oops: 17 [#1] SMP ARM
> >> [ 20.241122] Modules linked in:
> >> [ 20.244278] CPU: 0 PID: 1480 Comm: ethtool Not tainted 4.19.0-rc3 #138
> >> [ 20.251013] Hardware name: Broadcom Northstar Plus SoC
> >> [ 20.256316] PC is at sfp_get_module_info+0x8/0x10
> >> [ 20.261172] LR is at dev_ethtool+0x218c/0x2afc
> >>
> >> Specifically guard against that.
> >
> > The other alternative to this would be to only set ndev->sfp_bus when
> > the bus moves to registered state, which would probably be a saner
> > alternative than to add an additional layer of tests. IOW:
>
> Indeed, your patch works for me:
>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Thanks, I'll re-send it properly for davem tomorrow.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-17 23:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14 23:01 [PATCH net] net: phy: sfp: Prevent NULL deference of socket_ops Florian Fainelli
2018-09-15 7:32 ` Russell King - ARM Linux
2018-09-17 17:30 ` Florian Fainelli
2018-09-17 23:13 ` Russell King - ARM Linux
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).