[PATCH] char: ppdev: check if ioctl argument is present and valid

[PATCH] char: ppdev: check if ioctl argument is present and valid

[Harshal Chaudhari]

Checking the argument passed to the ioctl is valid
or not. if not then return -EINVAL.

Signed-off-by: Harshal Chaudhari <harshalchau04@gmail.com>
---
 drivers/char/ppdev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 38b46c7d1737..001392980202 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -354,7 +354,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	unsigned int minor = iminor(file_inode(file));
 	struct pp_struct *pp = file->private_data;
 	struct parport *port;
-	void __user *argp = (void __user *)arg;
+	void __user *argp = NULL;
 	struct ieee1284_info *info;
 	unsigned char reg;
 	unsigned char mask;
@@ -364,6 +364,16 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct timespec64 ts;
 	int ret;
 
+	if (_IOC_TYPE(cmd) != PP_IOCTL)
+		return -ENOTTY;
+
+	/* check if ioctl argument is present and valid */
+	if (_IOC_DIR(cmd) != _IOC_NONE) {
+		argp = (void __user *)arg;
+		if (!argp)
+			return -EINVAL;
+	}
+
 	/* First handle the cases that don't take arguments. */
 	switch (cmd) {
 	case PPCLAIM:
-- 
2.17.1

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Arnd Bergmann]

On Thu, Oct 8, 2020 at 8:27 PM Harshal Chaudhari
<harshalchau04@gmail.com> wrote:
>
> Checking the argument passed to the ioctl is valid
> or not. if not then return -EINVAL.
>
> Signed-off-by: Harshal Chaudhari <harshalchau04@gmail.com>
> ---
>  drivers/char/ppdev.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 38b46c7d1737..001392980202 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -354,7 +354,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>         unsigned int minor = iminor(file_inode(file));
>         struct pp_struct *pp = file->private_data;
>         struct parport *port;
> -       void __user *argp = (void __user *)arg;
> +       void __user *argp = NULL;
>         struct ieee1284_info *info;
>         unsigned char reg;
>         unsigned char mask;

Assigning to NULL serves no purpose here.

> @@ -364,6 +364,16 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>         struct timespec64 ts;
>         int ret;
>
> +       if (_IOC_TYPE(cmd) != PP_IOCTL)
> +               return -ENOTTY;

This looks correct but is normally done as a "default" case

> +       /* check if ioctl argument is present and valid */
> +       if (_IOC_DIR(cmd) != _IOC_NONE) {
> +               argp = (void __user *)arg;
> +               if (!argp)
> +                       return -EINVAL;
> +       }

This is a change in behavior, it changes the return code from the correct
-EFAULT to an unusual -EINVAL for the special case that the pointer
is NULL compared to other invalid pointers.

      Arnd

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Greg KH]

On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> Checking the argument passed to the ioctl is valid
> or not. if not then return -EINVAL.

Along the the comments that Arnd made, this is not the correct value to
be returning from an ioctl when you don't pass in the correct command.

And it doesn't match what your patch says, please be consistent.

And do you have this device to be able to test your changes?

thanks,

greg k-h

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Sudip Mukherjee]

On Fri, Oct 9, 2020 at 5:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> > Checking the argument passed to the ioctl is valid
> > or not. if not then return -EINVAL.
>
> Along the the comments that Arnd made, this is not the correct value to
> be returning from an ioctl when you don't pass in the correct command.
>
> And it doesn't match what your patch says, please be consistent.
>
> And do you have this device to be able to test your changes?

I will test this tomorrow. But from an initial look, its going to
break ppdev. There are few ioctls which don't need any arguments.

-- 
Regards
Sudip

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Sudip Mukherjee]

On Sat, Oct 10, 2020 at 1:08 AM Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
>
> On Fri, Oct 9, 2020 at 5:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> > > Checking the argument passed to the ioctl is valid
> > > or not. if not then return -EINVAL.
> >
> > Along the the comments that Arnd made, this is not the correct value to
> > be returning from an ioctl when you don't pass in the correct command.
> >
> > And it doesn't match what your patch says, please be consistent.
> >
> > And do you have this device to be able to test your changes?
>
> I will test this tomorrow. But from an initial look, its going to
> break ppdev. There are few ioctls which don't need any arguments.

No, sorry. I missed the check for _IOC_NONE.
Tested on a desktop which has a parallel port with a very basic test
code of open->claim->write->release->close and it still works.


-- 
Regards
Sudip

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[harshal chaudhari]

On Sat, Oct 10, 2020 at 2:41 PM Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
>
> On Sat, Oct 10, 2020 at 1:08 AM Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> >
> > On Fri, Oct 9, 2020 at 5:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> > > > Checking the argument passed to the ioctl is valid
> > > > or not. if not then return -EINVAL.
> > >
> > > Along the the comments that Arnd made, this is not the correct value to
> > > be returning from an ioctl when you don't pass in the correct command.

Thanks Greg for the comment. i am checking with value
-EFAULT now, i will get back to you with changes as consideration
of Arnd comments.

> > > And it doesn't match what your patch says, please be consistent.

I just want to perform the Argument check here only.  back then i
was trying with access_ok() as well, but access_ok() return success
even if i passed a NULL pointer. so that's why i removed it from here.

> > > And do you have this device to be able to test your changes?

Yes I have a device and I tested these changes with the few ioctls.

> > I will test this tomorrow. But from an initial look, its going to
> > break ppdev. There are few ioctls which don't need any arguments.
>
> No, sorry. I missed the check for _IOC_NONE.
> Tested on a desktop which has a parallel port with a very basic test
> code of open->claim->write->release->close and it still works.

Thanks a lot Sudip for your time.

> --
> Regards
> Sudip

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Greg KH]

On Tue, Oct 13, 2020 at 02:31:21PM +0530, harshal chaudhari wrote:
> On Sat, Oct 10, 2020 at 2:41 PM Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> >
> > On Sat, Oct 10, 2020 at 1:08 AM Sudip Mukherjee
> > <sudipm.mukherjee@gmail.com> wrote:
> > >
> > > On Fri, Oct 9, 2020 at 5:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> > > > > Checking the argument passed to the ioctl is valid
> > > > > or not. if not then return -EINVAL.
> > > >
> > > > Along the the comments that Arnd made, this is not the correct value to
> > > > be returning from an ioctl when you don't pass in the correct command.
> 
> Thanks Greg for the comment. i am checking with value
> -EFAULT now, i will get back to you with changes as consideration
> of Arnd comments.

-EFAULT is only used if there really is a memory fault, which I strongly
doubt is the case here.

> > > > And it doesn't match what your patch says, please be consistent.
> 
> I just want to perform the Argument check here only.  back then i
> was trying with access_ok() as well, but access_ok() return success
> even if i passed a NULL pointer. so that's why i removed it from here.

access_ok() should never be used in drivers anymore.

thanks,

greg k-h

RE: [PATCH] char: ppdev: check if ioctl argument is present and valid

[David Laight]

From: harshal chaudhari
> Sent: 13 October 2020 10:01
> 
> On Sat, Oct 10, 2020 at 2:41 PM Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> >
> > On Sat, Oct 10, 2020 at 1:08 AM Sudip Mukherjee
> > <sudipm.mukherjee@gmail.com> wrote:
> > >
> > > On Fri, Oct 9, 2020 at 5:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Oct 08, 2020 at 11:57:13PM +0530, Harshal Chaudhari wrote:
> > > > > Checking the argument passed to the ioctl is valid
> > > > > or not. if not then return -EINVAL.
> > > >
> > > > Along the the comments that Arnd made, this is not the correct value to
> > > > be returning from an ioctl when you don't pass in the correct command.
> 
> Thanks Greg for the comment. i am checking with value
> -EFAULT now, i will get back to you with changes as consideration
> of Arnd comments.
> 
> > > > And it doesn't match what your patch says, please be consistent.
> 
> I just want to perform the Argument check here only.  back then i
> was trying with access_ok() as well, but access_ok() return success
> even if i passed a NULL pointer. so that's why i removed it from here.

Why bother.
You have to check the copy_from_user() whatever else you might have
checked earlied.
So why optimise for the error case that never happens.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[Arnd Bergmann]

On Sat, Oct 24, 2020 at 5:54 PM harshal chaudhari
<harshalchau04@gmail.com> wrote:
> On Tue, Oct 13, 2020 at 4:42 PM David Laight <David.Laight@aculab.com> wrote:

> So I am a little bit confused about this check whether it's required or not
> Please could you point me in the right direction?
>
> In any case, thanks for your help ...
>
> Here is a driver source located in: linux/drivers/misc/xilinx_sdfec.c
>
> static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd,
> unsigned long data)
> {
> struct xsdfec_dev *xsdfec;
> void __user *arg = NULL;
> int rval = -EINVAL;
>
> if (_IOC_TYPE(cmd) != XSDFEC_MAGIC)
>                return -ENOTTY;
>
> /* check if ioctl argument is present and valid */
> if (_IOC_DIR(cmd) != _IOC_NONE)
> {
>         arg = (void __user *)data;
>         if (!arg)
>                    return rval;
> }
>

All of this can be removed, and replaced with unconditional

     void __user *arg = (void __user *)data;
     int rval;

with an "rval = -ENOTTY" added in the 'default' case. This will
make it behave more like other drivers, returning -ENOTTY for
any unknown ioctl command, and returning -EFAULT for all
invalid pointers, including NULL.

xsdfec_dev_compat_ioctl() can removed as well, with the file operations
changed to an unconditional (removing the #ifdef)

      .compat_ioctl = compat_ptr_ioctl(),

This will not affect behavior, it's just another simplification.


          Arnd

Re: [PATCH] char: ppdev: check if ioctl argument is present and valid

[harshal chaudhari]

On Sun, Oct 25, 2020 at 12:51 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Sat, Oct 24, 2020 at 5:54 PM harshal chaudhari
> <harshalchau04@gmail.com> wrote:
> > On Tue, Oct 13, 2020 at 4:42 PM David Laight <David.Laight@aculab.com> wrote:
>
> > So I am a little bit confused about this check whether it's required or not
> > Please could you point me in the right direction?
> >
> > In any case, thanks for your help ...
> >
> > Here is a driver source located in: linux/drivers/misc/xilinx_sdfec.c
> >
> > static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd,
> > unsigned long data)
> > {
> > struct xsdfec_dev *xsdfec;
> > void __user *arg = NULL;
> > int rval = -EINVAL;
> >
> > if (_IOC_TYPE(cmd) != XSDFEC_MAGIC)
> >                return -ENOTTY;
> >
> > /* check if ioctl argument is present and valid */
> > if (_IOC_DIR(cmd) != _IOC_NONE)
> > {
> >         arg = (void __user *)data;
> >         if (!arg)
> >                    return rval;
> > }
> >
>
> All of this can be removed, and replaced with unconditional
>
>      void __user *arg = (void __user *)data;
>      int rval;
>
> with an "rval = -ENOTTY" added in the 'default' case. This will
> make it behave more like other drivers, returning -ENOTTY for
> any unknown ioctl command, and returning -EFAULT for all
> invalid pointers, including NULL.
>
> xsdfec_dev_compat_ioctl() can removed as well, with the file operations
> changed to an unconditional (removing the #ifdef)
>
>       .compat_ioctl = compat_ptr_ioctl(),
>
> This will not affect behavior, it's just another simplification.

Thanks a lot Arnd. Got the point. and i will make the change
and i will send it to relevant maintainers of that driver file.

Once again thanks!

>           Arnd

RE: [PATCH] char: ppdev: check if ioctl argument is present and valid

[David Laight]

From: Arnd Bergmann
> Sent: 24 October 2020 20:21
> To: harshal chaudhari <harshalchau04@gmail.com>
> Cc: David Laight <David.Laight@ACULAB.COM>; Greg KH <gregkh@linuxfoundation.org>; Sudip Mukherjee
> <sudipm.mukherjee@gmail.com>; linux-kernel <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] char: ppdev: check if ioctl argument is present and valid
> 
> On Sat, Oct 24, 2020 at 5:54 PM harshal chaudhari
> <harshalchau04@gmail.com> wrote:
> > On Tue, Oct 13, 2020 at 4:42 PM David Laight <David.Laight@aculab.com> wrote:
> 
> > So I am a little bit confused about this check whether it's required or not
> > Please could you point me in the right direction?
> >
> > In any case, thanks for your help ...
> >
> > Here is a driver source located in: linux/drivers/misc/xilinx_sdfec.c
> >
> > static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd,
> > unsigned long data)
> > {
> > struct xsdfec_dev *xsdfec;
> > void __user *arg = NULL;
> > int rval = -EINVAL;
> >
> > if (_IOC_TYPE(cmd) != XSDFEC_MAGIC)
> >                return -ENOTTY;
> >
> > /* check if ioctl argument is present and valid */
> > if (_IOC_DIR(cmd) != _IOC_NONE)
> > {
> >         arg = (void __user *)data;
> >         if (!arg)
> >                    return rval;
> > }
> >
> 
> All of this can be removed, and replaced with unconditional
> 
>      void __user *arg = (void __user *)data;
>      int rval;
> 
> with an "rval = -ENOTTY" added in the 'default' case. This will
> make it behave more like other drivers, returning -ENOTTY for
> any unknown ioctl command, and returning -EFAULT for all
> invalid pointers, including NULL.

Yep, the thing to remember is that even if you actually
verified that the user buffer could be accessed on entry
to the ioctl code another application thread could unmap
the memory area before you do a later access.

What you may want to do is copy the user buffer into a
kernel buffer at the top of the ioctl code and write it
back at the bottom.
But do make absolutely sure you don't overflow the kernel
buffer of access beyond its end.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)