ethtool: fix a missing-check bug

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().

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@umn.edu>
---
 net/core/ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

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@umn.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
>

From: Wenwen Wang <wang6495@umn.edu>
Date: Tue,  9 Oct 2018 08:15:38 -0500

> 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().
> 
> 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@umn.edu>

Applied.