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@xxxxxxxxx>
> Cc: David Laight <David.Laight@xxxxxxxxxx>; Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Sudip Mukherjee
> <sudipm.mukherjee@xxxxxxxxx>; linux-kernel <linux-kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx> wrote:
> > On Tue, Oct 13, 2020 at 4:42 PM David Laight <David.Laight@xxxxxxxxxx> 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)