https://bugs.dpdk.org/show_bug.cgi?id=2102
Bug ID: 2102
Summary: net/ice: ice_get_module_eeprom() can read the wrong
page when a block straddles a 256-byte boundary
Product: DPDK
Version: unspecified
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: ethdev
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
https://github.com/DPDK/dpdk/blob/a13c1053360ee26bbacca9f883d3f3cb83a0f366/drivers/net/intel/ice/ice_ethdev.c#L6683-L6772
for (i = 0; i < info->length; i += SFF_READ_BLOCK_SIZE) {
offset = i + info->offset;
page = 0;
/* Check if we need to access the other memory page */
if (is_sfp) {
if (offset >= ICE_MODULE_SFF_8079_LEN) {
offset -= ICE_MODULE_SFF_8079_LEN;
addr = ICE_I2C_EEPROM_DEV_ADDR2;
}
} else {
while (offset >= ICE_MODULE_SFF_8436_LEN) {
offset -= ICE_MODULE_SFF_8436_LEN / 2;
page++;
}
}
...
status = ice_aq_sff_eeprom(hw, 0, addr, offset, page, !is_sfp, value,
SFF_READ_BLOCK_SIZE, 0, NULL);
...
if ((i + SFF_READ_BLOCK_SIZE) <= info->length)
memcpy(data + i, value, SFF_READ_BLOCK_SIZE);
}
The page/address switch is computed once per 8-byte block, from that
block's starting offset only. Nothing checks whether offset +
SFF_READ_BLOCK_SIZE actually stays within the same page/address before
issuing the read, so a block that starts just below a boundary is
still issued as a single read against that one address/page -- the
bytes before the boundary come back correct, but the trailing bytes
past it are read from whatever happens to sit at that same local
offset on the wrong side, and that wrong data is copied into the
caller's buffer along with the rest.
1. SFP (is_sfp branch): A0h/A2h straddle, and past A2h's own end.
A call with info->offset=253, info->length=8 has its one block at
offset=253, which is < 256 so it's read entirely from A0h, bytes
253-260 -- but bytes 256-260 actually belong to A2h. That wrong data
is copied to the caller.
The same kind of gap exists at A2h's own end: info->offset=505,
info->length=8 computes offset=505-256=249 on A2h, reading local
A2h bytes 249-256 -- byte 256 is past A2h's own 256-byte range.
2. QSFP (else branch): wrong page for the straddling tail bytes.
A call with info->offset=250, info->length=8 has its one block at
offset=250, page=0 (since 250 < 256), reading bytes 250-257 -- but
bytes 256-257 belong to the next page. That wrong data is copied to
the caller. The same gap recurs at every subsequent page boundary.
Likely consequence in all cases (not empirically tested): wrong byte
values returned to the caller under a fully successful status, for
any read whose block happens to straddle one of these boundaries.
What happens for an offset far past any valid range differs between
the two branches:
- SFP: info->offset=600 computes offset=600-256=344 (only one
subtraction, no loop) and addr=A2h. 344 (0x158) fails the
mem_addr & 0xff00 check in ice_aq_sff_eeprom() below, so this case
at least returns an error (ICE_ERR_PARAM) instead of silently
reading garbage.
- QSFP: the while loop keeps subtracting 128 and incrementing page
for as long as offset >= 256, with no upper bound on page. There is
no equivalent range check on page anywhere -- the result of asking
for a page number the module doesn't implement is undefined from
the driver's side and would depend on the module's own hardware
behavior.
https://github.com/DPDK/dpdk/blob/a13c1053360ee26bbacca9f883d3f3cb83a0f366/drivers/net/intel/ice/base/ice_common.c#L4350-L4379
int
ice_aq_sff_eeprom(struct ice_hw *hw, u16 lport, u8 bus_addr,
u16 mem_addr, u8 page, u8 set_page, u8 *data, u8 length,
bool write, struct ice_sq_cd *cd)
{
...
if (!data || (mem_addr & 0xff00))
return ICE_ERR_PARAM;
...
}
ice_aq_sff_eeprom() validates the starting mem_addr but never checks
mem_addr + length against the page boundary, and never validates page
at all.
Suggested fix:
Hardening ice_aq_sff_eeprom() to also reject mem_addr + length >
0xff would catch some out-of-range cases earlier, but that alone is
not a fix and must not be the only change: ice_get_module_eeprom()
always requests a fixed SFF_READ_BLOCK_SIZE (8) bytes regardless of
how much of that block the caller actually needs, so a legitimate
small request (e.g. offset=250, length=4) would start failing outright
once such a check is added, even though it never needed the
straddling bytes in the first place.
The required fix belongs in ice_get_module_eeprom() itself. A single
I2C/admin-queue transaction cannot cross either boundary correctly in
the first place -- SFP's A0h and A2h are distinct I2C device
addresses, and QSFP's pages reuse the same address range with
different contents depending on which page was last selected, so a
straddling block was never going to be satisfiable as one read
regardless of any driver change. The fix is to check whether offset +
SFF_READ_BLOCK_SIZE crosses the relevant boundary and, if so, split
that iteration into two correctly-addressed sub-reads (switching
device address for SFP, or writing the new page-select value for
QSFP) instead of issuing one read spanning both. Each individual read
should also be sized to min(SFF_READ_BLOCK_SIZE, info->length - i)
rather than always requesting a full fixed 8 bytes, so a small
trailing request doesn't over-read into the next page for data nobody
asked for.
With that in place, adding the mem_addr + length check to
ice_aq_sff_eeprom() becomes reasonable defense-in-depth against any
other/future caller of that function, but it is not a substitute for
the ice_get_module_eeprom() fix above.
--
You are receiving this mail because:
You are the assignee for the bug.
