linux-kernel - Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

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]

Message-ID: <7145471.hhkX02hEGi@wuerfel>
Date:   Fri, 02 Sep 2016 11:02:13 +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>,
        Arve Hjønnevåg <arve@...roid.com>,
        Riley Andrews <riandrews@...roid.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>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Brian Starkey <brian.starkey@....com>,
        Chen Feng <puck.chen@...ilicon.com>
Subject: Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:

> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -22,6 +22,29 @@
>  #include "ion_priv.h"
>  #include "compat_ion.h"
>  
> +union ion_ioctl_arg {
> +	struct ion_fd_data fd;
> +	struct ion_allocation_data allocation;
> +	struct ion_handle_data handle;
> +	struct ion_custom_data custom;
> +	struct ion_abi_version abi_version;
> +};

Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).

> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +{
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case ION_IOC_ABI_VERSION:
> +		ret = arg->abi_version.reserved != 0;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret ? -EINVAL : 0;
> +}

I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.

> +/**
> + * struct ion_abi_version
> + *
> + *  @version - current ABI version
> + */
> +
> +#define ION_ABI_VERSION                KERNEL_VERSION(0, 1, 0)
> +
> +struct ion_abi_version {
> +	__u32 abi_version;
> +	__u32 reserved;
> +};
> +

This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.

	Arnd