linux-kernel - Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

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:   Wed, 07 Sep 2016 21:37:53 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     linaro-mm-sig@...ts.linaro.org
Cc:     Laura Abbott <labbott@...hat.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        John Stultz <john.stultz@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Riley Andrews <riandrews@...roid.com>,
        Brian Starkey <brian.starkey@....com>,
        devel@...verdev.osuosl.org, Jon Medhurst <tixy@...aro.org>,
        Android Kernel Team <kernel-team@...roid.com>,
        Liviu Dudau <Liviu.Dudau@....com>,
        linux-kernel@...r.kernel.org,
        Jeremy Gebben <jgebben@...eaurora.org>,
        Eun Taik Lee <eun.taik.lee@...sung.com>,
        Chen Feng <puck.chen@...ilicon.com>
Subject: Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote:

> -	if (dir & _IOC_WRITE)
> -		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> -			return -EFAULT;
> +	/*
> +	 * The copy_from_user is unconditional here for both read and write
> +	 * to do the validate. If there is no write for the ioctl, the
> +	 * buffer is cleared
> +	 */
> +	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> +		return -EFAULT;
> +
> +	ret = validate_ioctl_arg(cmd, &data);
> +	if (WARN_ON_ONCE(ret))
> +		return ret;

I noticed that the WARN_ON_ONCE warns about invalid user input,
but I think we tend to normally just use WARN_ON for things that
go wrong inside of the kernel or in hardware. 

Maybe better use printk_once() or printk_ratelimited.

Is there any noticeable overhead in always copying the structure?
copy_from_user() can be a bit slow depending on debugging or
security features, and it seems unnecessary if the validation
is only done for one of the commands.

Otherwise the patch looks good to me.

	Arnd