Re: [PATCH] Applying inappropriate ioctl operation on socket shouldreturn ENOTTY
From: Alan Cox
Date: Wed Apr 27 2011 - 08:08:01 EST
> diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
> index 25373df..50c09e4 100644
> --- a/drivers/char/applicom.c
> +++ b/drivers/char/applicom.c
> @@ -838,6 +838,6 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> Dummy = readb(apbs[IndexCard].RamIO + VERS);
> kfree(adgl);
> mutex_unlock(&ac_mutex);
> - return 0;
> + return ret;
> }
This one in fact is a bug fix where 0 gets returned not an error code it
ought to be submitted separately and described as such.
> diff --git a/drivers/char/dtlk.c b/drivers/char/dtlk.c
> index 85156dd..2d116d5 100644
> --- a/drivers/char/dtlk.c
> +++ b/drivers/char/dtlk.c
> @@ -289,7 +289,7 @@ static long dtlk_ioctl(struct file *file,
> return put_user(portval, argp);
>
> default:
> - return -EINVAL;
> + return -ENOTTY;
> }
> }
This one looks good (and the driver has another error in the ioctl
handler too that wants fixing where it returnds -EINVAL not -EFAULT)
>
> diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> index 0e941b5..95278e9 100644
> --- a/drivers/char/generic_nvram.c
> +++ b/drivers/char/generic_nvram.c
> @@ -111,7 +111,7 @@ static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> nvram_sync();
> break;
> default:
> - return -EINVAL;
> + return -ENOTTY;
> }
Looks good
> return 0;
> diff --git a/drivers/char/genrtc.c b/drivers/char/genrtc.c
> index f773a9d..6f4c3da 100644
> --- a/drivers/char/genrtc.c
> +++ b/drivers/char/genrtc.c
> @@ -330,7 +330,7 @@ static int gen_rtc_ioctl(struct file *file,
> }
> }
>
> - return -EINVAL;
> + return -ENOTTY;
> }
Likewise
> static long gen_rtc_unlocked_ioctl(struct file *file, unsigned int cmd,
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 7066e80..720de66 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -575,7 +575,7 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
> case HPET_IE_ON:
> return hpet_ioctl_ieon(devp);
> default:
> - return -EINVAL;
> + return -ENOTTY;
> }
Ok
> err = 0;
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index d72433f..4ba9b9f 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -370,7 +370,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
> break;
>
> default:
> - return -EINVAL;
> + return -ENOTTY;
This one is incomplete - the driver also has a bogus check for arg being
non zero. That means ioctl(fd, BOGUS, 0) will return the wrong error code
still.
> }
>
> if (val < 0)
> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> index 2aa3977..bc8af5a 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -232,7 +232,7 @@ static int ipmi_ioctl(struct file *file,
> unsigned int cmd,
> unsigned long data)
> {
> - int rv = -EINVAL;
> + int rv = -ENOTTY;
> struct ipmi_file_private *priv = file->private_data;
> void __user *arg = (void __user *)data;
No - there are cases that should return -EINVAL that this will break - a
default case needs adding
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 97c3edb..2ff32c8 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -650,7 +650,7 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
> break;
>
> default:
> - retval = -EINVAL;
> + retval = -ENOTTY;
> }
> return retval;
Looks good
> }
> diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
> index a12f524..45b7a7a 100644
> --- a/drivers/char/nwflash.c
> +++ b/drivers/char/nwflash.c
> @@ -115,7 +115,7 @@ static long flash_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> gbWriteBase64Enable = 0;
> gbWriteEnable = 0;
> mutex_unlock(&flash_mutex);
> - return -EINVAL;
> + return -ENOTTY;
Ok
> }
> mutex_unlock(&flash_mutex);
> return 0;
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index f176dba..8dce214 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -622,7 +622,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
> default:
> pr_debug(CHRDEV "%x: What? (cmd=0x%x)\n", minor, cmd);
> - return -EINVAL;
> + return -ENOTTY;
> }
Looks good
>
> /* Keep the compiler happy */
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d4ddeba..40aad1c 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1157,7 +1157,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> rand_initialize();
> return 0;
> default:
> - return -EINVAL;
> + return -ENOTTY;
Ok
> }
> }
>
> diff --git a/drivers/char/raw.c b/drivers/char/raw.c
> index b4b9d5a..a992bf1 100644
> --- a/drivers/char/raw.c
> +++ b/drivers/char/raw.c
> @@ -231,7 +231,7 @@ static long raw_ctl_ioctl(struct file *filp, unsigned int command,
> return 0;
> }
>
> - return -EINVAL;
> + return -ENOTTY;
Ok
> }
>
> #ifdef CONFIG_COMPAT
> @@ -273,7 +273,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
> return 0;
> }
>
> - return -EINVAL;
> + return -ENOTTY;
> }
> #endif
Ok
>
> diff --git a/drivers/char/viotape.c b/drivers/char/viotape.c
> index ad6e64a..a427d40 100644
> --- a/drivers/char/viotape.c
> +++ b/drivers/char/viotape.c
> @@ -529,7 +529,7 @@ static int viotap_ioctl(struct inode *inode, struct file *file,
>
> down(&reqSem);
>
> - ret = -EINVAL;
> + ret = -ENOTTY;
Again this messes up the returns because code assumes the initial
default. The original code also has bugs too (wrong error off
copy_*_user() again)
>
> switch (cmd) {
> case MTIOCTOP:
> diff --git a/fs/pipe.c b/fs/pipe.c
> index da42f7d..fe7ffe4 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -665,7 +665,7 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>
> return put_user(count, (int __user *)arg);
> default:
> - return -EINVAL;
> + return -ENOTTY;
> }
Looks good - but this one really does want to be a patch on its own as if
anything causes compatibility funnies it will be this, and we need to be
sure we can bisect nicely to it should this occur.
> }
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c2ac599..b93c76d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4773,7 +4773,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
> * is never reached
> */
> WARN_ON(1);
> - err = -EINVAL;
> + err = -ENOTTY;
This case doesn't really matter - it can't happen anyway so might as well
change
> break;
>
> }
> @@ -5041,7 +5041,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
> /* Set the per device memory buffer space.
> * Not applicable in our case */
> case SIOCSIFLINK:
> - return -EINVAL;
> + return -EOPNOTSUPP;
This change seems unrelated to anything in your description and outside
of anything SuS cares about or demands.
>
> /*
> * Unknown or private ioctl.
> @@ -5062,7 +5062,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
> /* Take care of Wireless Extensions */
> if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
> return wext_handle_ioctl(net, &ifr, cmd, arg);
> - return -EINVAL;
> + return -ENOTTY;
and this one looks right.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/