From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752504AbcIHAPC (ORCPT <rfc822;w@1wt.eu>);
        Wed, 7 Sep 2016 20:15:02 -0400
Received: from mail-yw0-f182.google.com ([209.85.161.182]:34492 "EHLO
        mail-yw0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751263AbcIHAOx (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 7 Sep 2016 20:14:53 -0400
Subject: Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to
 query available heaps
To: Arnd Bergmann <arnd@arndb.de>, linaro-mm-sig@lists.linaro.org
References: <1473274199-30338-1-git-send-email-labbott@redhat.com>
 <1473274199-30338-3-git-send-email-labbott@redhat.com>
 <3889942.OlqmzfXO6P@wuerfel>
Cc: Sumit Semwal <sumit.semwal@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= <arve@android.com>,
        Riley Andrews <riandrews@android.com>,
        Brian Starkey <brian.starkey@arm.com>, devel@driverdev.osuosl.org,
        Jon Medhurst <tixy@linaro.org>,
        Android Kernel Team <kernel-team@android.com>,
        Liviu Dudau <Liviu.Dudau@arm.com>, linux-kernel@vger.kernel.org,
        Jeremy Gebben <jgebben@codeaurora.org>,
        Eun Taik Lee <eun.taik.lee@samsung.com>,
        Chen Feng <puck.chen@hisilicon.com>
From: Laura Abbott <labbott@redhat.com>
Message-ID: <123eb3c5-ecec-4b05-d541-832086e9c670@redhat.com>
Date: Wed, 7 Sep 2016 17:14:48 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <3889942.OlqmzfXO6P@wuerfel>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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