| 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
| ||
|
Message-ID: <20090324174057.47f7da3d@bike.lwn.net>
Date: Tue, 24 Mar 2009 17:40:57 -0600
From: Jonathan Corbet <corbet@....net>
To: stoyboyker@...il.com
Cc: linux-kernel@...r.kernel.org,
Stoyan Gaydarov <stoyboyker@...il.com>, gregkh@...e.de
Subject: Re: [PATCH 01/13] [staging] changed ioctls to unlocked
OK, looking at the patch now...
> drivers/staging/epl/EplApiLinuxKernel.c | 10 ++-
> drivers/staging/meilhaus/memain.c | 124 +++++++++++++++++++++----------
> drivers/staging/poch/poch.c | 25 +++++--
> 3 files changed, 107 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/epl/EplApiLinuxKernel.c b/drivers/staging/epl/EplApiLinuxKernel.c
> index 05ca062..9517da2 100644
> --- a/drivers/staging/epl/EplApiLinuxKernel.c
> +++ b/drivers/staging/epl/EplApiLinuxKernel.c
> @@ -219,7 +219,7 @@ static ssize_t EplLinRead(struct file *pInstance_p, char *pDstBuff_p,
> size_t BuffSize_p, loff_t * pFileOffs_p);
> static ssize_t EplLinWrite(struct file *pInstance_p, const char *pSrcBuff_p,
> size_t BuffSize_p, loff_t * pFileOffs_p);
> -static int EplLinIoctl(struct inode *pDeviceFile_p, struct file *pInstance_p,
> +static long EplLinIoctl(struct file *pInstance_p,
> unsigned int uiIoctlCmd_p, unsigned long ulArg_p);
>
> //---------------------------------------------------------------------------
> @@ -237,7 +237,7 @@ static struct file_operations EplLinFileOps_g = {
> .release = EplLinRelease,
> .read = EplLinRead,
> .write = EplLinWrite,
> - .ioctl = EplLinIoctl,
> + .unlocked_ioctl = EplLinIoctl,
>
> };
>
> @@ -551,12 +551,13 @@ static ssize_t EplLinWrite(struct file *pInstance_p, // information about driver
> // -> ioctl(...)
> //---------------------------------------------------------------------------
>
> -static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the device to open
> - struct file *pInstance_p, // information about driver instance
> +static long EplLinIoctl(struct file *pInstance_p, // information about driver instance
> unsigned int uiIoctlCmd_p, // Ioctl command to execute
> unsigned long ulArg_p) // Ioctl command specific argument/parameter
> {
>
> + lock_kernel();
> +
> tEplKernel EplRet;
> int iErr;
> int iRet;
> @@ -1148,6 +1149,7 @@ static int EplLinIoctl(struct inode *pDeviceFile_p, // information about the dev
> Exit:
>
> // TRACE1("EPL: - EplLinIoctl (iRet=%d)\n", iRet);
> + unlock_kernel();
> return (iRet);
>
> }
Gee what ugly code you're working on here. It's like...crap or
something...:)
You should avoid making it worse by putting lock_kernel() above the
variable declarations, though. That will set off alarms for sure, even in
-staging code.
> diff --git a/drivers/staging/meilhaus/memain.c b/drivers/staging/meilhaus/memain.c
> index b09d1a6..9a1706b 100644
> --- a/drivers/staging/meilhaus/memain.c
> +++ b/drivers/staging/meilhaus/memain.c
> @@ -95,7 +95,7 @@ static int replace_with_dummy(int vendor_id, int device_id, int serial_no);
> static void clear_device_list(void);
> static int me_open(struct inode *inode_ptr, struct file *filep);
> static int me_release(struct inode *, struct file *);
> -static int me_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
> +static long me_ioctl(struct file *, unsigned int, unsigned long);
> //static int me_probe_usb(struct usb_interface *interface, const struct usb_device_id *id);
> //static void me_disconnect_usb(struct usb_interface *interface);
>
> @@ -109,7 +109,7 @@ static struct cdev *cdevp;
>
> static struct file_operations me_file_operations = {
> .owner = THIS_MODULE,
> - .ioctl = me_ioctl,
> + .unlocked_ioctl = me_ioctl,
> .open = me_open,
> .release = me_release,
> };
> @@ -1751,14 +1751,17 @@ static void me_disconnect_usb(struct usb_interface *interface)
> }
> */
>
> -static int me_ioctl(struct inode *inodep,
> - struct file *filep, unsigned int service, unsigned long arg)
> +static long me_ioctl(struct file *filep, unsigned int service,
> + unsigned long arg)
> {
> + lock_kernel();
> + long ret;
Please declare the variable first.
> PDEBUG("executed.\n");
>
> if (_IOC_TYPE(service) != MEMAIN_MAGIC) {
> PERROR("Invalid magic number.\n");
> + unlock_kernel();
> return -ENOTTY;
> }
You also clearly do not need to do lock_kernel() before this test, so you
could move it down here and avoid one unlock/return sequence.
> @@ -1766,147 +1769,186 @@ static int me_ioctl(struct inode *inodep,
>
> switch (service) {
> case ME_IO_IRQ_ENABLE:
> - return me_io_irq_start(filep, (me_io_irq_start_t *) arg);
> + ret = me_io_irq_start(filep, (me_io_irq_start_t *) arg);
> + break;
>
> case ME_IO_IRQ_WAIT:
> - return me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
> + ret = me_io_irq_wait(filep, (me_io_irq_wait_t *) arg);
> + break;
>
> case ME_IO_IRQ_DISABLE:
> - return me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
> + ret = me_io_irq_stop(filep, (me_io_irq_stop_t *) arg);
> + break;
>
> case ME_IO_RESET_DEVICE:
> - return me_io_reset_device(filep, (me_io_reset_device_t *) arg);
> + ret = me_io_reset_device(filep, (me_io_reset_device_t *) arg);
> + break;
>
> case ME_IO_RESET_SUBDEVICE:
> - return me_io_reset_subdevice(filep,
> + ret = me_io_reset_subdevice(filep,
> (me_io_reset_subdevice_t *) arg);
> + break;
>
> case ME_IO_SINGLE_CONFIG:
> - return me_io_single_config(filep,
> + ret = me_io_single_config(filep,
> (me_io_single_config_t *) arg);
> + break;
>
> case ME_IO_SINGLE:
> - return me_io_single(filep, (me_io_single_t *) arg);
> + ret = me_io_single(filep, (me_io_single_t *) arg);
> + break;
>
> case ME_IO_STREAM_CONFIG:
> - return me_io_stream_config(filep,
> + ret = me_io_stream_config(filep,
> (me_io_stream_config_t *) arg);
> + break;
>
> case ME_IO_STREAM_NEW_VALUES:
> - return me_io_stream_new_values(filep,
> + ret = me_io_stream_new_values(filep,
> (me_io_stream_new_values_t *)
> arg);
> + break;
>
> case ME_IO_STREAM_READ:
> - return me_io_stream_read(filep, (me_io_stream_read_t *) arg);
> + ret = me_io_stream_read(filep, (me_io_stream_read_t *) arg);
> + break;
>
> case ME_IO_STREAM_START:
> - return me_io_stream_start(filep, (me_io_stream_start_t *) arg);
> + ret = me_io_stream_start(filep, (me_io_stream_start_t *) arg);
> + break;
>
> case ME_IO_STREAM_STATUS:
> - return me_io_stream_status(filep,
> + ret = me_io_stream_status(filep,
> (me_io_stream_status_t *) arg);
> + break;
>
> case ME_IO_STREAM_STOP:
> - return me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
> + ret = me_io_stream_stop(filep, (me_io_stream_stop_t *) arg);
> + break;
>
> case ME_IO_STREAM_WRITE:
> - return me_io_stream_write(filep, (me_io_stream_write_t *) arg);
> + ret = me_io_stream_write(filep, (me_io_stream_write_t *) arg);
> + break;
>
> case ME_LOCK_DRIVER:
> - return me_lock_driver(filep, (me_lock_driver_t *) arg);
> + ret = me_lock_driver(filep, (me_lock_driver_t *) arg);
> + break;
>
> case ME_LOCK_DEVICE:
> - return me_lock_device(filep, (me_lock_device_t *) arg);
> + ret = me_lock_device(filep, (me_lock_device_t *) arg);
> + break;
>
> case ME_LOCK_SUBDEVICE:
> - return me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
> + ret = me_lock_subdevice(filep, (me_lock_subdevice_t *) arg);
> + break;
>
> case ME_QUERY_INFO_DEVICE:
> - return me_query_info_device(filep,
> + ret = me_query_info_device(filep,
> (me_query_info_device_t *) arg);
> + break;
>
> case ME_QUERY_DESCRIPTION_DEVICE:
> - return me_query_description_device(filep,
> + ret = me_query_description_device(filep,
> (me_query_description_device_t
> *) arg);
> + break;
>
> case ME_QUERY_NAME_DEVICE:
> - return me_query_name_device(filep,
> + ret = me_query_name_device(filep,
> (me_query_name_device_t *) arg);
> + break;
>
> case ME_QUERY_NAME_DEVICE_DRIVER:
> - return me_query_name_device_driver(filep,
> + ret = me_query_name_device_driver(filep,
> (me_query_name_device_driver_t
> *) arg);
> + break;
>
> case ME_QUERY_NUMBER_DEVICES:
> - return me_query_number_devices(filep,
> + ret = me_query_number_devices(filep,
> (me_query_number_devices_t *)
> arg);
> + break;
>
> case ME_QUERY_NUMBER_SUBDEVICES:
> - return me_query_number_subdevices(filep,
> + ret = me_query_number_subdevices(filep,
> (me_query_number_subdevices_t
> *) arg);
> + break;
>
> case ME_QUERY_NUMBER_CHANNELS:
> - return me_query_number_channels(filep,
> + ret = me_query_number_channels(filep,
> (me_query_number_channels_t *)
> arg);
> + break;
>
> case ME_QUERY_NUMBER_RANGES:
> - return me_query_number_ranges(filep,
> + ret = me_query_number_ranges(filep,
> (me_query_number_ranges_t *) arg);
> + break;
>
> case ME_QUERY_RANGE_BY_MIN_MAX:
> - return me_query_range_by_min_max(filep,
> + ret = me_query_range_by_min_max(filep,
> (me_query_range_by_min_max_t *)
> arg);
> + break;
>
> case ME_QUERY_RANGE_INFO:
> - return me_query_range_info(filep,
> + ret = me_query_range_info(filep,
> (me_query_range_info_t *) arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_BY_TYPE:
> - return me_query_subdevice_by_type(filep,
> + ret = me_query_subdevice_by_type(filep,
> (me_query_subdevice_by_type_t
> *) arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_TYPE:
> - return me_query_subdevice_type(filep,
> + ret = me_query_subdevice_type(filep,
> (me_query_subdevice_type_t *)
> arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_CAPS:
> - return me_query_subdevice_caps(filep,
> + ret = me_query_subdevice_caps(filep,
> (me_query_subdevice_caps_t *)
> arg);
> + break;
>
> case ME_QUERY_SUBDEVICE_CAPS_ARGS:
> - return me_query_subdevice_caps_args(filep,
> + ret = me_query_subdevice_caps_args(filep,
> (me_query_subdevice_caps_args_t
> *) arg);
> + break;
>
> case ME_QUERY_TIMER:
> - return me_query_timer(filep, (me_query_timer_t *) arg);
> + ret = me_query_timer(filep, (me_query_timer_t *) arg);
> + break;
>
> case ME_QUERY_VERSION_MAIN_DRIVER:
> - return me_query_version_main_driver(filep,
> + ret = me_query_version_main_driver(filep,
> (me_query_version_main_driver_t
> *) arg);
> + break;
>
> case ME_QUERY_VERSION_DEVICE_DRIVER:
> - return me_query_version_device_driver(filep,
> + ret = me_query_version_device_driver(filep,
> (me_query_version_device_driver_t
> *) arg);
> + break;
>
> case ME_CONFIG_LOAD:
> - return me_config_load(filep, (me_config_load_t *) arg);
> + ret = me_config_load(filep, (me_config_load_t *) arg);
> + break;
> + }
> + if(!ret) {
> + PERROR("Invalid ioctl number.\n");
> + ret = -ENOTTY;
> }
Actually, you've changed the control flow a bit, and probably broken the
function too. If it's truly an invalid ioctl number, you'll just drop out
of the switch - there's no default branch. But ret will be a random value
in that case.
In every other case, you take a return code of zero (which means success)
and map it onto -ENOTTY. Probably not the right thing to do.
> - PERROR("Invalid ioctl number.\n");
> - return -ENOTTY;
> + unlock_kernel();
> + return ret;
> }
>
> // Init and exit of module.
> diff --git a/drivers/staging/poch/poch.c b/drivers/staging/poch/poch.c
> index 0d111dd..03cc5b7 100644
> --- a/drivers/staging/poch/poch.c
> +++ b/drivers/staging/poch/poch.c
> @@ -979,9 +979,10 @@ static unsigned int poch_poll(struct file *filp, poll_table *pt)
> return ret;
> }
>
> -static int poch_ioctl(struct inode *inode, struct file *filp,
> - unsigned int cmd, unsigned long arg)
> +static long poch_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> {
> + lock_kernel();
> struct channel_info *channel = filp->private_data;
> void __iomem *fpga = channel->fpga_iomem;
> void __iomem *bridge = channel->bridge_iomem;
After declarations, please. If you're not sure about accesses to channel,
you need to separate the declarations and the initializations of the
variables.
> @@ -1026,8 +1027,10 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
> }
> break;
> case POCH_IOC_GET_COUNTERS:
> - if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters)))
> + if (!access_ok(VERIFY_WRITE, argp, sizeof(struct poch_counters))) {
> + unlock_kernel();
> return -EFAULT;
> + }
In general, you want to have a single unlock/return at the end, and use
goto to get there from inside the function. Lots of returns from the
middle of a function which takes locks can lead to grief sooner or later.
If there's only one path out, things are harder to break.
> spin_lock_irq(&channel->counters_lock);
> counters = channel->counters;
> @@ -1036,23 +1039,31 @@ static int poch_ioctl(struct inode *inode, struct file *filp,
>
> ret = copy_to_user(argp, &counters,
> sizeof(struct poch_counters));
> - if (ret)
> + if (ret) {
> + unlock_kernel();
> return ret;
> + }
>
> break;
> case POCH_IOC_SYNC_GROUP_FOR_USER:
> case POCH_IOC_SYNC_GROUP_FOR_DEVICE:
> vms = find_vma(current->mm, arg);
> - if (!vms)
> + if (!vms) {
> + unlock_kernel();
> /* Address not mapped. */
> return -EINVAL;
> - if (vms->vm_file != filp)
> + }
> + if (vms->vm_file != filp) {
> + unlock_kernel();
> /* Address mapped from different device/file. */
> return -EINVAL;
> + }
>
> flush_cache_range(vms, arg, arg + channel->group_size);
> break;
> }
> +
> + unlock_kernel();
> return 0;
> }
This driver looks like it has other locking problems, incidentally; it has
a spinlock for its counters, but there's nothing serializing access to the
device registers.
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists