Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Dec 2022 19:22:32 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     "Radu Nicolae Pirea (OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     andrew@lunn.ch, f.fainelli@gmail.com, davem@davemloft.net,
        gregkh@linuxfoundation.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH RESEND v2] net: dsa: sja1105: avoid out of bounds access
 in sja1105_init_l2_policing()
Message-ID: <20221208172232.fldnlue35km76ldt@skbuf>
References: <20221207132347.38698-1-radu-nicolae.pirea@oss.nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221207132347.38698-1-radu-nicolae.pirea@oss.nxp.com>
Precedence: bulk

On Wed, Dec 07, 2022 at 03:23:47PM +0200, Radu Nicolae Pirea (OSS) wrote:
> The SJA1105 family has 45 L2 policing table entries
> (SJA1105_MAX_L2_POLICING_COUNT) and SJA1110 has 110
> (SJA1110_MAX_L2_POLICING_COUNT). Keeping the table structure but
> accounting for the difference in port count (5 in SJA1105 vs 10 in
> SJA1110) does not fully explain the difference. Rather, the SJA1110 also
> has L2 ingress policers for multicast traffic. If a packet is classified
> as multicast, it will be processed by the policer index 99 + SRCPORT.
> 
> The sja1105_init_l2_policing() function initializes all L2 policers such
> that they don't interfere with normal packet reception by default. To have
> a common code between SJA1105 and SJA1110, the index of the multicast
> policer for the port is calculated because it's an index that is out of
> bounds for SJA1105 but in bounds for SJA1110, and a bounds check is
> performed.
> 
> The code fails to do the proper thing when determining what to do with the
> multicast policer of port 0 on SJA1105 (ds->num_ports = 5). The "mcast"
> index will be equal to 45, which is also equal to
> table->ops->max_entry_count (SJA1105_MAX_L2_POLICING_COUNT). So it passes
> through the check. But at the same time, SJA1105 doesn't have multicast
> policers. So the code programs the SHARINDX field of an out-of-bounds
> element in the L2 Policing table of the static config.
> 
> The comparison between index 45 and 45 entries should have determined the
> code to not access this policer index on SJA1105, since its memory wasn't
> even allocated.
> 
> With enough bad luck, the out-of-bounds write could even overwrite other
> valid kernel data, but in this case, the issue was detected using KASAN.
> 
> Kernel log:
> 
> sja1105 spi5.0: Probed switch chip: SJA1105Q
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in sja1105_setup+0x1cbc/0x2340
> Write of size 8 at addr ffffff880bd57708 by task kworker/u8:0/8
> ...
> Workqueue: events_unbound deferred_probe_work_func
> Call trace:
> ...
> sja1105_setup+0x1cbc/0x2340
> dsa_register_switch+0x1284/0x18d0
> sja1105_probe+0x748/0x840
> ...
> Allocated by task 8:
> ...
> sja1105_setup+0x1bcc/0x2340
> dsa_register_switch+0x1284/0x18d0
> sja1105_probe+0x748/0x840
> ...
> 
> Fixes: 38fbe91f2287 ("net: dsa: sja1105: configure the multicast policers, if present")
> CC: stable@vger.kernel.org # 5.15+
> Signed-off-by: Radu Nicolae Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>