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:   Tue, 6 Dec 2022 18:25:56 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>,
        "Radu Nicolae Pirea (OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     netdev@vger.kernel.org, stable@vger.kernel.org,
        linux-kernel@vger.kernel.org, andrew@lunn.ch, pabeni@redhat.com,
        kuba@kernel.org, edumazet@google.com, davem@davemloft.net,
        f.fainelli@gmail.com, Radu Pirea <radu-nicolae.pirea@nxp.com>
Subject: Re: [PATCH] net: dsa: sja1105: fix slab-out-of-bounds in
 sja1105_setup
Message-ID: <20221206162556.ibgw6xyi7jnjwbsg@skbuf>
References: <20221206151136.802344-1-radu-nicolae.pirea@oss.nxp.com>
 <Y49oaMcgstaa+l5G@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y49oaMcgstaa+l5G@kroah.com>
Precedence: bulk

On Tue, Dec 06, 2022 at 05:06:00PM +0100, Greg KH wrote:
> On Tue, Dec 06, 2022 at 05:11:36PM +0200, Radu Nicolae Pirea (OSS) wrote:
> > From: Radu Pirea <radu-nicolae.pirea@nxp.com>
> > 
> > Fix slab-out-of-bounds in sja1105_setup.
> > 
> > Kernel log:
> 
> <snip>
> 
> This log doesn't say much, sorry.  Please read the kernel documentation
> for how to write a good changelog text and how to submit a patch to the
> stable trees (hint, this isn't how...)

Agree with Greg.

The commit description should say that 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_setup() function initializes all L2 policers such that they
don't interfere with normal packet reception by default. To have common
code between SJA1105 and SJA1110, the index of the multicast policer for
the port is calculated, and because it's an index that is out of bounds
for SJA1105 but in bounds for SJA1110, 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.

Or something like that. The point is that you should use the commit
description to prove to yourself (and also to readers) that the change
is correct.