linux-kernel - Re: [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array overflows in type3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 17 Nov 2021 08:51:52 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Teng Qi <starmiku1207184332@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, tanghui20@...wei.com,
        Networking <netdev@...r.kernel.org>,
        Parisc List <linux-parisc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jia-Ju Bai <baijiaju1990@...il.com>, islituo@...il.com,
        TOTE Robot <oslab@...nghua.edu.cn>
Subject: Re: [PATCH] net: ethernet: dec: tulip: de4x5: fix possible array
 overflows in type3_infoblock()

On Wed, Nov 17, 2021 at 4:37 AM Teng Qi <starmiku1207184332@...il.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@...nghua.edu.cn>
> Signed-off-by: Teng Qi <starmiku1207184332@...il.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