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

[Michal Kubecek]

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@xxxxxxx>
> ---
>  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
>