Re: [PATCH RFC] staging: comedi: fix user/kernel space access of cmd->chanlist

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2012-09-18 13:11, Ian Abbott wrote:
On 2012-09-18 10:24, Ian Abbott wrote:
On 2012-09-18 01:17, H Hartley Sweeten wrote:
The 'chanlist' in the comedi_cmd struct is an unsigned int __user
pointer.

The do_cmd_ioctl() and do_cmdtest_ioctl() functions in comedi_fops
do a copy_from_user() to move the data from user space to kernel
space before passing the comedi_cmd to the comedi drivers.

Unfortunately, the drivers then think 'chanlist' is still a
__user pointer since thats how the struct is defined.

Make the 'chanlist' a union of both a __user and kernel pointer.
The do_cmd_*_ioctl() functions are the only ones that use the
__user pointer. All the drivers then use the kernel pointer to
access the chanlist.

Personally, I'd rather get rid of the __user pointers in comedi.h and do
the appropriate casting in the comedi core.

On further investigation, it seems that pointers in structs used by
ioctls are generally tagged with __user in the kernel header files.
Still, I don't think the low-level drivers should have to use union
member access to get a kernel pointer.

The only other workaround I can think of is to have kernel versions of
the ioctl data structures, e.g. `struct comedi_kcmd` shadowing `struct
comedi_cmd` with the only difference being the lack of a `__user` tag on
the pointers.  That would be a substantial patch.  Perhaps it could be
broken down by temporarily defining macros such as

    #define comedi_kcmd comedi_cmd
    #define comedi_kinsn comedi_insn

etc. while the drivers are switched over to using `struct comedi_kcmd`
instead of `struct comedi_cmd`, etc.  Once that is done, the new types
can be defined for real (removing the macros) and the comedi core
changed to copy the user types to the kernel types and vice versa.

Actually, since the low-level drivers don't use the `data` member of `struct comedi_insn` (they get passed a separate pointer to the data in kernel space), I don't think we need a `struct comedi_kinsn`. I think we could get away with just `struct comedi_kcmd` and `struct kdevconfig`.

This gives a clear demarcation between kernel types and user types
without silly unions whose sole purpose is to keep `sparse` happy.  I
expect the user types and kernel types to stay in sync, but they don't
have to as long as conversions between the two are handled properly.

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux