Hi all,
This is my first pass at compat_sys_fcntl and compat_sys_fcntl64.
I am hoping it is OK with everyone.
This is just the generic part of the patch, arch's to follow.
As a side effect, this allows the removal of struct flock64 from all
the 64 bit architectures.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
diff -ruN 2.5.63-32bit.1/fs/compat.c 2.5.63-32bit.2/fs/compat.c
--- 2.5.63-32bit.1/fs/compat.c 2003-01-14 09:57:57.000000000 +1100
+++ 2.5.63-32bit.2/fs/compat.c 2003-02-25 14:35:59.000000000 +1100
@@ -75,36 +75,6 @@
return error;
}
-int get_compat_flock(struct flock *kfl, struct compat_flock *ufl)
-{
- int err;
-
- if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)))
- return -EFAULT;
-
- err = __get_user(kfl->l_type, &ufl->l_type);
- err |= __get_user(kfl->l_whence, &ufl->l_whence);
- err |= __get_user(kfl->l_start, &ufl->l_start);
- err |= __get_user(kfl->l_len, &ufl->l_len);
- err |= __get_user(kfl->l_pid, &ufl->l_pid);
- return err;
-}
-
-int put_compat_flock(struct flock *kfl, struct compat_flock *ufl)
-{
- int err;
-
- if (!access_ok(VERIFY_WRITE, ufl, sizeof(*ufl)))
- return -EFAULT;
-
- err = __put_user(kfl->l_type, &ufl->l_type);
- err |= __put_user(kfl->l_whence, &ufl->l_whence);
- err |= __put_user(kfl->l_start, &ufl->l_start);
- err |= __put_user(kfl->l_len, &ufl->l_len);
- err |= __put_user(kfl->l_pid, &ufl->l_pid);
- return err;
-}
-
static int put_compat_statfs(struct compat_statfs *ubuf, struct statfs *kbuf)
{
if (verify_area(VERIFY_WRITE, ubuf, sizeof(*ubuf)) ||
@@ -159,3 +129,120 @@
out:
return error;
}
+
+static int get_compat_flock(struct flock *kfl, struct compat_flock *ufl)
+{
+ if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)) ||
+ __get_user(kfl->l_type, &ufl->l_type) ||
+ __get_user(kfl->l_whence, &ufl->l_whence) ||
+ __get_user(kfl->l_start, &ufl->l_start) ||
+ __get_user(kfl->l_len, &ufl->l_len) ||
+ __get_user(kfl->l_pid, &ufl->l_pid))
+ return -EFAULT;
+ return 0;
+}
+
+static int put_compat_flock(struct flock *kfl, struct compat_flock *ufl)
+{
+ if (!access_ok(VERIFY_WRITE, ufl, sizeof(*ufl)) ||
+ __put_user(kfl->l_type, &ufl->l_type) ||
+ __put_user(kfl->l_whence, &ufl->l_whence) ||
+ __put_user(kfl->l_start, &ufl->l_start) ||
+ __put_user(kfl->l_len, &ufl->l_len) ||
+ __put_user(kfl->l_pid, &ufl->l_pid))
+ return -EFAULT;
+ return 0;
+}
+
+static int get_compat_flock64(struct flock *kfl, struct compat_flock64 *ufl)
+{
+ if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)) ||
+ __get_user(kfl->l_type, &ufl->l_type) ||
+ __get_user(kfl->l_whence, &ufl->l_whence) ||
+ __get_user(kfl->l_start, &ufl->l_start) ||
+ __get_user(kfl->l_len, &ufl->l_len) ||
+ __get_user(kfl->l_pid, &ufl->l_pid))
+ return -EFAULT;
+ return 0;
+}
+
+static int put_compat_flock64(struct flock *kfl, struct compat_flock64 *ufl)
+{
+ if (!access_ok(VERIFY_WRITE, ufl, sizeof(*ufl)) ||
+ __put_user(kfl->l_type, &ufl->l_type) ||
+ __put_user(kfl->l_whence, &ufl->l_whence) ||
+ __put_user(kfl->l_start, &ufl->l_start) ||
+ __put_user(kfl->l_len, &ufl->l_len) ||
+ __put_user(kfl->l_pid, &ufl->l_pid))
+ return -EFAULT;
+ return 0;
+}
+
+extern asmlinkage long sys_fcntl(unsigned int, unsigned int, unsigned long);
+
+asmlinkage long compat_sys_fcntl64(unsigned int fd, unsigned int cmd,
+ unsigned long arg)
+{
+ mm_segment_t old_fs;
+ struct flock f;
+ long ret;
+
+ switch (cmd) {
+ case F_GETLK:
+ case F_SETLK:
+ case F_SETLKW:
+ ret = get_compat_flock(&f, (struct compat_flock *)arg);
+ if (ret != 0)
+ break;
+ old_fs = get_fs();
+ set_fs(KERNEL_DS);
+ ret = sys_fcntl(fd, cmd, (unsigned long)&f);
+ set_fs(old_fs);
+ if ((cmd == F_GETLK) && (ret == 0)) {
+ if ((f.l_start >= COMPAT_OFF_T_MAX) ||
+ ((f.l_start + f.l_len) >= COMPAT_OFF_T_MAX))
+ ret = -EOVERFLOW;
+ if (ret == 0)
+ ret = put_compat_flock(&f,
+ (struct compat_flock *)arg);
+ }
+ break;
+
+ case F_GETLK64:
+ case F_SETLK64:
+ case F_SETLKW64:
+ ret = get_compat_flock64(&f, (struct compat_flock64 *)arg);
+ if (ret != 0)
+ break;
+ old_fs = get_fs();
+ set_fs(KERNEL_DS);
+ ret = sys_fcntl(fd, F_GETLK, (unsigned long)&f);
+ ret = sys_fcntl(fd, (cmd == F_GETLK64) ? F_GETLK :
+ ((cmd == F_SETLK64) ? F_SETLK : F_SETLKW),
+ (unsigned long)&f);
+ set_fs(old_fs);
+ if ((cmd == F_GETLK64) && (ret == 0)) {
+ if ((f.l_start >= COMPAT_LOFF_T_MAX) ||
+ ((f.l_start + f.l_len) >= COMPAT_LOFF_T_MAX))
+ ret = -EOVERFLOW;
+ if (ret == 0)
+ ret = put_compat_flock64(&f,
+ (struct compat_flock64 *)arg);
+ }
+ break;
+
+ default:
+ ret = sys_fcntl(fd, cmd, arg);
+ break;
+ }
+ return ret;
+}
+
+asmlinkage long compat_sys_fcntl(unsigned int fd, unsigned int cmd,
+ unsigned long arg)
+{
+ if ((cmd == F_GETLK64) || (cmd == F_SETLK64) || (cmd == F_SETLKW64))
+ return -EINVAL;
+ return compat_sys_fcntl64(fd, cmd, arg);
+}
+
diff -ruN 2.5.63-32bit.1/include/linux/compat.h 2.5.63-32bit.2/include/linux/compat.h
--- 2.5.63-32bit.1/include/linux/compat.h 2003-01-17 14:01:07.000000000 +1100
+++ 2.5.63-32bit.2/include/linux/compat.h 2003-02-25 14:36:00.000000000 +1100
@@ -10,7 +10,6 @@
#include <linux/stat.h>
#include <linux/param.h> /* for HZ */
-#include <linux/fcntl.h> /* for struct flock */
#include <asm/compat.h>
#define compat_jiffies_to_clock_t(x) \
@@ -40,8 +39,6 @@
} compat_sigset_t;
extern int cp_compat_stat(struct kstat *, struct compat_stat *);
-extern int get_compat_flock(struct flock *, struct compat_flock *);
-extern int put_compat_flock(struct flock *, struct compat_flock *);
extern int get_compat_timespec(struct timespec *, struct compat_timespec *);
extern int put_compat_timespec(struct timespec *, struct compat_timespec *);
diff -ruN 2.5.63-32bit.1/include/linux/fs.h 2.5.63-32bit.2/include/linux/fs.h
--- 2.5.63-32bit.1/include/linux/fs.h 2003-02-25 12:59:59.000000000 +1100
+++ 2.5.63-32bit.2/include/linux/fs.h 2003-02-25 14:36:00.000000000 +1100
@@ -525,8 +525,10 @@
extern int fcntl_getlk(struct file *, struct flock *);
extern int fcntl_setlk(struct file *, unsigned int, struct flock *);
+#if BITS_PER_LONG == 32
extern int fcntl_getlk64(struct file *, struct flock64 *);
extern int fcntl_setlk64(struct file *, unsigned int, struct flock64 *);
+#endif
/* fs/locks.c */
extern void locks_init_lock(struct file_lock *);
On Fri, Feb 28, 2003 at 05:33:49AM +0100, Stephen Rothwell wrote:
> +static int get_compat_flock(struct flock *kfl, struct compat_flock *ufl)
> +{
> + if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)) ||
> + __get_user(kfl->l_type, &ufl->l_type) ||
> + __get_user(kfl->l_whence, &ufl->l_whence) ||
> + __get_user(kfl->l_start, &ufl->l_start) ||
> + __get_user(kfl->l_len, &ufl->l_len) ||
> + __get_user(kfl->l_pid, &ufl->l_pid))
Perhaps there should be really a big fat comment on top of compat.c
that it depends on a hole on __PAGE_OFFSET if the arch allows passing
64bit pointers to the compat functions.
> +
> +asmlinkage long compat_sys_fcntl(unsigned int fd, unsigned int cmd,
> + unsigned long arg)
> +{
> + if ((cmd == F_GETLK64) || (cmd == F_SETLK64) || (cmd == F_SETLKW64))
> + return -EINVAL;
That won't work for IA32 emulation. There are programs that call
old style fcntl() with F_*LK64. Just drop the if here.
> + return compat_sys_fcntl64(fd, cmd, arg);
-Andi
Hi Andi,
Thanks for the comments.
From: Andi Kleen <[email protected]>
>
> On Fri, Feb 28, 2003 at 05:33:49AM +0100, Stephen Rothwell wrote:
> > +static int get_compat_flock(struct flock *kfl, struct compat_flock *ufl)
> > +{
> > + if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)) ||
> > + __get_user(kfl->l_type, &ufl->l_type) ||
> > + __get_user(kfl->l_whence, &ufl->l_whence) ||
> > + __get_user(kfl->l_start, &ufl->l_start) ||
> > + __get_user(kfl->l_len, &ufl->l_len) ||
> > + __get_user(kfl->l_pid, &ufl->l_pid))
>
> Perhaps there should be really a big fat comment on top of compat.c
> that it depends on a hole on __PAGE_OFFSET if the arch allows passing
> 64bit pointers to the compat functions.
One of my tasks is to document all the assumptions in hte comapt code
(and there a a few - and I find more as I go :-)). However, could you
elaborate a bit here - do you mean passing 64 bit pointers from user mode?
> > +asmlinkage long compat_sys_fcntl(unsigned int fd, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + if ((cmd == F_GETLK64) || (cmd == F_SETLK64) || (cmd == F_SETLKW64))
> > + return -EINVAL;
>
> That won't work for IA32 emulation. There are programs that call
> old style fcntl() with F_*LK64. Just drop the if here.
I was going to elaborate on this when I sent the x86_64 part of the patch.
If you read the "normal" kernel fcntl function, it does NOT accept
F_GETLK64, F_SETLK64 or F_SETLKW64 through the fcntl sys call only
through the fcntl64 syscall. So any program that does call the old
style fcntl() with one of those will fail on ia32 - which is what you are
trying to emulate.
Cheers,
Stephen
On Fri, Feb 28, 2003 at 10:50:35AM +0100, [email protected] wrote:
> >
> > On Fri, Feb 28, 2003 at 05:33:49AM +0100, Stephen Rothwell wrote:
> > > +static int get_compat_flock(struct flock *kfl, struct compat_flock *ufl)
> > > +{
> > > + if (!access_ok(VERIFY_READ, ufl, sizeof(*ufl)) ||
> > > + __get_user(kfl->l_type, &ufl->l_type) ||
> > > + __get_user(kfl->l_whence, &ufl->l_whence) ||
> > > + __get_user(kfl->l_start, &ufl->l_start) ||
> > > + __get_user(kfl->l_len, &ufl->l_len) ||
> > > + __get_user(kfl->l_pid, &ufl->l_pid))
> >
> > Perhaps there should be really a big fat comment on top of compat.c
> > that it depends on a hole on __PAGE_OFFSET if the arch allows passing
> > 64bit pointers to the compat functions.
>
> One of my tasks is to document all the assumptions in hte comapt code
> (and there a a few - and I find more as I go :-)). However, could you
> elaborate a bit here - do you mean passing 64 bit pointers from user mode?
On some architectures it may be possible to smuggle 64bit pointers into
function arguments of the 32bit emulation. It should be able to handle
that without security holes.
e.g. on x86-64 it was for some time using ptrace - you could do
a syscall trace and restart a system call and modify the input arguments
again to contain 64bit pointers. I closed this, but there may be similar
holes on other ports. IMHO the 32bit emulation layer should be 64bit
input argument clean to avoid such subtle problems.
Now there used to be some code that did:
if (get_user(a, &userstruct->firstmember) ||
__get_user(b, &userstruct->secondmember))
return -EFAULT;
Assuming that the access_ok in get_user for sizeof(firstmember) covers
secondmember too which doesn't do access_ok in __get_user. This only
works assuming it should handle 64bit pointers when there is a memory
hole at the end of the user process space, otherwise it could
access kernel pages directly after TASK_SIZE. x86-64 has a big enough
hole there, i assume sparc64 and ia64 have too, but i don't know
about the other 64bit ports.
Actually your fcntl code is ok in this regard because it
uses access_ok with the correct argument, but other code sometimes doesn't.
> > > +asmlinkage long compat_sys_fcntl(unsigned int fd, unsigned int cmd,
> > > + unsigned long arg)
> > > +{
> > > + if ((cmd == F_GETLK64) || (cmd == F_SETLK64) || (cmd == F_SETLKW64))
> > > + return -EINVAL;
> >
> > That won't work for IA32 emulation. There are programs that call
> > old style fcntl() with F_*LK64. Just drop the if here.
>
> I was going to elaborate on this when I sent the x86_64 part of the patch.
> If you read the "normal" kernel fcntl function, it does NOT accept
> F_GETLK64, F_SETLK64 or F_SETLKW64 through the fcntl sys call only
> through the fcntl64 syscall. So any program that does call the old
> style fcntl() with one of those will fail on ia32 - which is what you are
> trying to emulate.
Are you sure? I thought it did. But perhaps I'm confusing it with
fcntl64 here. fcntl64 definitely needs to accept the old style calls
(I remember debugging a problem in some application that relied on that)
-Andi
> Now there used to be some code that did:
>
> if (get_user(a, &userstruct->firstmember) ||
> __get_user(b, &userstruct->secondmember))
> return -EFAULT;
>
> Assuming that the access_ok in get_user for sizeof(firstmember) covers
> secondmember too which doesn't do access_ok in __get_user. This only
> works assuming it should handle 64bit pointers when there is a memory
> hole at the end of the user process space, otherwise it could
> access kernel pages directly after TASK_SIZE. x86-64 has a big enough
> hole there, i assume sparc64 and ia64 have too, but i don't know
> about the other 64bit ports.
Yeah there are a bunch of those in the ioctl and syscall translation
code that annoys me. ppc64 is safe too, but its not something we should
rely on.
Anton