netdev - Re: [PATCH] ethtool: fix a missing-check bug

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, 9 Oct 2018 18:04:21 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>, "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Ilya Lesokhin <ilyal@...lanox.com>,
        Edward Cree <ecree@...arflare.com>,
        Yury Norov <ynorov@...iumnetworks.com>,
        Alan Brady <alan.brady@...el.com>,
        Eugenia Emantayev <eugenia@...lanox.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ethtool: fix a missing-check bug

On Tue, Oct 09, 2018 at 08:15:38AM -0500, Wenwen Wang wrote:
> In ethtool_get_rxnfc(), the eth command 'cmd' is compared against
> 'ETHTOOL_GRXFH' to see whether it is necessary to adjust the variable
> 'info_size'. Then the whole structure of 'info' is copied from the
> user-space buffer 'useraddr' with 'info_size' bytes. In the following
> execution, 'info' may be copied again from the buffer 'useraddr' depending
> on the 'cmd' and the 'info.flow_type'. However, after these two copies,
> there is no check between 'cmd' and 'info.cmd'. In fact, 'cmd' is also
> copied from the buffer 'useraddr' in dev_ethtool(), which is the caller
> function of ethtool_get_rxnfc(). Given that 'useraddr' is in the user
> space, a malicious user can race to change the eth command in the buffer
> between these copies. By doing so, the attacker can supply inconsistent
> data and cause undefined behavior because in the following execution 'info'
> will be passed to ops->get_rxnfc().

Do you have an example how userspace could abuse the race to make kernel
do something bad which it couldn't with the patch? I could think of only
two or three potentially problematic scenarios:

1. Userspace changes cmd to a value which would not have been dispatched
into ethtool_get_rxnfc() otherwise. While this is unfortunate, existing
in-tree ethtool::get_rxnfc() handlers would only return -EOPNOTSUPP or
-EINVAL in such case so no harm done.

2. Userspace uses ETHTOOL_GRXFH with FLOW_RSS not set in flow_type but
then switches cmd to other subcommand so that ethtool_ops::get_rxnfc()
handler is called with only partially initialized info and some garbage
in the rest. However, unless this new cmd is completely wrong (case 1
above), userspace could have sent exactly the same garbage directly.

3. The "garbage" might be leftover data which could leak into userspace
on return. However, as ethtool_get_rxnfc() would still use "short" value
of info_size, it would have to leak indirectly by affecting the value of
info.flow_type or info.data which seems rather theoretical.

Did I miss something?

I don't want to say that the check shouldn't be added, I'm just not
convinced that the reasoning in commit message is correct.

Michal Kubecek

> 
> This patch adds a necessary check on 'info.cmd' and 'cmd' to confirm that
> they are still same after the two copies in ethtool_get_rxnfc(). Otherwise,
> an error code EINVAL will be returned.
> 
> Signed-off-by: Wenwen Wang <wang6495@....edu>
> ---
>  net/core/ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c9993c6..0136625 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1015,6 +1015,9 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
>  			return -EINVAL;
>  	}
>  
> +	if (info.cmd != cmd)
> +		return -EINVAL;
> +
>  	if (info.cmd == ETHTOOL_GRXCLSRLALL) {
>  		if (info.rule_cnt > 0) {
>  			if (info.rule_cnt <= KMALLOC_MAX_SIZE / sizeof(u32))
> -- 
> 2.7.4
>