Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Feb 15, 2021 at 05:15:21PM +0300, Dan Carpenter wrote:
> On Mon, Feb 15, 2021 at 03:19:31PM +0200, Vladimir Oltean wrote:
> > Hi Dan,
> >
> > On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote:
> > > db->index is less than db->num_ports which 32 or less but sometimes it
> > > comes from the device tree so who knows.
> >
> > The destination port mask is copied into a 12-bit field of the packet,
> > starting at bit offset 67 and ending at 56:
> >
> > static inline void ocelot_ifh_set_dest(void *injection, u64 dest)
> > {
> > 	packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0);
> > }
> >
> > So this DSA tagging protocol supports at most 12 bits, which is clearly
> > less than 32. Attempting to send to a port number > 12 will cause the
> > packing() call to truncate way before there will be 32-bit truncation
> > due to type promotion of the BIT(port) argument towards u64.
> >
> > > The ocelot_ifh_set_dest() function takes a u64 though and that
> > > suggests that BIT() should be changed to BIT_ULL().
> >
> > I understand that you want to silence the warning, which fundamentally
> > comes from the packing() API which works with u64 values and nothing of
> > a smaller size. So I can send a patch which replaces BIT(port) with
> > BIT_ULL(port), even if in practice both are equally fine.
>
> I don't have a strong feeling about this...  Generally silencing
> warnings just to make a checker happy is the wrong idea.

In this case it is a harmless wrong idea.

> To be honest, I normally ignore these warnings.  But I have been looking
> at them recently to try figure out if we could make it so it would only
> generate a warning where "db->index" was known as possibly being in the
> 32-63 range.  So I looked at this one.
>
> And now I see some ways that Smatch could have parsed this better...

For DSA, the dp->index should be lower than ds->num_ports by
construction (see dsa_switch_touch_ports). In turn, ds->num_ports is set
to constant values smaller than 12 in felix_pci_probe() and in seville_probe().

For ocelot on the other hand, there is a restriction put in
mscc_ocelot_init_ports that the port must be <= ocelot->num_phys_ports,
which is set to "of_get_child_count(ports)". So there is indeed a
possible attack by device tree on the ocelot driver. The number of
physical ports does not depend on device tree
(arch/mips/boot/dts/mscc/ocelot.dtsi), but should be hardcoded to 11.
How many ports there are defined in DT doesn't change how many physical
ports there are. For example, the CPU port module is at index 11, and in
the code it is referenced as ocelot->ports[ocelot->num_phys_ports].
If num_phys_ports has any other value than 11, the driver malfunctions
because the index of the CPU port is misidentified. I'd rather fix this
if there was some way in which static analysis could then determine that
"port" is bounded by a constant smaller than the truncation threshold.

However, I'm not sure how to classify the severity of this problem.
There's a million of other ways in which the system can malfunction if
it is being "attacked by device tree".