Re: [PATCH] [FAT] fix VFAT compat ioctls on 64-bit systems

[Bart Oldeman <bartoldeman@users.sourceforge.net>]

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 */