netdev - Re: [PATCH net] net: mvpp2: Prevent parser TCAM memory corruption

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: <20250320105747.6f271fff@fedora.home>
Date: Thu, 20 Mar 2025 10:57:47 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, marcin.s.wojtas@...il.com,
 linux@...linux.org.uk, andrew@...n.ch, edumazet@...gle.com,
 pabeni@...hat.com, ezequiel.garcia@...e-electrons.com,
 netdev@...r.kernel.org
Subject: Re: [PATCH net] net: mvpp2: Prevent parser TCAM memory corruption

Hi Tobias,

On Thu, 20 Mar 2025 10:17:00 +0100
Tobias Waldekranz <tobias@...dekranz.com> wrote:

> Protect the parser TCAM/SRAM memory, and the cached (shadow) SRAM
> information, from concurrent modifications.
> 
> Both the TCAM and SRAM tables are indirectly accessed by configuring
> an index register that selects the row to read or write to. This means
> that operations must be atomic in order to, e.g., avoid spreading
> writes across multiple rows. Since the shadow SRAM array is used to
> find free rows in the hardware table, it must also be protected in
> order to avoid TOCTOU errors where multiple cores allocate the same
> row.
> 
> This issue was detected in a situation where `mvpp2_set_rx_mode()` ran
> concurrently on two CPUs. In this particular case the
> MVPP2_PE_MAC_UC_PROMISCUOUS entry was corrupted, causing the
> classifier unit to drop all incoming unicast - indicated by the
> `rx_classifier_drops` counter.
> 
> Fixes: 3f518509dedc ("ethernet: Add new driver for Marvell Armada 375 network unit")
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---

[...]

> +int mvpp2_prs_init_from_hw(struct mvpp2 *priv, struct mvpp2_prs_entry *pe,
> +			   int tid)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&priv->prs_spinlock, flags);
> +	err = mvpp2_prs_init_from_hw_unlocked(priv, pe, tid);
> +	spin_unlock_irqrestore(&priv->prs_spinlock, flags);

That's indeed an issue, I'm wondering however if you really need to
irqsave/irqrestore everytime you protect the accesses to the Parser.

From what I remember we don't touch the Parser in the interrupt path,
it's mostly a consequence to netdev ops being called (promisc, vlan
add/kill, mc/uc filtering and a lot in the init path).

Maxime