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 22:58:26 +0000
From:	"Keller, Jacob E" <jacob.e.keller@...el.com>
To:	"alexander.duyck@...il.com" <alexander.duyck@...il.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, 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.

Thanks,
Jake