netdev - Re: [net 8/8] ixgbe: ethtool: stats user buffer overrun

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]

Message-ID: <1328714229.12637.43.camel@deadeye>
Date:	Wed, 8 Feb 2012 15:17:09 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
CC:	<davem@...emloft.net>, John Fastabend <john.r.fastabend@...el.com>,
	<netdev@...r.kernel.org>, <gospo@...hat.com>, <sassmann@...hat.com>
Subject: Re: [net 8/8] ixgbe: ethtool: stats user buffer overrun

On Wed, 2012-02-08 at 01:36 -0800, Jeff Kirsher wrote:
> From: John Fastabend <john.r.fastabend@...el.com>
> 
> If the number of tx/rx queues changes the ethtool ioctl
> ETHTOOL_GSTATS may overrun the userspace buffer. This
> occurs because the general practice in user space to
> query stats is to issue a ETHTOOL_GSSET cmd to learn the
> buffer size needed, allocate the buffer, then call
> ETHTOOL_GSTIRNGS and ETHTOOL_GSTATS. If the number of
> real_num_queues is changed or flow control attributes
> are changed after ETHTOOL_GSSET but before the
> ETHTOOL_GSTRINGS/ETHTOOL_GSTATS a user space buffer
> overrun occurs.

This is a problem with several ethtool operations - the user buffer size
is implicit.

> To fix the overrun always return the max buffer size
> needed from get_sset_count() then return all strings
> and stats from get_strings()/get_ethtool_stats().
> 
> This _will_ change the output from the ioctl() call
> which could break applications and script parsing in
> theory. I believe these changes should not break existing
> tools because the only changes will be more {tx|rx}_queues
> and the {tx|rx}_pb_* stats will always be returned.
[...]

Yes, this sounds perfectly reasonable.  And this change is definitely
worth making.

However, even if the number of stats (or other attributes) for a device
never change at run-time, devices can be renamed concurrently so that
the second operation runs on a different device!

Perhaps we could change dev_ioctl so that if ifreq::ifr_name is an empty
string then the socket's bound device is used.  However, setting
SO_BINDTODEVICE currently requires CAP_NET_RAW.  Alternately, there
could be some sort of union between ifr_name and an ifindex.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html