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
| ||
|
Date: Sat, 30 Apr 2022 15:26:43 -0700 From: Colin Foster <colin.foster@...advantage.com> To: Vladimir Oltean <vladimir.oltean@....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 On Sat, Apr 30, 2022 at 09:56:52PM +0000, Vladimir Oltean wrote: > On Sat, Apr 30, 2022 at 10:24:00AM -0700, Colin Foster wrote: > > Hi Vladimir, > > > > On Sat, Apr 30, 2022 at 02:24:57PM +0000, Vladimir Oltean wrote: > > > 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? > > > > I'm not seeing issues, no. I was looking to implement the shared > > ocelot_vcap struct between the 7514 and (in-development 7512. In doing > > so I came across this realization that these per-file structures could > > be referenced multiple times, which was the point of this patch. If the > > structure were simply a const configuration there would be no issue, but > > since it is half const and half runtime populated it got more complicated. > > > > (that is likely why I didn't make it shared initially... which feels > > like ages ago at this point) > > > > Whether or not hardware exists that could be affected by this corner > > case I don't know. > > VSC7512 documentation at the following link, VCAP constants are laid out > in tables 72-74 starting with page 112: > https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf > > VSC7514 documentation at the following link, VCAP constants are laid out > in tables 71-73 starting with page 111: > https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf > > As you can see, they are identical. Coincidence? I think not. After all, > they are from the same generation and have the same port count. > So even if the new vsc7512 driver reuses the vsc7514 structure for VCAP > properties, and is instantiated in a system where a vsc7514 switch is > also instantiated, I claim that nothing bad will happen. Are you > claiming otherwise? What is that bad thing, exactly? I see your point - I misinterpreted the severity here. I agree at the end of the day we'll possibly write the same values into a memory location multiple times, and since there's no supported hardware that differs there won't be a risk. If, at some point in the future, a chip comes along with slightly different parameters it could become a problem, but there's no need to solve a problem now that might never exist. Thanks for the feedback. I'll drop this patch, as it isn't necessary. > > > > > > 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. > > > > I see. So I do have a misunderstanding in the process. > > > > I shouldn't have submitted this to net, because it isn't an actual "bug" > > I observed. Instead it was a potential issue with existing code, and > > could have affected certain hardware configurations. How should I have > > sent this out? (RFC? net-next? separate conversation discussing the > > validity?) > > I can't answer how you should have sent out this patch, since I don't > yet understand what is gained by making the change. > > > Back to this patch in particular: > > > > You're saying there's no need to duplicate the vcap_props structure > > array per ocelot instance. Understood. Would it be an improvement to > > split up vcap into a const configuration section (one per hardware > > layout) and a detected set? Or would you have any other suggestion? > > Maybe, although I assume the only reason why you're proposing that is > that you want to then proceed and make the detected properties unique > per switch, which again would increase the memory footprint of the > driver for a reason I am not following. > > I suppose there's also the option of leaving code that isn't broken > alone? > > > And, of course, I can drag this along with my 7512 patch set for now, > > Why? > > > or try to get this in now. This one feels like it is worth keeping > > separate... > > > > And thanks as always for your feedback!
Powered by blists - more mailing lists