Suspicious code in i2c-dev.c

[Frodo Looijaard]

Russell King wrote:

> Looking through i2c-dev.c, I noticed the following code, which appears
> to be wrong:
> 
>   /* copy user space data to kernel space. */
>   tmp = kmalloc(count,GFP_KERNEL);
>   if (tmp==NULL)
>      return -ENOMEM;
> 
> #ifdef DEBUG
>   printk("i2c-dev,o: i2c-%d reading %d bytes.\n",MINOR(inode->i_rdev),count);
> #endif
> 
>   ret = i2c_master_recv(client,tmp,count);
>   copy_to_user(buf,tmp,count);
>   kfree(tmp);
>   return ret;
> 
> for the following reasons:
> 
> 1. If i2c_master_recv does not write to `tmp' but returns an error,
>    copy_to_user will copy uninitialised kernel memory to the user,
>    possibly from another user process - security concern.

Granted. Easily solved with a test whether ret equals zero at this place.
I have applied this to our master archive; we'll include it in the
next diffs we send (probably tomorrow).
 
> 2. copy_to_user may fault, and if this is the case, the user will
>    never know (the user will not see the EFAULT error code returned).

> There are more instances of point 2, including some with copy_from_user.

Hmm. Can you elaborate on that? What exactly will happen when a
copy_{to,from}_user faults? And what does a fault in this instance
mean (cache miss?).

Thanks,
  Frodo

-- 
Frodo Looijaard <frodol en dds.nl>  PGP key and more: http://huizen.dds.nl/~frodol
Defenestration n. (formal or joc.):
  The act of removing Windows from your computer in disgust, usually followed
  by the installation of Linux or some other Unix-like operating system.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo en vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/