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

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 linux-cve-announce 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:	Tue, 1 Mar 2016 15:05:48 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	"Keller, Jacob E" <jacob.e.keller@...el.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"bhutchings@...arflare.com" <bhutchings@...arflare.com>,
	"jeff@...zik.org" <jeff@...zik.org>,
	"Rustad, Mark D" <mark.d.rustad@...el.com>,
	"mirq-linux@...e.qmqm.pl" <mirq-linux@...e.qmqm.pl>
Subject: Re: [PATCH v2] ethtool: check size of user memory before copying
 strings and stats

On Tue, Mar 1, 2016 at 2:58 PM, Keller, Jacob E
<jacob.e.keller@...el.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.

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.

- Alex