netdev - issue with set_vlan_mode in starfire driver

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>] [day] [month] [year] [list]

Message-ID: <31f2b9a1-19e8-e6db-6af8-77db17864dbb@canonical.com>
Date:   Fri, 9 Jul 2021 14:46:09 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: issue with set_vlan_mode in starfire driver

Hi,

Static analysis with Coverity on the starfire driver has detected an
issue introduced in function set_vlan_mode with the following commit:

commit 5da96be53a16a62488316810d0c7c5d58ce3ee4f
Author: Jiri Pirko <jpirko@...hat.com>
Date:   Wed Jul 20 04:54:31 2011 +0000

    starfire: do vlan cleanup

The analysis is as follows:

1743 static u32 set_vlan_mode(struct netdev_private *np)
1744 {
1745        u32 ret = VlanMode;
1746        u16 vid;
1747        void __iomem *filter_addr = np->base + HashTable + 8;
1748        int vlan_count = 0;
1749
1750        for_each_set_bit(vid, np->active_vlans, VLAN_N_VID) {
1751                if (vlan_count == 32)
1752                        break;
1753                writew(vid, filter_addr);
1754                filter_addr += 16;
1755                vlan_count++;
1756        }

cond_const: Condition vlan_count == 32, taking true branch. Now the
value of vlan_count is equal to 32.

1757        if (vlan_count == 32) {
1758                ret |= PerfectFilterVlan;

const: At condition vlan_count < 32, the value of vlan_count must be
equal to 32.
dead_error_condition: The condition vlan_count < 32 cannot be true.

1759                while (vlan_count < 32) {

Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: writew(0,
filter_addr);.

1760                        writew(0, filter_addr);
1761                        filter_addr += 16;
1762                        vlan_count++;
1763                }
1764        }
1765        return ret;
1766 }

Looking at commit 5da96be53a16a62488316810d0c7c5d58ce3ee4f it appears
that the check if (vlan_count == 32) should be if (vid == VLAN_N_VID) if
I understand things correctly.

However, I'm not sure about the setting of ret |= PerfectFilterVlan -
should that be set if the vlan_count reaches 32 or if vid reaches
VLAN_N_VID.  I don't understand the semantics of setting the
PerfectFilterVlan bit so I'm a bit stuck at figuring out an appropriate fix.

Thought had better flag this up as an issue since I can't resolve it.

Colin