Re: [PATCH v2] ethtool: check size of user memory before copying strings and stats

["Keller, Jacob E" <jacob.e.keller@intel.com>]

On Tue, 2016-03-01 at 15:05 -0800, Alexander Duyck wrote:
> On Tue, Mar 1, 2016 at 2:58 PM, Keller, Jacob E
> <jacob.e.keller@intel.com> wrote:
> > 
> > On Tue, 2016-03-01 at 14:31 -0800, Alexander Duyck wrote:
> > > 
> > > This still has the potential to provide garbage data.  What you
> > > should
> > > probably do at each stage is make sure the length matches with
> > > the
> > > exact value that you would expect.
> > > 
> > Sure, an exact check could be done instead, however...
> > 
> > > 
> > > I assume you cannot have any fields shuffle on you?  What I mean
> > > by
> > > that is that you don't want to have a setup with 4 Tx and 4 Rx
> > > rings
> > > where you then replace it with 1 Tx and 7 Rx rings and try to
> > > populate
> > > the same data into a setup where the strings reported are for 4
> > > Tx
> > > and
> > > 4 Rx.  You should double check that the length can be used as a
> > > means
> > > of identifying exactly what strings will be where.
> > > 
> > > - Alex
> > 
> > Darn. Looks like you're right. It would be theoretically possible
> > for
> > the number of queues (or other variables) to change such that the
> > size
> > matches but the data no longer lines up against the strings.
> > 
> > For queues, I don't think we're vulnerable on the fm10k driver,
> > because
> > we only use combined queues. However, we already have support for
> > "debug-statistics" which shows extra stats plus some stats per
> > virtual
> > function. I am not sure if these could change within the time
> > window to
> > result in garbage data.
> > 
> > I don't know how much of a real world problem this would be though.
> > 
> > I'm guessing it's more reason to promote the idea of converting to
> > some
> > new tool based on netlink.
> Yeah, we had basically pushed the issue under the rug by using a
> static layout for statistics.
> 

Yep.

> Was the motivation for getting rid of the static layout just to
> declutter the display?  I know I have gotten into the habit of just
> piping ethtool -S through a "| grep -v :\ 0" to drop any of the
> unused
> stats from the display.  Also you may see issues depending on how the
> stats might be parsed by user-space scripts if you start modifying
> what is displayed and what is not.
> 

Generally, I'm not too concerned about specific user-scripts parsing
since these already have to be heavily dependent on the driver in
question (since each driver has their own naming scheme).

I added a while back the private flag "debug-statistics" to enable a
bunch of extra statistics which weren't always relevant.

In some cases, like the queue size change it was done in order to
simply remove clutter of 128 queueus when many systems would only use a
few of those. Just doing grep -v :\ 0 will remove these lines, true.

However, that doesn't help the debug-statistics case, where we have a
ton of statistics which may be relevant to some debugging, but aren't
really useful to see all the time. In addition, it also shows stats for
VFs in this mode as well.

Thanks,
Jake