On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
Add compat ioctl in ppdev in order to solve the y2038 issue in later patch. This patch simply add pp_do_ioctl to compat_ioctl, because I found that all the ioctl access the arg as a pointer.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org
drivers/char/ppdev.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index ae0b42b..9207658 100644 --- a/drivers/char/ppdev.c +++ b/drivers/char/ppdev.c @@ -69,6 +69,7 @@ #include <linux/ppdev.h> #include <linux/mutex.h> #include <linux/uaccess.h> +#include <linux/compat.h>
#define PP_VERSION "ppdev: user-space parallel port driver" #define CHRDEV "ppdev" @@ -635,6 +636,11 @@ static long pp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return ret; }
+static long pp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{
return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));+}
static int pp_open (struct inode * inode, struct file * file) { unsigned int minor = iminor(inode); @@ -744,6 +750,9 @@ static const struct file_operations pp_fops = { .write = pp_write, .poll = pp_poll, .unlocked_ioctl = pp_ioctl, +#ifdef CONFIG_COMPAT
.compat_ioctl = pp_compat_ioctl,+#endif
Does adding this patch w/o the following patch break 32bit apps using this on 64bit kernels?
thanks -john
On Wednesday 08 July 2015 13:17:18 John Stultz wrote:
On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
Add compat ioctl in ppdev in order to solve the y2038 issue in later patch. This patch simply add pp_do_ioctl to compat_ioctl, because I found that all the ioctl access the arg as a pointer.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org
I just saw this mail fly by when you replied, but I guess it would have been better to reply when the original mail came.
The description above makes no sense: The problem for compat ioctl is not whether the argument is a pointer or not, but rather what data structure it points to. In this case, we already know that it is /not/ compatible between 32-bit and 64-bit user space, because at least two commands need special handling for the timespec argument, which gets added in patch 4 of the series.
This means patches 3 and 4 have to be swapped in order to allow bisection and not introduce a bug when only this one gets applied but patch 4 is missing.
Moreover, all other ioctl commands that are handled in pp_ioctl need to be checked regarding what their arguments are, including data structures pointed to by the arguments (recursively, if there are again pointers in those structures).
unsigned int minor = iminor(inode);@@ -744,6 +750,9 @@ static const struct file_operations pp_fops = { .write = pp_write, .poll = pp_poll, .unlocked_ioctl = pp_ioctl, +#ifdef CONFIG_COMPAT
.compat_ioctl = pp_compat_ioctl,+#endif
The #ifdef here is not necessary, but will cause a warning on kernels that do not define CONFIG_COMPAT, in particular all 32-bit ones.
Does adding this patch w/o the following patch break 32bit apps using this on 64bit kernels?
Without the patch, those apps will all get -EINVAL from the ioctl handler. With the patch, the kernel actually performs the ioctl that was requested, but that may use the wrong data structure.
Arnd
-
Arnd Bergmann -
John Stultz