While doing some compat_ioctl conversions I noticed a few issues
in compat_sys_ioctl:
- It is not completely compatible to old ->ioctl because
the traditional common ioctls are not checked before it. I added
a check for those. The main advantage is that the handler
now works the same as a traditional handler even when it returns
-EINVAL
- The private socket ioctl check should only apply for sockets.
- There was a security hook missing. Drawback is that it uses
the same hook now, and the LSM module cannot distingush between
32bit and 64bit clients. But it'll have to live with that for now.
Signed-off-by: Andi Kleen <[email protected]>
diff -u linux-2.6.11-rc1-bk4/fs/ioctl.c-o linux-2.6.11-rc1-bk4/fs/ioctl.c
--- linux-2.6.11-rc1-bk4/fs/ioctl.c-o 2005-01-17 10:39:40.000000000 +0100
+++ linux-2.6.11-rc1-bk4/fs/ioctl.c 2005-01-17 21:57:09.000000000 +0100
@@ -78,6 +78,10 @@
}
+/*
+ * When you add any new common ioctls to the switches above and below
+ * please update compat_sys_ioctl() too.
+ */
asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
struct file * filp;
diff -u linux-2.6.11-rc1-bk4/fs/compat.c-o linux-2.6.11-rc1-bk4/fs/compat.c
--- linux-2.6.11-rc1-bk4/fs/compat.c-o 2005-01-17 10:39:40.000000000 +0100
+++ linux-2.6.11-rc1-bk4/fs/compat.c 2005-01-18 08:04:11.000000000 +0100
@@ -25,6 +25,7 @@
#include <linux/file.h>
#include <linux/vfs.h>
#include <linux/ioctl32.h>
+#include <linux/ioctl.h>
#include <linux/init.h>
#include <linux/sockios.h> /* for SIOCDEVPRIVATE */
#include <linux/smb.h>
@@ -47,6 +48,7 @@
#include <asm/uaccess.h>
#include <asm/mmu_context.h>
+#include <asm/ioctls.h>
/*
* Not all architectures have sys_utime, so implement this in terms
@@ -437,16 +439,41 @@
if (!filp)
goto out;
- if (filp->f_op && filp->f_op->compat_ioctl) {
- error = filp->f_op->compat_ioctl(filp, cmd, arg);
- if (error != -ENOIOCTLCMD)
- goto out_fput;
- }
-
- if (!filp->f_op ||
- (!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl))
- goto do_ioctl;
+ /*
+ * To allow the compat_ioctl handlers to be self contained
+ * we need to check the common ioctls here first.
+ * Just handle them with the standard handlers below.
+ */
+ switch (cmd) {
+ case FIOCLEX:
+ case FIONCLEX:
+ case FIONBIO:
+ case FIOASYNC:
+ case FIOQSIZE:
+ break;
+
+ case FIBMAP:
+ case FIGETBSZ:
+ case FIONREAD:
+ if (S_ISREG(filp->f_dentry->d_inode->i_mode))
+ break;
+ /*FALL THROUGH*/
+ default:
+ if (filp->f_op && filp->f_op->compat_ioctl) {
+ error = filp->f_op->compat_ioctl(filp, cmd, arg);
+ if (error != -ENOIOCTLCMD)
+ goto out_fput;
+ }
+
+ if (!filp->f_op ||
+ (!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl))
+ goto do_ioctl;
+ break;
+ }
+
+ /* When register_ioctl32_conversion is finally gone remove
+ this lock! -AK */
down_read(&ioctl32_sem);
for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
if (t->cmd == cmd)
@@ -454,7 +481,8 @@
}
up_read(&ioctl32_sem);
- if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
+ if (S_ISSOCK(filp->f_dentry->d_inode->i_mode) &&
+ cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
error = siocdevprivate_ioctl(fd, cmd, arg);
} else {
static int count;
@@ -468,6 +496,11 @@
found_handler:
if (t->handler) {
+ /* RED-PEN how should LSM module know it's handling 32bit? */
+ error = security_file_ioctl(filp, cmd, arg);
+ if (error)
+ goto out_fput;
+
lock_kernel();
error = t->handler(fd, cmd, arg, filp);
unlock_kernel();
Hi, Andi!
Quoting r. Andi Kleen ([email protected]) "[PATCH] Some fixes for compat ioctl":
>
> While doing some compat_ioctl conversions I noticed a few issues
> in compat_sys_ioctl:
>
> - It is not completely compatible to old ->ioctl because
> the traditional common ioctls are not checked before it.
How is this different from what we have for compat_sys_ioctl
in 2.6.10? Or is this an old bug?
> I added
> a check for those.
Cant we just add them to fs/compat_ioctl.c instead, and be done?
> The main advantage is that the handler
> now works the same as a traditional handler even when it returns
> -EINVAL
We still need the conversion functions in fs/compat_ioctl.c, I
think. If that is true, for some devices the handler only almost works
if it returns -EINVAL, and maybe its best not to encourage this.
I have another idea: maybe, lets move the unlocked_ioctl handler up so
that it, too, is required to return -NOIOCTLCMD?
I would argue it also may improve ioctl performance by a small margin,
since it is the unlocked_ioctl/compat_ioctl that do the "real" work.
I plan to send, separately, a patch that does this.
Please comment.
>
> - The private socket ioctl check should only apply for sockets.
Its an old issue, isnt it? And probably better split in a separate
patch?
> - There was a security hook missing. Drawback is that it uses
> the same hook now, and the LSM module cannot distingush between
> 32bit and 64bit clients. But it'll have to live with that for now.
It seems it is still missing for compat_ioctl. No?
I am sending a patch to fix this separately.
MST
Attached patch is against 2.6.11-rc1-bk5
Signed-off-by: Michael S. Tsirkin <[email protected]>
Add a missing security hook for compatibility ioctl.
diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
--- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
+++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
@@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
if (!filp)
goto out;
+ /* RED-PEN how should LSM module know it's handling 32bit? */
+ error = security_file_ioctl(filp, cmd, arg);
+ if (error)
+ goto out_fput;
+
if (filp->f_op && filp->f_op->compat_ioctl) {
error = filp->f_op->compat_ioctl(filp, cmd, arg);
if (error != -ENOIOCTLCMD)
Attached patch is against 2.6.11-rc1-bk5.
It is split out from Andi's big patch.
It is really unchanged so I dont put a signed-off-by here.
Signed-off-by: Andi Kleen <[email protected]>
SIOCDEVPRIVATE ioctl command only applies to socket descriptors.
diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
--- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
+++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
@@ -454,7 +460,8 @@ asmlinkage long compat_sys_ioctl(unsigne
}
up_read(&ioctl32_sem);
- if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
+ if (S_ISSOCK(filp->f_dentry->d_inode->i_mode) &&
+ cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
error = siocdevprivate_ioctl(fd, cmd, arg);
} else {
static int count;
Attached patch is against 2.6.11-rc1-bk5
A piece is copied from Andi's patch, too. No idea if
his SOB should be here too. Here it is just in case.
Signed-off-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>
Make common ioctls apply for compat_ioctl.
diff -rup linux-2.6.10-orig/fs/compat_ioctl.c linux-2.6.10-ioctl-sym/fs/compat_ioctl.c
--- linux-2.6.10-orig/fs/compat_ioctl.c 2004-12-24 23:36:01.000000000 +0200
+++ linux-2.6.10-ioctl-sym/fs/compat_ioctl.c 2005-01-18 10:54:31.736650344 +0200
@@ -3165,6 +3165,14 @@ static int do_ncp_setprivatedata(unsigne
#endif
#ifdef DECLARES
+HANDLE_IOCTL(FIOCLEX,NULL)
+HANDLE_IOCTL(FIONCLEX,NULL)
+HANDLE_IOCTL(FIONBIO,NULL)
+HANDLE_IOCTL(FIOASYNC,NULL)
+HANDLE_IOCTL(FIOQSIZE,NULL)
+HANDLE_IOCTL(FIBMAP,NULL)
+HANDLE_IOCTL(FIGETBSZ,NULL)
+HANDLE_IOCTL(FIONREAD,NULL)
HANDLE_IOCTL(MEMREADOOB32, mtd_rw_oob)
HANDLE_IOCTL(MEMWRITEOOB32, mtd_rw_oob)
#ifdef CONFIG_NET
diff -rup linux-2.6.10-orig/fs/ioctl.c linux-2.6.10-ioctl-sym/fs/ioctl.c
--- linux-2.6.10-orig/fs/ioctl.c 2005-01-18 10:58:33.609880024 +0200
+++ linux-2.6.10-ioctl-sym/fs/ioctl.c 2005-01-18 10:51:55.690372984 +0200
@@ -77,7 +72,10 @@ static int file_ioctl(struct file *filp,
return do_ioctl(filp, cmd, arg);
}
-
+/*
+ * When you add any new common ioctls to the switches above and below
+ * please update compat_ioctl.c too.
+ */
asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
struct file * filp;
On Tue, Jan 18, 2005 at 12:48:16PM +0200, Michael S. Tsirkin wrote:
> Attached patch is against 2.6.11-rc1-bk5.
> It is split out from Andi's big patch.
> It is really unchanged so I dont put a signed-off-by here.
>
> Signed-off-by: Andi Kleen <[email protected]>
>
> SIOCDEVPRIVATE ioctl command only applies to socket descriptors.
>
> diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
> --- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
> +++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
> @@ -454,7 +460,8 @@ asmlinkage long compat_sys_ioctl(unsigne
> }
> up_read(&ioctl32_sem);
>
> - if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
> + if (S_ISSOCK(filp->f_dentry->d_inode->i_mode) &&
> + cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
> error = siocdevprivate_ioctl(fd, cmd, arg);
Maybe this should move into a new sock_compat_ioctl() instead?
This is just a split off from Andi's patch, so I didnt add my SOB here.
Signed-off-by: Andi Kleen <[email protected]>
Since its too early to deprecate (un)register_ioctl32_conversion,
add a comment for that time when we do.
diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
--- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
+++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
@@ -447,6 +452,7 @@ asmlinkage long compat_sys_ioctl(unsigne
(!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl))
goto do_ioctl;
+ /* When register_ioctl32_conversion is gone remove this lock! -AK */
down_read(&ioctl32_sem);
for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
if (t->cmd == cmd)
> > - if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
> > + if (S_ISSOCK(filp->f_dentry->d_inode->i_mode) &&
> > + cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
> > error = siocdevprivate_ioctl(fd, cmd, arg);
>
> Maybe this should move into a new sock_compat_ioctl() instead?
>
Seems like overkill for 3 lines of code.
-Andi
Signed-off-by: Michael S. Tsirkin <[email protected]>
Make unlocked_ioctl and compat_ioctl behave symmetrically -
check them first thing, and always require returning ENOIOCTLCMD
on error from unlocked_ioctl, same as we do for compat_ioctl.
This also makes it possible to override *all* ioctl commands, and
hopefully may enable some speed ups on the data path.
diff -rup linux-2.6.10-orig/fs/ioctl.c linux-2.6.10-ioctl-sym/fs/ioctl.c
--- linux-2.6.10-orig/fs/ioctl.c 2005-01-18 10:58:33.609880024 +0200
+++ linux-2.6.10-ioctl-sym/fs/ioctl.c 2005-01-18 10:51:55.690372984 +0200
@@ -24,12 +24,7 @@ static long do_ioctl(struct file *filp,
if (!filp->f_op)
goto out;
- if (filp->f_op->unlocked_ioctl) {
- error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
- if (error == -ENOIOCTLCMD)
- error = -EINVAL;
- goto out;
- } else if (filp->f_op->ioctl) {
+ if (filp->f_op->ioctl) {
lock_kernel();
error = filp->f_op->ioctl(filp->f_dentry->d_inode,
filp, cmd, arg);
@@ -93,6 +91,12 @@ asmlinkage long sys_ioctl(unsigned int f
if (error)
goto out_fput;
+ if (filp->f_op && filp->f_op->unlocked_ioctl) {
+ error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
+ if (error != -ENOIOCTLCMD)
+ goto out_fput;
+ }
+
switch (cmd) {
case FIOCLEX:
set_close_on_exec(fd, 1);
* Michael S. Tsirkin ([email protected]) wrote:
> diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
> --- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
> +++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
> @@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
> if (!filp)
> goto out;
>
> + /* RED-PEN how should LSM module know it's handling 32bit? */
> + error = security_file_ioctl(filp, cmd, arg);
> + if (error)
> + goto out_fput;
> +
This is now called twice in the plain do_ioctl: case. A generic vfs handler
could alleviate that.
===== fs/ioctl.c 1.15 vs edited =====
--- 1.15/fs/ioctl.c 2005-01-15 14:31:01 -08:00
+++ edited/fs/ioctl.c 2005-01-18 11:18:33 -08:00
@@ -77,21 +77,10 @@ static int file_ioctl(struct file *filp,
return do_ioctl(filp, cmd, arg);
}
-
-asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
{
- struct file * filp;
unsigned int flag;
- int on, error = -EBADF;
- int fput_needed;
-
- filp = fget_light(fd, &fput_needed);
- if (!filp)
- goto out;
-
- error = security_file_ioctl(filp, cmd, arg);
- if (error)
- goto out_fput;
+ int on, error = 0;
switch (cmd) {
case FIOCLEX:
@@ -157,6 +146,24 @@ asmlinkage long sys_ioctl(unsigned int f
error = do_ioctl(filp, cmd, arg);
break;
}
+ return error;
+}
+
+asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+{
+ struct file * filp;
+ int error = -EBADF;
+ int fput_needed;
+
+ filp = fget_light(fd, &fput_needed);
+ if (!filp)
+ goto out;
+
+ error = security_file_ioctl(filp, cmd, arg);
+ if (error)
+ goto out_fput;
+
+ error = vfs_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
===== fs/compat.c 1.48 vs edited =====
--- 1.48/fs/compat.c 2005-01-15 14:31:01 -08:00
+++ edited/fs/compat.c 2005-01-18 11:07:56 -08:00
@@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
if (!filp)
goto out;
+ /* RED-PEN how should LSM module know it's handling 32bit? */
+ error = security_file_ioctl(filp, cmd, arg);
+ if (error)
+ goto out_fput;
+
if (filp->f_op && filp->f_op->compat_ioctl) {
error = filp->f_op->compat_ioctl(filp, cmd, arg);
if (error != -ENOIOCTLCMD)
@@ -477,7 +482,7 @@ asmlinkage long compat_sys_ioctl(unsigne
up_read(&ioctl32_sem);
do_ioctl:
- error = sys_ioctl(fd, cmd, arg);
+ error = vfs_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
===== include/linux/fs.h 1.373 vs edited =====
--- 1.373/include/linux/fs.h 2005-01-15 14:31:01 -08:00
+++ edited/include/linux/fs.h 2005-01-18 11:10:54 -08:00
@@ -1564,6 +1564,8 @@ extern int vfs_stat(char __user *, struc
extern int vfs_lstat(char __user *, struct kstat *);
extern int vfs_fstat(unsigned int, struct kstat *);
+extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
+
extern struct file_system_type *get_fs_type(const char *name);
extern struct super_block *get_super(struct block_device *);
extern struct super_block *user_get_super(dev_t);
Hello!
Quoting r. Chris Wright ([email protected]) "Re: [PATCH 1/5] compat_ioctl call seems to miss a security hook":
> * Michael S. Tsirkin ([email protected]) wrote:
> > diff -rup linux-2.6.10-orig/fs/compat.c linux-2.6.10-ioctl-sym/fs/compat.c
> > --- linux-2.6.10-orig/fs/compat.c 2005-01-18 10:58:33.609880024 +0200
> > +++ linux-2.6.10-ioctl-sym/fs/compat.c 2005-01-18 10:54:26.289478440 +0200
> > @@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
> > if (!filp)
> > goto out;
> >
> > + /* RED-PEN how should LSM module know it's handling 32bit? */
> > + error = security_file_ioctl(filp, cmd, arg);
> > + if (error)
> > + goto out_fput;
> > +
>
> This is now called twice in the plain do_ioctl: case. A generic vfs handler
> could alleviate that.
I'm all for it, but the way the patch below works, we could end up
calling ->ioctl or ->unlocked_ioctl from the compat
syscall, and we dont want that.
MST
> ===== fs/ioctl.c 1.15 vs edited =====
> --- 1.15/fs/ioctl.c 2005-01-15 14:31:01 -08:00
> +++ edited/fs/ioctl.c 2005-01-18 11:18:33 -08:00
> @@ -77,21 +77,10 @@ static int file_ioctl(struct file *filp,
> return do_ioctl(filp, cmd, arg);
> }
>
> -
> -asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
> +int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
> {
> - struct file * filp;
> unsigned int flag;
> - int on, error = -EBADF;
> - int fput_needed;
> -
> - filp = fget_light(fd, &fput_needed);
> - if (!filp)
> - goto out;
> -
> - error = security_file_ioctl(filp, cmd, arg);
> - if (error)
> - goto out_fput;
> + int on, error = 0;
>
> switch (cmd) {
> case FIOCLEX:
> @@ -157,6 +146,24 @@ asmlinkage long sys_ioctl(unsigned int f
> error = do_ioctl(filp, cmd, arg);
> break;
> }
> + return error;
> +}
> +
> +asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
> +{
> + struct file * filp;
> + int error = -EBADF;
> + int fput_needed;
> +
> + filp = fget_light(fd, &fput_needed);
> + if (!filp)
> + goto out;
> +
> + error = security_file_ioctl(filp, cmd, arg);
> + if (error)
> + goto out_fput;
> +
> + error = vfs_ioctl(filp, fd, cmd, arg);
> out_fput:
> fput_light(filp, fput_needed);
> out:
> ===== fs/compat.c 1.48 vs edited =====
> --- 1.48/fs/compat.c 2005-01-15 14:31:01 -08:00
> +++ edited/fs/compat.c 2005-01-18 11:07:56 -08:00
> @@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
> if (!filp)
> goto out;
>
> + /* RED-PEN how should LSM module know it's handling 32bit? */
> + error = security_file_ioctl(filp, cmd, arg);
> + if (error)
> + goto out_fput;
> +
> if (filp->f_op && filp->f_op->compat_ioctl) {
> error = filp->f_op->compat_ioctl(filp, cmd, arg);
> if (error != -ENOIOCTLCMD)
> @@ -477,7 +482,7 @@ asmlinkage long compat_sys_ioctl(unsigne
>
> up_read(&ioctl32_sem);
> do_ioctl:
> - error = sys_ioctl(fd, cmd, arg);
> + error = vfs_ioctl(filp, fd, cmd, arg);
> out_fput:
> fput_light(filp, fput_needed);
> out:
> ===== include/linux/fs.h 1.373 vs edited =====
> --- 1.373/include/linux/fs.h 2005-01-15 14:31:01 -08:00
> +++ edited/include/linux/fs.h 2005-01-18 11:10:54 -08:00
> @@ -1564,6 +1564,8 @@ extern int vfs_stat(char __user *, struc
> extern int vfs_lstat(char __user *, struct kstat *);
> extern int vfs_fstat(unsigned int, struct kstat *);
>
> +extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
> +
> extern struct file_system_type *get_fs_type(const char *name);
> extern struct super_block *get_super(struct block_device *);
> extern struct super_block *user_get_super(dev_t);
* Michael S. Tsirkin ([email protected]) wrote:
> I'm all for it, but the way the patch below works, we could end up
> calling ->ioctl or ->unlocked_ioctl from the compat
> syscall, and we dont want that.
Hmm, I didn't actually change how those are called. So if it's an issue,
then I don't think this patch introduces it.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoting r. Chris Wright ([email protected]) "Re: [PATCH 1/5] compat_ioctl call seems to miss a security hook":
> * Michael S. Tsirkin ([email protected]) wrote:
> > I'm all for it, but the way the patch below works, we could end up
> > calling ->ioctl or ->unlocked_ioctl from the compat
> > syscall, and we dont want that.
>
> Hmm, I didn't actually change how those are called. So if it's an issue,
> then I don't think this patch introduces it.
>
> thanks,
> -chris
Sorry, you are right, we go to do_ioctl only if there are no
callbacks.
Patch looks good to go to me.
mst
* Michael S. Tsirkin ([email protected]) wrote:
> Quoting r. Chris Wright ([email protected]) "Re: [PATCH 1/5] compat_ioctl call seems to miss a security hook":
> > * Michael S. Tsirkin ([email protected]) wrote:
> > > I'm all for it, but the way the patch below works, we could end up
> > > calling ->ioctl or ->unlocked_ioctl from the compat
> > > syscall, and we dont want that.
> >
> > Hmm, I didn't actually change how those are called. So if it's an issue,
> > then I don't think this patch introduces it.
>
> Sorry, you are right, we go to do_ioctl only if there are no
> callbacks.
I suppose there is one case (not introduced by the patch). Not sure if
it's even a problem though:
t->cmd matches, yet NULL t->handler. This will fall-thru to
the do_ioctl: case. I assume NULL handler is for case where no
conversion is needed, so it's not a problem? At least some callers of
register_ioctl32_conversion() pass NULL handler.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoting r. Chris Wright ([email protected])
> > > > I'm all for it, but the way the patch below works, we could end up
> > > > calling ->ioctl or ->unlocked_ioctl from the compat
> > > > syscall, and we dont want that.
> > >
> > > Hmm, I didn't actually change how those are called. So if it's an issue,
> > > then I don't think this patch introduces it.
> >
> > Sorry, you are right, we go to do_ioctl only if there are no
> > callbacks.
>
> I suppose there is one case (not introduced by the patch). Not sure if
> it's even a problem though:
>
> t->cmd matches, yet NULL t->handler. This will fall-thru to
> the do_ioctl: case. I assume NULL handler is for case where no
> conversion is needed, so it's not a problem? At least some callers of
> register_ioctl32_conversion() pass NULL handler.
Yes, this is an by design, you put in a NULL handler to say: I dont need
conversions, call my regular handlers. Some in-tree drivers do this.
MST