* [PATCH net] ice: copy last block omitted in ice_get_module_eeprom()
@ 2023-02-28 20:41 Petr Oros
2023-03-01 15:13 ` [Intel-wired-lan] " Alexander Lobakin
2023-03-01 17:14 ` [PATCH net v2] " Petr Oros
0 siblings, 2 replies; 4+ messages in thread
From: Petr Oros @ 2023-02-28 20:41 UTC (permalink / raw)
To: netdev
Cc: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba, pabeni,
scott.w.taylor, intel-wired-lan, linux-kernel
ice_get_module_eeprom() is broken since commit e9c9692c8a81 ("ice:
Reimplement module reads used by ethtool") In this refactor,
ice_get_module_eeprom() reads the eeprom in blocks of size 8.
But the condition that should protect the buffer overflow
ignores the last block. The last block always contains zeros.
Fix adding memcpy for last block.
Bug uncovered by ethtool upstream commit 9538f384b535
("netlink: eeprom: Defer page requests to individual parsers")
After this commit, ethtool reads a block with length = 1;
to read the SFF-8024 identifier value.
unpatched driver:
$ ethtool -m enp65s0f0np0 offset 0x90 length 8
Offset Values
------ ------
0x0090: 00 00 00 00 00 00 00 00
$ ethtool -m enp65s0f0np0 offset 0x90 length 12
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c 00 00 00 00
$
$ ethtool -m enp65s0f0np0
Offset Values
------ ------
0x0000: 11 06 06 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 08 00
0x0070: 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
patched driver:
$ ethtool -m enp65s0f0np0 offset 0x90 length 8
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c
$ ethtool -m enp65s0f0np0 offset 0x90 length 12
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c 61 6e 6f 78
$ ethtool -m enp65s0f0np0
Identifier : 0x11 (QSFP28)
Extended identifier : 0x00
Extended identifier description : 1.5W max. Power consumption
Extended identifier description : No CDR in TX, No CDR in RX
Extended identifier description : High Power Class (> 3.5 W) not enabled
Connector : 0x23 (No separable connector)
Transceiver codes : 0x88 0x00 0x00 0x00 0x00 0x00 0x00 0x00
Transceiver type : 40G Ethernet: 40G Base-CR4
Transceiver type : 25G Ethernet: 25G Base-CR CA-N
Encoding : 0x05 (64B/66B)
BR, Nominal : 25500Mbps
Rate identifier : 0x00
Length (SMF,km) : 0km
Length (OM3 50um) : 0m
Length (OM2 50um) : 0m
Length (OM1 62.5um) : 0m
Length (Copper or Active cable) : 1m
Transmitter technology : 0xa0 (Copper cable unequalized)
Attenuation at 2.5GHz : 4db
Attenuation at 5.0GHz : 5db
Attenuation at 7.0GHz : 7db
Attenuation at 12.9GHz : 10db
........
....
Fixes: e9c9692c8a81 ("ice: Reimplement module reads used by ethtool")
Signed-off-by: Petr Oros <poros@redhat.com>
---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index b360bd8f15998b..33b2bee5cfb40f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4356,6 +4356,8 @@ ice_get_module_eeprom(struct net_device *netdev,
/* Make sure we have enough room for the new block */
if ((i + SFF_READ_BLOCK_SIZE) < ee->len)
memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
+ else if (ee->len - i > 0)
+ memcpy(data + i, value, ee->len - i);
}
}
return 0;
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net] ice: copy last block omitted in ice_get_module_eeprom()
2023-02-28 20:41 [PATCH net] ice: copy last block omitted in ice_get_module_eeprom() Petr Oros
@ 2023-03-01 15:13 ` Alexander Lobakin
2023-03-01 17:14 ` [PATCH net v2] " Petr Oros
1 sibling, 0 replies; 4+ messages in thread
From: Alexander Lobakin @ 2023-03-01 15:13 UTC (permalink / raw)
To: Petr Oros
Cc: netdev, intel-wired-lan, jesse.brandeburg, linux-kernel, edumazet,
anthony.l.nguyen, kuba, pabeni, davem
From: Petr Oros <poros@redhat.com>
Date: Tue, 28 Feb 2023 21:41:39 +0100
> ice_get_module_eeprom() is broken since commit e9c9692c8a81 ("ice:
> Reimplement module reads used by ethtool") In this refactor,
> ice_get_module_eeprom() reads the eeprom in blocks of size 8.
> But the condition that should protect the buffer overflow
> ignores the last block. The last block always contains zeros.
> Fix adding memcpy for last block.
[...]
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index b360bd8f15998b..33b2bee5cfb40f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -4356,6 +4356,8 @@ ice_get_module_eeprom(struct net_device *netdev,
> /* Make sure we have enough room for the new block */
> if ((i + SFF_READ_BLOCK_SIZE) < ee->len)
> memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
> + else if (ee->len - i > 0)
> + memcpy(data + i, value, ee->len - i);
Maybe just unify those two?
copy_len = min_t(u32, SFF_READ_BLOCK_SIZE,
ee->len - i);
memcpy(data + i, value, copy_len);
That's pretty much a reword of your code.
The functional change is good to me.
> }
> }
> return 0;
Thanks,
Olek
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH net v2] ice: copy last block omitted in ice_get_module_eeprom()
2023-02-28 20:41 [PATCH net] ice: copy last block omitted in ice_get_module_eeprom() Petr Oros
2023-03-01 15:13 ` [Intel-wired-lan] " Alexander Lobakin
@ 2023-03-01 17:14 ` Petr Oros
2023-03-01 19:26 ` Jesse Brandeburg
1 sibling, 1 reply; 4+ messages in thread
From: Petr Oros @ 2023-03-01 17:14 UTC (permalink / raw)
To: netdev
Cc: aleksander.lobakin, jesse.brandeburg, anthony.l.nguyen, davem,
edumazet, kuba, pabeni, scott.w.taylor, intel-wired-lan,
linux-kernel
ice_get_module_eeprom() is broken since commit e9c9692c8a81 ("ice:
Reimplement module reads used by ethtool") In this refactor,
ice_get_module_eeprom() reads the eeprom in blocks of size 8.
But the condition that should protect the buffer overflow
ignores the last block. The last block always contains zeros.
Bug uncovered by ethtool upstream commit 9538f384b535
("netlink: eeprom: Defer page requests to individual parsers")
After this commit, ethtool reads a block with length = 1;
to read the SFF-8024 identifier value.
unpatched driver:
$ ethtool -m enp65s0f0np0 offset 0x90 length 8
Offset Values
------ ------
0x0090: 00 00 00 00 00 00 00 00
$ ethtool -m enp65s0f0np0 offset 0x90 length 12
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c 00 00 00 00
$
$ ethtool -m enp65s0f0np0
Offset Values
------ ------
0x0000: 11 06 06 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 08 00
0x0070: 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00
patched driver:
$ ethtool -m enp65s0f0np0 offset 0x90 length 8
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c
$ ethtool -m enp65s0f0np0 offset 0x90 length 12
Offset Values
------ ------
0x0090: 00 00 01 a0 4d 65 6c 6c 61 6e 6f 78
$ ethtool -m enp65s0f0np0
Identifier : 0x11 (QSFP28)
Extended identifier : 0x00
Extended identifier description : 1.5W max. Power consumption
Extended identifier description : No CDR in TX, No CDR in RX
Extended identifier description : High Power Class (> 3.5 W) not enabled
Connector : 0x23 (No separable connector)
Transceiver codes : 0x88 0x00 0x00 0x00 0x00 0x00 0x00 0x00
Transceiver type : 40G Ethernet: 40G Base-CR4
Transceiver type : 25G Ethernet: 25G Base-CR CA-N
Encoding : 0x05 (64B/66B)
BR, Nominal : 25500Mbps
Rate identifier : 0x00
Length (SMF,km) : 0km
Length (OM3 50um) : 0m
Length (OM2 50um) : 0m
Length (OM1 62.5um) : 0m
Length (Copper or Active cable) : 1m
Transmitter technology : 0xa0 (Copper cable unequalized)
Attenuation at 2.5GHz : 4db
Attenuation at 5.0GHz : 5db
Attenuation at 7.0GHz : 7db
Attenuation at 12.9GHz : 10db
........
....
Fixes: e9c9692c8a81 ("ice: Reimplement module reads used by ethtool")
Signed-off-by: Petr Oros <poros@redhat.com>
---
v2: memcpy unified calls
---
---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index b360bd8f15998b..1dc3f9fc74bdfb 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4293,6 +4293,7 @@ ice_get_module_eeprom(struct net_device *netdev,
bool is_sfp = false;
unsigned int i, j;
u16 offset = 0;
+ u32 copy_len;
u8 page = 0;
int status;
@@ -4354,8 +4355,9 @@ ice_get_module_eeprom(struct net_device *netdev,
}
/* Make sure we have enough room for the new block */
- if ((i + SFF_READ_BLOCK_SIZE) < ee->len)
- memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
+ copy_len = min_t(u32, SFF_READ_BLOCK_SIZE,
+ ee->len - i);
+ memcpy(data + i, value, copy_len);
}
}
return 0;
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net v2] ice: copy last block omitted in ice_get_module_eeprom()
2023-03-01 17:14 ` [PATCH net v2] " Petr Oros
@ 2023-03-01 19:26 ` Jesse Brandeburg
0 siblings, 0 replies; 4+ messages in thread
From: Jesse Brandeburg @ 2023-03-01 19:26 UTC (permalink / raw)
To: Petr Oros, netdev
Cc: aleksander.lobakin, anthony.l.nguyen, davem, edumazet, kuba,
pabeni, scott.w.taylor, intel-wired-lan, linux-kernel
On 3/1/2023 9:14 AM, Petr Oros wrote:
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index b360bd8f15998b..1dc3f9fc74bdfb 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -4293,6 +4293,7 @@ ice_get_module_eeprom(struct net_device *netdev,
> bool is_sfp = false;
> unsigned int i, j;
> u16 offset = 0;
> + u32 copy_len;
copy_len should only be declared in the "if" below so that it's only
declared in the context it is used.
> u8 page = 0;
> int status;
>
> @@ -4354,8 +4355,9 @@ ice_get_module_eeprom(struct net_device *netdev,
> }
>
> /* Make sure we have enough room for the new block */
> - if ((i + SFF_READ_BLOCK_SIZE) < ee->len)
> - memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
> + copy_len = min_t(u32, SFF_READ_BLOCK_SIZE,
> + ee->len - i);
also this line can be unwrapped now.
> + memcpy(data + i, value, copy_len);
> }
> }
> return 0;
I've tested this and it fixes the problem. Interestingly, the ethtool
application when compiled without netlink support parsed the output
correctly so caused a bit of confusion around this issue.
Thank you Petr!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-01 19:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-28 20:41 [PATCH net] ice: copy last block omitted in ice_get_module_eeprom() Petr Oros
2023-03-01 15:13 ` [Intel-wired-lan] " Alexander Lobakin
2023-03-01 17:14 ` [PATCH net v2] " Petr Oros
2023-03-01 19:26 ` Jesse Brandeburg
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).