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 S1756819AbcIGThs (ORCPT <rfc822;w@1wt.eu>);
        Wed, 7 Sep 2016 15:37:48 -0400
Received: from mout.kundenserver.de ([212.227.126.131]:64371 "EHLO
        mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1756711AbcIGTho (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 7 Sep 2016 15:37:44 -0400
From: Arnd Bergmann <arnd@arndb.de>
To: linaro-mm-sig@lists.linaro.org
Cc: Laura Abbott <labbott@redhat.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?ISO-8859-1?Q?Hj=F8nnev=E5g?= <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>
Subject: Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps
Date: Wed, 07 Sep 2016 21:37:53 +0200
Message-ID: <3889942.OlqmzfXO6P@wuerfel>
User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; )
In-Reply-To: <1473274199-30338-3-git-send-email-labbott@redhat.com>
References: <1473274199-30338-1-git-send-email-labbott@redhat.com> <1473274199-30338-3-git-send-email-labbott@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-Provags-ID: V03:K0:er/l5yU/Plyt4GR7nZ2bqstfEvuYrDsmvBEXNY7AEMgb67X5lr9
 vx4jUZmEhqC9L9+p7hT3xRT1DoYeZyXFOi+I+JofLgS8LZ2/dtVRpw9dktaddPcH3IC2p0a
 puQmu+jrlWkWLswfJxhQ4SU/Gk+ZTKe95cIRoRYCMiKwe2bq5XAoMJJBqGSltJO/15rkEdD
 /cm8qEmGvsnNXUbF8lQSw==
X-UI-Out-Filterresults: notjunk:1;V01:K0:0oKQVEg1t5k=:CQwnnzgkQ4801HrahbFjqT
 LgVK6FzZzkQULlOzXs9HESV/AozOVInOPNBSdYdPdqR3D9vd4emSkJSww4aohj9GeEEvYghtm
 Vj87iUR88swIqY+glDaoTxsVOS6+pbfLxIsHscAFQ0SITtSvQ0e7EVmU5Z9wlBDTx+/+ZIlXi
 UDr+014tBJ0O/uGZxuDtpda7RG0Czgz5aZ0hpdSCII2aPmJLVEN53n1j0gPQZV762WkGmA2y8
 Mm8pnp/L1iXaORZUZRlpiBVgZ7C+gMwxsluyRL21dxGUyY68V96dB6+iKpfuMUvLImjlQBr4A
 wHdXpYG/A43Q6Jplv9BxsQkaV9PcZPygnN8Wpn04SlMueOVrlZtRlM8Jnn4CLRAGwm3axUlc+
 rDktZ2U+dCg/5DWYkupdrRGrW7NdzGymXUq0GXVw3NCA4mLmVspTssPufbylnBo0wTsy5oq5O
 xtP5hMXiNXpnYQC69t7SqE60vN/KGGqeySaEQ0ptnQLPGvviuRynNv+tpr5IARt/7tvOFBFvB
 2KuRFKRgSiIhdKP9Jn9xb7NVyYphSzud+AdLW3hhoj1Kw6fhjvQxq/koJc9DMzCnKPujilaPx
 uaBh/zc8dOgP9WYE8sm2tMyt8TCtFXGvrV58OHn8MvYFydzYAx06k4zzpBkLTDcSxWqvcUTNV
 6/Fn4olk3Y8VXZgw/EcF9D8aVAnuwAlWh8e5YEP9QeTkH7qgpygEBitTeB7sT8LmrCbzpyMcy
 mH7L922+NNHaUIDt
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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