* [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3_infoblock()
@ 2021-11-17 3:37 Teng Qi
2021-11-17 7:51 ` Arnd Bergmann
0 siblings, 1 reply; 2+ messages in thread
From: Teng Qi @ 2021-11-17 3:37 UTC (permalink / raw)
To: davem, kuba, arnd, tanghui20
Cc: netdev, linux-parisc, linux-kernel, baijiaju1990, islituo,
Teng Qi, TOTE Robot
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;
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 <oslab@tsinghua.edu.cn>
Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
---
drivers/net/ethernet/dec/tulip/de4x5.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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;
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3_infoblock()
2021-11-17 3:37 [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3_infoblock() Teng Qi
@ 2021-11-17 7:51 ` Arnd Bergmann
0 siblings, 0 replies; 2+ messages in thread
From: Arnd Bergmann @ 2021-11-17 7:51 UTC (permalink / raw)
To: Teng Qi
Cc: David Miller, Jakub Kicinski, Arnd Bergmann, tanghui20,
Networking, Parisc List, Linux Kernel Mailing List, Jia-Ju Bai,
islituo, TOTE Robot
On Wed, Nov 17, 2021 at 4:37 AM Teng Qi <starmiku1207184332@gmail.com> 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 <oslab@tsinghua.edu.cn>
> Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-17 7:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 3:37 [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3_infoblock() Teng Qi
2021-11-17 7:51 ` Arnd Bergmann
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).