Re: Need Help on compat_ioctl implementation

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 24 July 2007, Devesh Sharma wrote:
> Hello all,
> I am facing some difficulty to implement compat_ioctl entry point in
> my driver code, please help me out to sort out the things.
> The ioctl is READ WRITE type,
> It takes one header structure as argument the structure is as follows,
> typedef struct
>     {
>         unsigned int Header;
>         void  *          Data;
>         unsigned int  Size;
>     } IOCTL_HEADER;
> 
> corresponding compat strucutre is
> typedef struct ioctl_header32
> {
>  compat_uint_t  Header ;
>  compat_uptr_t Data ;
>  compat_uint_t Size ;
> }IOCTL_HEADER32 ;
> 
> the IOCTL is expected to fill following structure and return.
> typedef struct
> {
>     unsigned int msgsize;
>     char           msg[512];
> } MSG ;

What driver is that? Is it in the mainline kernel? If not, you
should probably completely redo your ioctl interface, as this
has problems to start with, even for the non-compat case...

>  compat_ioctl entry point is as follows.
> 
> long func_ioctl32(
> 
>             struct file *filp,
> 
>             unsigned int cmd,
> 
>             unsigned long arg)
> {
>     long ret = 0 ;
>     IOCTL_HEADER hdr64 ;
>     IOCTL_HEADER32 __user *hdr32 ;
>     unsigned long tmp ;
>     u64 data ;
>     unsigned long size ;
> 
>     hdr32 = (IOCTL_HEADER32 __user *)compat_ptr(arg) ;
>     __get_user(tmp, &(hdr32->Header)) ;
>     __get_user(data, &(hdr32->Data)) ;
>     __get_user(size, &(hdr32->Size)) ;
> 
>     if(!access_ok(
>                   VERIEY_READ|VERIFY_WRITE,
>                   compat_ptr(data),
>                   sizeof(unsigned long)))
>     {
>         printk("No access to this address\n") ;
>         return -EFAULT ;
>     }

You need to check access_ok() before you attempt __get_user.
Also, you must check the return code from each __get_user,
because access_ok() does not check if the data is actually
readable, only if it is part of the user space address range.

It would be easier to do a single copy_from_user on the entire
32 bit header.

>     hdr64.Header = tmp ;
>     hdr64.Data    = data ;

this needs to do a compat_ptr() conversion.

>     hdr64.Size   = size ;
>     handle_ioctls(filp, cmd, (void *)&hdr64) ; /*This func is specific
> to compat ioctl.*/

your handle_ioctls should not need to be specific to compat ioctl,
it's much easier to use the same code for both cases.

>    return ret;
> }
> 
> In handle_ioctl I am doing following steps.
> {
> ioctl_hdr = (IOCTL_HEADER *)cmd_buff ;
> 
>     switch(cmd)
>     {
>         case IOCTL_MSG_GET :
>              {
>                  MSG32 *msg32 ; /* Compat msg struct is defined.*/

The way you showed the definition, MSG32 is identical to MSG,
so you should not define a new structure.

>                   MSG msg ;
>                  unsigned int i ;
> 
>                  msg32 = (MSG32 __user *)ioctl_hdr->Data ;
>                  msg.msgsize = 128 ;
>                  strcpy(msg.msg, MSG_STR) ; /*MSG_STR is a macro string*/
>                  put_user(msg.msgsize, &(msg32->msgsize)) ; /* This
> works fine.*/
>                  put_user(msg.msg[0], (char *) compat_ptr(msg32->msg))
> ; /* This is giving SEG FAULT */

this is not the place to use compat_ptr. If you want to write a single
character here, you could do it like

	put_user(msg.msg[0], &msg32->msg[0]));

But really, you should not need a compat handler for this part, just
call the native handle_ioctl function on the msg.

>              }
>      }
> }
> 
> Question :
> What is the correct way to perform string copy? Or is this method
> correct? if not what is the correct way?
> Any links or pointers are most welcome

Honestly, your code looks so wrong that it's probably not worth trying
to get just the compat_ioctl part right. Please give a pointer to
the full source.

	Arnd <><