Re: [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3_infoblock()
From: Arnd Bergmann <arnd@arndb.de>
Date: 2021-11-17 07:52:13
Also in:
lkml
On Wed, Nov 17, 2021 at 4:37 AM Teng Qi [off-list ref] wrote:
The definition of macro MOTO_SROM_BUG is: #define MOTO_SROM_BUG (lp->active == 8 && (get_unaligned_le32( dev->dev_addr) & 0x00ffffff) == 0x3e0008) and the if statement if (MOTO_SROM_BUG) lp->active = 0; using this macro indicates lp->active could be 8. If lp->active is 8 and the second comparison of this macro is false. lp->active will remain 8 in: lp->phy[lp->active].gep = (*p ? p : NULL); p += (2 * (*p) + 1); lp->phy[lp->active].rst = (*p ? p : NULL); p += (2 * (*p) + 1); lp->phy[lp->active].mc = get_unaligned_le16(p); p += 2; lp->phy[lp->active].ana = get_unaligned_le16(p); p += 2; lp->phy[lp->active].fdx = get_unaligned_le16(p); p += 2; lp->phy[lp->active].ttm = get_unaligned_le16(p); p += 2; lp->phy[lp->active].mci = *p;
This is a very nice analysis of the problem!
However, the length of array lp->phy is 8, so array overflows can occur. To fix these possible array overflows, we first check lp->active and then set it to 0 if it is equal to DE4X5_MAX_PHY (i.e., 8). Reported-by: TOTE Robot <redacted> Signed-off-by: Teng Qi <redacted>
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/dec/tulip/de4x5.c b/drivers/net/ethernet/dec/tulip/de4x5.c index 13121c4dcfe6..18132deac2bf 100644 --- a/drivers/net/ethernet/dec/tulip/de4x5.c +++ b/drivers/net/ethernet/dec/tulip/de4x5.c@@ -4708,7 +4708,8 @@ type3_infoblock(struct net_device *dev, u_char count, u_char *p) if (lp->state == INITIALISED) { lp->ibn = 3; lp->active = *p++; - if (MOTO_SROM_BUG) lp->active = 0; + /* The DE4X5_MAX_PHY is length of lp->phy, and its value is 8 */ + if (MOTO_SROM_BUG || lp->active == DE4X5_MAX_PHY) lp->active = 0;
I don't think this is a good fix, since this is technically the same as leaving
out the 'if (MOTO_SROM_BUG)' check and just checking for lp->active==8.
I would suggest leaving the existing logic in place (as I have no idea where
that came from), but adding a more defensive range check like:
if (WARN_ON(lp->active >= ARRAY_SIZE(lp->phy))
return -EINVAL;
Note also that this driver is already very old and orphaned, if your bot has a
lot more findings like this one, it may be best to prioritize fixing
drivers that
are actively used and maintained.
Arnd