netdev - Re: [PATCHv2 net-next-2.6] ethtool: Compat handling for struct ethtool

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:	Mon, 28 Feb 2011 15:51:57 -0800
From:	Kees Cook <kees.cook@...onical.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	David Miller <davem@...emloft.net>,
	Alexander Duyck <alexander.h.duyck@...el.com>,
	Santwona Behera <santwona.behera@....com>,
	netdev@...r.kernel.org
Subject: Re: [PATCHv2 net-next-2.6] ethtool: Compat handling for struct
 ethtool_rxnfc

Hi Ben,

On Mon, Feb 28, 2011 at 09:55:41PM +0000, Ben Hutchings wrote:
> I'm not sure whether more checks on rule_cnt are required for security
> or whether compat_alloc_user_space() and copy_in_user() can be relied on
> to limit any buffer overrun to the user process's own memory.  It looks
> like this is safe on x86.

I'm less familiar with the compat world, but it looks sane to me. All the
copy_in_user() calls are calculated based on structure sizes, right? Except
for the rule_cnt one, which was already bounds-checked for output. AFAIK,
reading beyond the end of &rxnfc->rule_locs[0] should be contained to
userspace memory due to the access_ok() check in copy_in_user().

> +	switch (ethcmd) {
> +	default:
> +		break;
> +	case ETHTOOL_GRXCLSRLALL:
> +		/* Buffer size is variable */
> +		if (get_user(rule_cnt, &compat_rxnfc->rule_cnt))
> +			return -EFAULT;
> +		if (rule_cnt > KMALLOC_MAX_SIZE / sizeof(u32))
> +			return -ENOMEM;
> +		buf_size += rule_cnt * sizeof(u32);
> +		/* fall through */
> +	case ETHTOOL_GRXRINGS:
> +	case ETHTOOL_GRXCLSRLCNT:
> +	case ETHTOOL_GRXCLSRULE:
> +		convert_out = true;
> +		/* fall through */
> +	case ETHTOOL_SRXCLSRLDEL:
> +	case ETHTOOL_SRXCLSRLINS:
> +		buf_size += sizeof(struct ethtool_rxnfc);
> +		convert_in = true;
> +		break;
> +	}
> ...
> +	if (convert_out) {
> ...
> +		if (ethcmd == ETHTOOL_GRXCLSRLALL) {
> +			if (get_user(rule_cnt, &compat_rxnfc->rule_cnt) ||
> +			    copy_in_user(&compat_rxnfc->rule_locs[0],
> +					 &rxnfc->rule_locs[0],
> +					 rule_cnt * sizeof(u32)))
> +				return -EFAULT;

-Kees

-- 
Kees Cook
Ubuntu Security Team
--
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