* [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems
@ 2007-04-29 18:14 Bart Oldeman
2007-04-29 19:54 ` OGAWA Hirofumi
0 siblings, 1 reply; 4+ messages in thread
From: Bart Oldeman @ 2007-04-29 18:14 UTC (permalink / raw)
To: hirofumi; +Cc: linux-kernel
Hello,
if you compile and run the below test case in an msdos or vfat directory
on an x86-64 system with -m32 you'll get garbage in the kernel_dirent
struct followed by a SIGSEGV.
The patch below fixes this. It's a minimal patch -- I haven't converted
spaces to tabs or added extra put_user checks.
The problems:
* d_ino/d_off are undefined for de[0]. Random values from the kernel stack
are copied here into user space.
* d_name, for both de[0] and de[1], is not zero terminated.
* if the long filename in de[1] is empty, d_ino/d_off are also undefined
for de[1].
Signed-off-by: Bart Oldeman <bartoldeman@users.sourceforge.net>
testcase:
#include <sys/types.h>
#include <sys/ioctl.h>
#include <dirent.h>
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
struct kernel_dirent {
long d_ino;
long d_off;
unsigned short d_reclen;
char d_name[256]; /* We must not include limits.h! */
};
#define VFAT_IOCTL_READDIR_BOTH _IOR('r', 1, struct kernel_dirent [2])
#define VFAT_IOCTL_READDIR_SHORT _IOR('r', 2, struct kernel_dirent [2])
int main(void)
{
int fd = open(".", O_RDONLY);
struct kernel_dirent de[2];
while (1) {
int i = ioctl(fd, VFAT_IOCTL_READDIR_BOTH, (long)de);
if (i == -1) break;
if (de[0].d_reclen == 0) break;
printf("SFN: reclen=%2d off=%d ino=%d, %-12s",
de[0].d_reclen, de[0].d_off, de[0].d_ino, de[0].d_name);
if (de[1].d_reclen)
printf("\tLFN: reclen=%2d off=%d ino=%d, %s",
de[1].d_reclen, de[1].d_off, de[1].d_ino, de[1].d_name);
printf("\n");
}
return 0;
}
Patch:
--- linux-2.6/fs/fat/dir.c.orig 2007-04-29 12:41:04.000000000 -0400
+++ linux-2.6/fs/fat/dir.c 2007-04-29 13:59:45.000000000 -0400
@@ -749,14 +749,23 @@ static int fat_dir_ioctl(struct inode *
static long fat_compat_put_dirent32(struct dirent *d,
struct compat_dirent __user *d32)
{
- if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
+ if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent[2])))
return -EFAULT;
- __put_user(d->d_ino, &d32->d_ino);
- __put_user(d->d_off, &d32->d_off);
__put_user(d->d_reclen, &d32->d_reclen);
+ __put_user(0, d32->d_name + d->d_reclen);
if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
return -EFAULT;
+ d++;
+ d32++;
+ __put_user(d->d_reclen, &d32->d_reclen);
+ __put_user(0, d32->d_name + d->d_reclen);
+ if (d->d_reclen) {
+ __put_user(d->d_ino, &d32->d_ino);
+ __put_user(d->d_off, &d32->d_off);
+ if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
+ return -EFAULT;
+ }
return 0;
}
@@ -787,8 +796,7 @@ static long fat_compat_dir_ioctl(struct
unlock_kernel();
set_fs(oldfs);
if (ret >= 0) {
- ret |= fat_compat_put_dirent32(&d[0], p);
- ret |= fat_compat_put_dirent32(&d[1], p + 1);
+ ret |= fat_compat_put_dirent32(d, p);
}
return ret;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems 2007-04-29 18:14 [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems Bart Oldeman @ 2007-04-29 19:54 ` OGAWA Hirofumi 2007-04-29 21:24 ` Bart Oldeman 0 siblings, 1 reply; 4+ messages in thread From: OGAWA Hirofumi @ 2007-04-29 19:54 UTC (permalink / raw) To: Bart Oldeman; +Cc: linux-kernel Hi, Bart Oldeman <bartoldeman@users.sourceforge.net> writes: > if you compile and run the below test case in an msdos or vfat directory > on an x86-64 system with -m32 you'll get garbage in the kernel_dirent > struct followed by a SIGSEGV. > > The patch below fixes this. It's a minimal patch -- I haven't converted > spaces to tabs or added extra put_user checks. I think we should use filldir() callback on compat_ioctl too, and it should fix all bugs of compat code. The untested patch is below, although the code seems not clean enough. What do you think? Thanks. -- OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> [PATCH] fat: fix VFAT compat ioctls on 64-bit systems If you compile and run the below test case in an msdos or vfat directory on an x86-64 system with -m32 you'll get garbage in the kernel_dirent struct followed by a SIGSEGV. The patch fixes this. Reported and initial fix by Bart Oldeman Signed-off-by: Bart Oldeman <bartoldeman@users.sourceforge.net> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> --- fs/fat/dir.c | 199 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 100 insertions(+), 99 deletions(-) diff -puN fs/fat/dir.c~fat-compait_ioctl-fix fs/fat/dir.c --- linux-2.6/fs/fat/dir.c~fat-compait_ioctl-fix 2007-04-30 04:38:17.000000000 +0900 +++ linux-2.6-hirofumi/fs/fat/dir.c 2007-04-30 04:43:56.000000000 +0900 @@ -422,7 +422,7 @@ EODir: EXPORT_SYMBOL_GPL(fat_search_long); struct fat_ioctl_filldir_callback { - struct dirent __user *dirent; + void __user *dirent; int result; /* for dir ioctl */ const char *longname; @@ -647,62 +647,85 @@ static int fat_readdir(struct file *filp return __fat_readdir(inode, filp, dirent, filldir, 0, 0); } -static int fat_ioctl_filldir(void *__buf, const char *name, int name_len, - loff_t offset, u64 ino, unsigned int d_type) +#define FAT_IOCTL_FILLDIR_FUNC(func, dirent_type) \ +static int func(void *__buf, const char *name, int name_len, \ + loff_t offset, u64 ino, unsigned int d_type) \ +{ \ + struct fat_ioctl_filldir_callback *buf = __buf; \ + struct dirent_type __user *d1 = buf->dirent; \ + struct dirent_type __user *d2 = d1 + 1; \ + \ + if (buf->result) \ + return -EINVAL; \ + buf->result++; \ + \ + if (name != NULL) { \ + /* dirent has only short name */ \ + if (name_len >= sizeof(d1->d_name)) \ + name_len = sizeof(d1->d_name) - 1; \ + \ + if (put_user(0, d2->d_name) || \ + put_user(0, &d2->d_reclen) || \ + copy_to_user(d1->d_name, name, name_len) || \ + put_user(0, d1->d_name + name_len) || \ + put_user(name_len, &d1->d_reclen)) \ + goto efault; \ + } else { \ + /* dirent has short and long name */ \ + const char *longname = buf->longname; \ + int long_len = buf->long_len; \ + const char *shortname = buf->shortname; \ + int short_len = buf->short_len; \ + \ + if (long_len >= sizeof(d1->d_name)) \ + long_len = sizeof(d1->d_name) - 1; \ + if (short_len >= sizeof(d1->d_name)) \ + short_len = sizeof(d1->d_name) - 1; \ + \ + if (copy_to_user(d2->d_name, longname, long_len) || \ + put_user(0, d2->d_name + long_len) || \ + put_user(long_len, &d2->d_reclen) || \ + put_user(ino, &d2->d_ino) || \ + put_user(offset, &d2->d_off) || \ + copy_to_user(d1->d_name, shortname, short_len) || \ + put_user(0, d1->d_name + short_len) || \ + put_user(short_len, &d1->d_reclen)) \ + goto efault; \ + } \ + return 0; \ +efault: \ + buf->result = -EFAULT; \ + return -EFAULT; \ +} + +FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, dirent) + +static int fat_ioctl_readdir(struct inode *inode, struct file *filp, + void __user *dirent, filldir_t filldir, + int short_only, int both) { - struct fat_ioctl_filldir_callback *buf = __buf; - struct dirent __user *d1 = buf->dirent; - struct dirent __user *d2 = d1 + 1; - - if (buf->result) - return -EINVAL; - buf->result++; - - if (name != NULL) { - /* dirent has only short name */ - if (name_len >= sizeof(d1->d_name)) - name_len = sizeof(d1->d_name) - 1; - - if (put_user(0, d2->d_name) || - put_user(0, &d2->d_reclen) || - copy_to_user(d1->d_name, name, name_len) || - put_user(0, d1->d_name + name_len) || - put_user(name_len, &d1->d_reclen)) - goto efault; - } else { - /* dirent has short and long name */ - const char *longname = buf->longname; - int long_len = buf->long_len; - const char *shortname = buf->shortname; - int short_len = buf->short_len; - - if (long_len >= sizeof(d1->d_name)) - long_len = sizeof(d1->d_name) - 1; - if (short_len >= sizeof(d1->d_name)) - short_len = sizeof(d1->d_name) - 1; - - if (copy_to_user(d2->d_name, longname, long_len) || - put_user(0, d2->d_name + long_len) || - put_user(long_len, &d2->d_reclen) || - put_user(ino, &d2->d_ino) || - put_user(offset, &d2->d_off) || - copy_to_user(d1->d_name, shortname, short_len) || - put_user(0, d1->d_name + short_len) || - put_user(short_len, &d1->d_reclen)) - goto efault; + struct fat_ioctl_filldir_callback buf; + int ret; + + buf.dirent = dirent; + buf.result = 0; + mutex_lock(&inode->i_mutex); + ret = -ENOENT; + if (!IS_DEADDIR(inode)) { + ret = __fat_readdir(inode, filp, &buf, fat_ioctl_filldir, + short_only, both); } - return 0; -efault: - buf->result = -EFAULT; - return -EFAULT; + mutex_unlock(&inode->i_mutex); + if (ret >= 0) + ret = buf.result; + return ret; } -static int fat_dir_ioctl(struct inode * inode, struct file * filp, - unsigned int cmd, unsigned long arg) +static int fat_dir_ioctl(struct inode *inode, struct file *filp, + unsigned int cmd, unsigned long arg) { - struct fat_ioctl_filldir_callback buf; - struct dirent __user *d1; - int ret, short_only, both; + struct dirent __user *d1 = (struct dirent __user *)arg; + int short_only, both; switch (cmd) { case VFAT_IOCTL_READDIR_SHORT: @@ -717,7 +740,6 @@ static int fat_dir_ioctl(struct inode * return fat_generic_ioctl(inode, filp, cmd, arg); } - d1 = (struct dirent __user *)arg; if (!access_ok(VERIFY_WRITE, d1, sizeof(struct dirent[2]))) return -EFAULT; /* @@ -728,69 +750,48 @@ static int fat_dir_ioctl(struct inode * if (put_user(0, &d1->d_reclen)) return -EFAULT; - buf.dirent = d1; - buf.result = 0; - mutex_lock(&inode->i_mutex); - ret = -ENOENT; - if (!IS_DEADDIR(inode)) { - ret = __fat_readdir(inode, filp, &buf, fat_ioctl_filldir, - short_only, both); - } - mutex_unlock(&inode->i_mutex); - if (ret >= 0) - ret = buf.result; - return ret; + return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir, + short_only, both); } #ifdef CONFIG_COMPAT #define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2]) #define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2]) -static long fat_compat_put_dirent32(struct dirent *d, - struct compat_dirent __user *d32) -{ - if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent))) - return -EFAULT; +FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent) - __put_user(d->d_ino, &d32->d_ino); - __put_user(d->d_off, &d32->d_off); - __put_user(d->d_reclen, &d32->d_reclen); - if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen)) - return -EFAULT; - - return 0; -} - -static long fat_compat_dir_ioctl(struct file *file, unsigned cmd, +static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd, unsigned long arg) { - struct compat_dirent __user *p = compat_ptr(arg); - int ret; - mm_segment_t oldfs = get_fs(); - struct dirent d[2]; + struct inode *inode = filp->f_path.dentry->d_inode; + struct compat_dirent __user *d1 = compat_ptr(arg); + int short_only, both; switch (cmd) { - case VFAT_IOCTL_READDIR_BOTH32: - cmd = VFAT_IOCTL_READDIR_BOTH; - break; case VFAT_IOCTL_READDIR_SHORT32: - cmd = VFAT_IOCTL_READDIR_SHORT; + short_only = 1; + both = 0; + break; + case VFAT_IOCTL_READDIR_BOTH32: + short_only = 0; + both = 1; break; default: return -ENOIOCTLCMD; } - set_fs(KERNEL_DS); - lock_kernel(); - ret = fat_dir_ioctl(file->f_path.dentry->d_inode, file, - cmd, (unsigned long) &d); - unlock_kernel(); - set_fs(oldfs); - if (ret >= 0) { - ret |= fat_compat_put_dirent32(&d[0], p); - ret |= fat_compat_put_dirent32(&d[1], p + 1); - } - return ret; + if (!access_ok(VERIFY_WRITE, d1, sizeof(struct compat_dirent[2]))) + return -EFAULT; + /* + * Yes, we don't need this put_user() absolutely. However old + * code didn't return the right value. So, app use this value, + * in order to check whether it is EOF. + */ + if (put_user(0, &d1->d_reclen)) + return -EFAULT; + + return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir, + short_only, both); } #endif /* CONFIG_COMPAT */ _ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems 2007-04-29 19:54 ` OGAWA Hirofumi @ 2007-04-29 21:24 ` Bart Oldeman 0 siblings, 0 replies; 4+ messages in thread From: Bart Oldeman @ 2007-04-29 21:24 UTC (permalink / raw) To: linux-kernel On 2007-04-29, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote: > > I think we should use filldir() callback on compat_ioctl too, and it > should fix all bugs of compat code. > > The untested patch is below, although the code seems not clean > enough. What do you think? There is one problem: - ret = __fat_readdir(inode, filp, &buf, fat_ioctl_filldir, + ret = __fat_readdir(inode, filp, &buf, filldir, With that change it works and this solution looks cleaner then my patch. Signed-off-by: Bart Oldeman <bartoldeman@users.sourceforge.net> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> --- linux-2.6/fs/fat/dir.c.old 2007-04-29 16:03:55.000000000 -0400 +++ linux-2.6/fs/fat/dir.c 2007-04-29 16:54:43.000000000 -0400 @@ -422,7 +422,7 @@ EODir: EXPORT_SYMBOL_GPL(fat_search_long); struct fat_ioctl_filldir_callback { - struct dirent __user *dirent; + void __user *dirent; int result; /* for dir ioctl */ const char *longname; @@ -647,62 +647,85 @@ static int fat_readdir(struct file *filp return __fat_readdir(inode, filp, dirent, filldir, 0, 0); } -static int fat_ioctl_filldir(void *__buf, const char *name, int name_len, - loff_t offset, u64 ino, unsigned int d_type) +#define FAT_IOCTL_FILLDIR_FUNC(func, dirent_type) \ +static int func(void *__buf, const char *name, int name_len, \ + loff_t offset, u64 ino, unsigned int d_type) \ +{ \ + struct fat_ioctl_filldir_callback *buf = __buf; \ + struct dirent_type __user *d1 = buf->dirent; \ + struct dirent_type __user *d2 = d1 + 1; \ + \ + if (buf->result) \ + return -EINVAL; \ + buf->result++; \ + \ + if (name != NULL) { \ + /* dirent has only short name */ \ + if (name_len >= sizeof(d1->d_name)) \ + name_len = sizeof(d1->d_name) - 1; \ + \ + if (put_user(0, d2->d_name) || \ + put_user(0, &d2->d_reclen) || \ + copy_to_user(d1->d_name, name, name_len) || \ + put_user(0, d1->d_name + name_len) || \ + put_user(name_len, &d1->d_reclen)) \ + goto efault; \ + } else { \ + /* dirent has short and long name */ \ + const char *longname = buf->longname; \ + int long_len = buf->long_len; \ + const char *shortname = buf->shortname; \ + int short_len = buf->short_len; \ + \ + if (long_len >= sizeof(d1->d_name)) \ + long_len = sizeof(d1->d_name) - 1; \ + if (short_len >= sizeof(d1->d_name)) \ + short_len = sizeof(d1->d_name) - 1; \ + \ + if (copy_to_user(d2->d_name, longname, long_len) || \ + put_user(0, d2->d_name + long_len) || \ + put_user(long_len, &d2->d_reclen) || \ + put_user(ino, &d2->d_ino) || \ + put_user(offset, &d2->d_off) || \ + copy_to_user(d1->d_name, shortname, short_len) || \ + put_user(0, d1->d_name + short_len) || \ + put_user(short_len, &d1->d_reclen)) \ + goto efault; \ + } \ + return 0; \ +efault: \ + buf->result = -EFAULT; \ + return -EFAULT; \ +} + +FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, dirent) + +static int fat_ioctl_readdir(struct inode *inode, struct file *filp, + void __user *dirent, filldir_t filldir, + int short_only, int both) { - struct fat_ioctl_filldir_callback *buf = __buf; - struct dirent __user *d1 = buf->dirent; - struct dirent __user *d2 = d1 + 1; - - if (buf->result) - return -EINVAL; - buf->result++; - - if (name != NULL) { - /* dirent has only short name */ - if (name_len >= sizeof(d1->d_name)) - name_len = sizeof(d1->d_name) - 1; - - if (put_user(0, d2->d_name) || - put_user(0, &d2->d_reclen) || - copy_to_user(d1->d_name, name, name_len) || - put_user(0, d1->d_name + name_len) || - put_user(name_len, &d1->d_reclen)) - goto efault; - } else { - /* dirent has short and long name */ - const char *longname = buf->longname; - int long_len = buf->long_len; - const char *shortname = buf->shortname; - int short_len = buf->short_len; - - if (long_len >= sizeof(d1->d_name)) - long_len = sizeof(d1->d_name) - 1; - if (short_len >= sizeof(d1->d_name)) - short_len = sizeof(d1->d_name) - 1; - - if (copy_to_user(d2->d_name, longname, long_len) || - put_user(0, d2->d_name + long_len) || - put_user(long_len, &d2->d_reclen) || - put_user(ino, &d2->d_ino) || - put_user(offset, &d2->d_off) || - copy_to_user(d1->d_name, shortname, short_len) || - put_user(0, d1->d_name + short_len) || - put_user(short_len, &d1->d_reclen)) - goto efault; + struct fat_ioctl_filldir_callback buf; + int ret; + + buf.dirent = dirent; + buf.result = 0; + mutex_lock(&inode->i_mutex); + ret = -ENOENT; + if (!IS_DEADDIR(inode)) { + ret = __fat_readdir(inode, filp, &buf, filldir, + short_only, both); } - return 0; -efault: - buf->result = -EFAULT; - return -EFAULT; + mutex_unlock(&inode->i_mutex); + if (ret >= 0) + ret = buf.result; + return ret; } -static int fat_dir_ioctl(struct inode * inode, struct file * filp, - unsigned int cmd, unsigned long arg) +static int fat_dir_ioctl(struct inode *inode, struct file *filp, + unsigned int cmd, unsigned long arg) { - struct fat_ioctl_filldir_callback buf; - struct dirent __user *d1; - int ret, short_only, both; + struct dirent __user *d1 = (struct dirent __user *)arg; + int short_only, both; switch (cmd) { case VFAT_IOCTL_READDIR_SHORT: @@ -717,7 +740,6 @@ static int fat_dir_ioctl(struct inode * return fat_generic_ioctl(inode, filp, cmd, arg); } - d1 = (struct dirent __user *)arg; if (!access_ok(VERIFY_WRITE, d1, sizeof(struct dirent[2]))) return -EFAULT; /* @@ -728,69 +750,48 @@ static int fat_dir_ioctl(struct inode * if (put_user(0, &d1->d_reclen)) return -EFAULT; - buf.dirent = d1; - buf.result = 0; - mutex_lock(&inode->i_mutex); - ret = -ENOENT; - if (!IS_DEADDIR(inode)) { - ret = __fat_readdir(inode, filp, &buf, fat_ioctl_filldir, - short_only, both); - } - mutex_unlock(&inode->i_mutex); - if (ret >= 0) - ret = buf.result; - return ret; + return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir, + short_only, both); } #ifdef CONFIG_COMPAT #define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2]) #define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2]) -static long fat_compat_put_dirent32(struct dirent *d, - struct compat_dirent __user *d32) -{ - if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent))) - return -EFAULT; +FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent) - __put_user(d->d_ino, &d32->d_ino); - __put_user(d->d_off, &d32->d_off); - __put_user(d->d_reclen, &d32->d_reclen); - if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen)) - return -EFAULT; - - return 0; -} - -static long fat_compat_dir_ioctl(struct file *file, unsigned cmd, +static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd, unsigned long arg) { - struct compat_dirent __user *p = compat_ptr(arg); - int ret; - mm_segment_t oldfs = get_fs(); - struct dirent d[2]; + struct inode *inode = filp->f_path.dentry->d_inode; + struct compat_dirent __user *d1 = compat_ptr(arg); + int short_only, both; switch (cmd) { - case VFAT_IOCTL_READDIR_BOTH32: - cmd = VFAT_IOCTL_READDIR_BOTH; - break; case VFAT_IOCTL_READDIR_SHORT32: - cmd = VFAT_IOCTL_READDIR_SHORT; + short_only = 1; + both = 0; + break; + case VFAT_IOCTL_READDIR_BOTH32: + short_only = 0; + both = 1; break; default: return -ENOIOCTLCMD; } - set_fs(KERNEL_DS); - lock_kernel(); - ret = fat_dir_ioctl(file->f_path.dentry->d_inode, file, - cmd, (unsigned long) &d); - unlock_kernel(); - set_fs(oldfs); - if (ret >= 0) { - ret |= fat_compat_put_dirent32(&d[0], p); - ret |= fat_compat_put_dirent32(&d[1], p + 1); - } - return ret; + if (!access_ok(VERIFY_WRITE, d1, sizeof(struct compat_dirent[2]))) + return -EFAULT; + /* + * Yes, we don't need this put_user() absolutely. However old + * code didn't return the right value. So, app use this value, + * in order to check whether it is EOF. + */ + if (put_user(0, &d1->d_reclen)) + return -EFAULT; + + return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir, + short_only, both); } #endif /* CONFIG_COMPAT */ ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <Pine.LNX.4.64.0704291730040.32422@localhost.localdomain>]
* Re: [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems [not found] <Pine.LNX.4.64.0704291730040.32422@localhost.localdomain> @ 2007-04-29 23:07 ` OGAWA Hirofumi 0 siblings, 0 replies; 4+ messages in thread From: OGAWA Hirofumi @ 2007-04-29 23:07 UTC (permalink / raw) To: Bart Oldeman; +Cc: linux-kernel, Andrew Morton Bart Oldeman <bartoldeman@users.sourceforge.net> writes: > I found your reply on linux-kernel via an archive but am not subscribed. Sorry. I CCed actually, but sf.net rejected my email configuration. > Anyway, I like your approach but there was one small bug. Thanks for fixing my stupid bug. -- OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> [PATCH] fat: fix VFAT compat ioctls on 64-bit systems If you compile and run the below test case in an msdos or vfat directory on an x86-64 system with -m32 you'll get garbage in the kernel_dirent struct followed by a SIGSEGV. The patch fixes this. Reported and initial fix by Bart Oldeman Signed-off-by: Bart Oldeman <bartoldeman@users.sourceforge.net> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> --- fs/fat/dir.c | 199 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 100 insertions(+), 99 deletions(-) diff -puN fs/fat/dir.c~fat-compait_ioctl-fix fs/fat/dir.c --- linux-2.6/fs/fat/dir.c~fat-compait_ioctl-fix 2007-04-30 07:56:47.000000000 +0900 +++ linux-2.6-hirofumi/fs/fat/dir.c 2007-04-30 07:56:47.000000000 +0900 @@ -422,7 +422,7 @@ EODir: EXPORT_SYMBOL_GPL(fat_search_long); struct fat_ioctl_filldir_callback { - struct dirent __user *dirent; + void __user *dirent; int result; /* for dir ioctl */ const char *longname; @@ -647,62 +647,85 @@ static int fat_readdir(struct file *filp return __fat_readdir(inode, filp, dirent, filldir, 0, 0); } -static int fat_ioctl_filldir(void *__buf, const char *name, int name_len, - loff_t offset, u64 ino, unsigned int d_type) +#define FAT_IOCTL_FILLDIR_FUNC(func, dirent_type) \ +static int func(void *__buf, const char *name, int name_len, \ + loff_t offset, u64 ino, unsigned int d_type) \ +{ \ + struct fat_ioctl_filldir_callback *buf = __buf; \ + struct dirent_type __user *d1 = buf->dirent; \ + struct dirent_type __user *d2 = d1 + 1; \ + \ + if (buf->result) \ + return -EINVAL; \ + buf->result++; \ + \ + if (name != NULL) { \ + /* dirent has only short name */ \ + if (name_len >= sizeof(d1->d_name)) \ + name_len = sizeof(d1->d_name) - 1; \ + \ + if (put_user(0, d2->d_name) || \ + put_user(0, &d2->d_reclen) || \ + copy_to_user(d1->d_name, name, name_len) || \ + put_user(0, d1->d_name + name_len) || \ + put_user(name_len, &d1->d_reclen)) \ + goto efault; \ + } else { \ + /* dirent has short and long name */ \ + const char *longname = buf->longname; \ + int long_len = buf->long_len; \ + const char *shortname = buf->shortname; \ + int short_len = buf->short_len; \ + \ + if (long_len >= sizeof(d1->d_name)) \ + long_len = sizeof(d1->d_name) - 1; \ + if (short_len >= sizeof(d1->d_name)) \ + short_len = sizeof(d1->d_name) - 1; \ + \ + if (copy_to_user(d2->d_name, longname, long_len) || \ + put_user(0, d2->d_name + long_len) || \ + put_user(long_len, &d2->d_reclen) || \ + put_user(ino, &d2->d_ino) || \ + put_user(offset, &d2->d_off) || \ + copy_to_user(d1->d_name, shortname, short_len) || \ + put_user(0, d1->d_name + short_len) || \ + put_user(short_len, &d1->d_reclen)) \ + goto efault; \ + } \ + return 0; \ +efault: \ + buf->result = -EFAULT; \ + return -EFAULT; \ +} + +FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, dirent) + +static int fat_ioctl_readdir(struct inode *inode, struct file *filp, + void __user *dirent, filldir_t filldir, + int short_only, int both) { - struct fat_ioctl_filldir_callback *buf = __buf; - struct dirent __user *d1 = buf->dirent; - struct dirent __user *d2 = d1 + 1; - - if (buf->result) - return -EINVAL; - buf->result++; - - if (name != NULL) { - /* dirent has only short name */ - if (name_len >= sizeof(d1->d_name)) - name_len = sizeof(d1->d_name) - 1; - - if (put_user(0, d2->d_name) || - put_user(0, &d2->d_reclen) || - copy_to_user(d1->d_name, name, name_len) || - put_user(0, d1->d_name + name_len) || - put_user(name_len, &d1->d_reclen)) - goto efault; - } else { - /* dirent has short and long name */ - const char *longname = buf->longname; - int long_len = buf->long_len; - const char *shortname = buf->shortname; - int short_len = buf->short_len; - - if (long_len >= sizeof(d1->d_name)) - long_len = sizeof(d1->d_name) - 1; - if (short_len >= sizeof(d1->d_name)) - short_len = sizeof(d1->d_name) - 1; - - if (copy_to_user(d2->d_name, longname, long_len) || - put_user(0, d2->d_name + long_len) || - put_user(long_len, &d2->d_reclen) || - put_user(ino, &d2->d_ino) || - put_user(offset, &d2->d_off) || - copy_to_user(d1->d_name, shortname, short_len) || - put_user(0, d1->d_name + short_len) || - put_user(short_len, &d1->d_reclen)) - goto efault; + struct fat_ioctl_filldir_callback buf; + int ret; + + buf.dirent = dirent; + buf.result = 0; + mutex_lock(&inode->i_mutex); + ret = -ENOENT; + if (!IS_DEADDIR(inode)) { + ret = __fat_readdir(inode, filp, &buf, filldir, + short_only, both); } - return 0; -efault: - buf->result = -EFAULT; - return -EFAULT; + mutex_unlock(&inode->i_mutex); + if (ret >= 0) + ret = buf.result; + return ret; } -static int fat_dir_ioctl(struct inode * inode, struct file * filp, - unsigned int cmd, unsigned long arg) +static int fat_dir_ioctl(struct inode *inode, struct file *filp, + unsigned int cmd, unsigned long arg) { - struct fat_ioctl_filldir_callback buf; - struct dirent __user *d1; - int ret, short_only, both; + struct dirent __user *d1 = (struct dirent __user *)arg; + int short_only, both; switch (cmd) { case VFAT_IOCTL_READDIR_SHORT: @@ -717,7 +740,6 @@ static int fat_dir_ioctl(struct inode * return fat_generic_ioctl(inode, filp, cmd, arg); } - d1 = (struct dirent __user *)arg; if (!access_ok(VERIFY_WRITE, d1, sizeof(struct dirent[2]))) return -EFAULT; /* @@ -728,69 +750,48 @@ static int fat_dir_ioctl(struct inode * if (put_user(0, &d1->d_reclen)) return -EFAULT; - buf.dirent = d1; - buf.result = 0; - mutex_lock(&inode->i_mutex); - ret = -ENOENT; - if (!IS_DEADDIR(inode)) { - ret = __fat_readdir(inode, filp, &buf, fat_ioctl_filldir, - short_only, both); - } - mutex_unlock(&inode->i_mutex); - if (ret >= 0) - ret = buf.result; - return ret; + return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir, + short_only, both); } #ifdef CONFIG_COMPAT #define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2]) #define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2]) -static long fat_compat_put_dirent32(struct dirent *d, - struct compat_dirent __user *d32) -{ - if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent))) - return -EFAULT; +FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent) - __put_user(d->d_ino, &d32->d_ino); - __put_user(d->d_off, &d32->d_off); - __put_user(d->d_reclen, &d32->d_reclen); - if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen)) - return -EFAULT; - - return 0; -} - -static long fat_compat_dir_ioctl(struct file *file, unsigned cmd, +static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd, unsigned long arg) { - struct compat_dirent __user *p = compat_ptr(arg); - int ret; - mm_segment_t oldfs = get_fs(); - struct dirent d[2]; + struct inode *inode = filp->f_path.dentry->d_inode; + struct compat_dirent __user *d1 = compat_ptr(arg); + int short_only, both; switch (cmd) { - case VFAT_IOCTL_READDIR_BOTH32: - cmd = VFAT_IOCTL_READDIR_BOTH; - break; case VFAT_IOCTL_READDIR_SHORT32: - cmd = VFAT_IOCTL_READDIR_SHORT; + short_only = 1; + both = 0; + break; + case VFAT_IOCTL_READDIR_BOTH32: + short_only = 0; + both = 1; break; default: return -ENOIOCTLCMD; } - set_fs(KERNEL_DS); - lock_kernel(); - ret = fat_dir_ioctl(file->f_path.dentry->d_inode, file, - cmd, (unsigned long) &d); - unlock_kernel(); - set_fs(oldfs); - if (ret >= 0) { - ret |= fat_compat_put_dirent32(&d[0], p); - ret |= fat_compat_put_dirent32(&d[1], p + 1); - } - return ret; + if (!access_ok(VERIFY_WRITE, d1, sizeof(struct compat_dirent[2]))) + return -EFAULT; + /* + * Yes, we don't need this put_user() absolutely. However old + * code didn't return the right value. So, app use this value, + * in order to check whether it is EOF. + */ + if (put_user(0, &d1->d_reclen)) + return -EFAULT; + + return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir, + short_only, both); } #endif /* CONFIG_COMPAT */ _ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-04-29 23:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-04-29 18:14 [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems Bart Oldeman
2007-04-29 19:54 ` OGAWA Hirofumi
2007-04-29 21:24 ` Bart Oldeman
[not found] <Pine.LNX.4.64.0704291730040.32422@localhost.localdomain>
2007-04-29 23:07 ` OGAWA Hirofumi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome