netdev - [PATCH net 1/6] net: mscc: ocelot: don't use list_empty() on non-initialized list element

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

Message-Id: <20220503115728.834457-2-vladimir.oltean@nxp.com>
Date:   Tue,  3 May 2022 14:57:23 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <olteanv@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Colin Foster <colin.foster@...advantage.com>
Subject: [PATCH net 1/6] net: mscc: ocelot: don't use list_empty() on non-initialized list element

Since the blamed commit, VCAP filters can appear on more than one list.
If their action is "trap", they are chained on ocelot->traps via
filter->trap_list.

Consequently, when we free a VCAP filter, we must remove it from all
lists it is a member of, including ocelot->traps.

Normally, conditionally removing an element from a list (depending on
whether it is present or not) involves traversing the list, but we
already have a reference to the element, so that isn't really necessary.
Moreover, the operation "list_del(&filter->trap_list)" operation is
fundamentally the same regardless of whether we've iterated through the
list or just happened to have the element. So I thought it would be ok
to check whether the element has been added to a list by calling
list_empty().

However, this does not do the correct thing. list_empty() checks whether
"head->next == head", but in our case, head->next == head->prev == NULL,
and head != NULL. This makes us proceed to call list_del(), which
modifies the prev pointer of the next element, and the next pointer of
the prev element. But the next and prev elements are NULL, so we
dereference those pointers and die.

It would appear that list_empty() is not the function to use to detect
that condition. But if we had previously called INIT_LIST_HEAD() on
&filter->trap_list, then we could use list_empty() to denote whether we
are members of a list (any list).

Although the more "natural" thing seems to be to iterate through
ocelot->traps and only remove the filter from the list if it was a
member of it, it seems pointless to do that.

So fix the bug by calling INIT_LIST_HEAD() on the non-head element.

Fixes: e42bd4ed09aa ("net: mscc: ocelot: keep traps in a list")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 03b5e59d033e..b8617e940063 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -793,6 +793,11 @@ static struct ocelot_vcap_filter
 		filter->egress_port.mask = GENMASK(key_length - 1, 0);
 	}

+	/* Allow the filter to be removed from ocelot->traps
+	 * without traversing the list
+	 */
+	INIT_LIST_HEAD(&filter->trap_list);
+
 	return filter;
 }

-- 
2.25.1