netdev - Re: [PATCH v1 net 2/2] net: mscc: ocelot: fix possible memory conflict for vcap

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 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]

Date:   Sat, 30 Apr 2022 14:24:57 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: Re: [PATCH v1 net 2/2] net: mscc: ocelot: fix possible memory
 conflict for vcap_props

Hi Colin,

On Fri, Apr 29, 2022 at 04:30:49PM -0700, Colin Foster wrote:
> Each instance of an ocelot struct has the ocelot_vcap_props structure being
> referenced. During initialization (ocelot_init), these vcap_props are
> detected and the structure contents are modified.
> 
> In the case of the standard ocelot driver, there will probably only be one
> instance of struct ocelot, since it is part of the chip.
> 
> For the Felix driver, there could be multiple instances of struct ocelot.
> In that scenario, the second time ocelot_init would get called, it would
> corrupt what had been done in the first call because they both reference
> *ocelot->vcap. Both of these instances were assigned the same memory
> location.
> 
> Move this vcap_props memory to within struct ocelot, so that each instance
> can modify the structure to their heart's content without corrupting other
> instances.
> 
> Fixes: 2096805497e2b ("net: mscc: ocelot: automatically detect VCAP
> constants")
> 
> Signed-off-by: Colin Foster <colin.foster@...advantage.com>
> ---

To prove an issue, you must come with an example of two switches which
share the same struct vcap_props, but contain different VCAP constants
in the hardware registers. Otherwise, what you call "corruption" is just
"overwriting with the same values".

I would say that by definition, if two such switches have different VCAP
constants, they have different vcap_props structures, and if they have
the same vcap_props structure, they have the same VCAP constants.

Therefore, even in a multi-switch environment, a second call to
ocelot_vcap_detect_constants() would overwrite the vcap->entry_width,
vcap->tg_width, vcap->sw_count, vcap->entry_count, vcap->action_count,
vcap->action_width, vcap->counter_words, vcap->counter_width with the
exact same values.

I do not see the point in duplicating struct vcap_props per ocelot
instance.

I assume you are noticing some problems with VSC7512? What are they?
Note that since VSC7512 isn't currently supported by the kernel, even a
theoretical corruption issue doesn't qualify as a bug, since there is no
way to reproduce it. All the Microchip switches supported by the kernel
are internal to an SoC, are single switches, and they have different
vcap_props structures.