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] [day] [month] [year] [list]

Date:   Wed, 7 Sep 2016 17:14:48 -0700
From:   Laura Abbott <labbott@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>, linaro-mm-sig@...ts.linaro.org
Cc:     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 09/07/2016 12:37 PM, Arnd Bergmann wrote:
> 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.
>

Sure, the error code should hopefully be enough of a hint to
userspace to maybe check the log.

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

Good point. It made sense with some of the other ioctls (specifically
the ABI) but isn't necessary now. We can evaluate later when other
ioctls get added.

> Otherwise the patch looks good to me.
>
> 	Arnd
>

Thanks!

Laura