Re: Linux Kernel i2c Integer Overflow Vulnerability

[Alexander Nyberg <alexn () telia com>]

> --- vuln code ---
> ssize_t i2cproc_bus_read(struct file * file, char *
> buf,size_t count, 
>                           loff_t *ppos)
>  {
>          struct inode * inode =
> file->f_dentry->d_inode;
>          char *kbuf;
>          struct i2c_client *client;
>          int i,j,k,order_nr,len=0;
>          size_t len_total;
>          int order[I2C_CLIENT_MAX];
>  
>          if (count > 4000)
>                  return -EINVAL; 
>          len_total = file->f_pos + count;
>          /* Too bad if this gets longer (unlikely) */
>          if (len_total > 4000)
>                  len_total = 4000;
>          for (i = 0; i < I2C_ADAP_MAX; i++)
>                  if (adapters[i]->inode ==
> inode->i_ino) {
>                  /* We need a bit of slack in the
> kernel buffer; this makes the
>                     sprintf safe. */
>                          if (! (kbuf = kmalloc(count +
> 80,GFP_KERNEL)))
>                                  return -ENOMEM;
>
> [...]
>
> --- end snippet ---
>
> Although a quick check is made to ensure that the
> user-supplied variable 'count' does not exceed 4000,
> sanity checks do not occur to check for negative
> integers in 'count'.  Since negative integers simply
> become _very_ large integers when represented as
> unsigned, a negative count argument to kmalloc() would
> cause unexpected behavior:

On i386:
typedef __kernel_size_t         size_t;
typedef unsigned int    __kernel_size_t;

asmlinkage ssize_t sys_read(unsigned int fd, char * buf, size_t count);
                                                        ^^^^^^^

You can't pass negative values, all archs have unsigned definitions of
size_t. Therefor "if (count > 4000)" is an ok range check.


> ---
>  if (! (kbuf = kmalloc(count + 80,GFP_KERNEL)))
> ---
>
> For example, if '-1' was passed to the routine as the
> 'count' argument, the above kmalloc() call would be
> equivalent to below:
>
> ---
>  if (! (kbuf = kmalloc(0xffffffff + 80,GFP_KERNEL)))
> ---
>
> This would cause an integer overflow during the
> kmalloc() call when 80 is added to count, resulting in
> a very small amount of memory being allocated.
>
> As in the comment just above the vulnerable kmalloc()
> call (/* We need a bit of slack in the kernel buffer;
> this makes the sprintf safe. */), the purpose of
> incrementing the 'count' argument by 80 is to stop the
> chance of a buffer overflow, but by supplying a
> suitable negative integer as 'count' (i.e -1), this
> allows an integer overflow, causing the kmalloc()
> argument to wrap back round to a small/negative value.
>
> In the sprintf() calls following the kmalloc() call,
> there is quite a possibility of overflowing the bounds
> of the newly allocated very small chunk of memory. 
> This might result in kernel panic, corruption of
> kernel memory, or maybe even elevation of privileges
> (*very* unlikely).
>
> i2cproc_bus_read() is implemented as a read() hook in
> the driver, as below:
>
> ---
> static struct file_operations i2cproc_operations = {
>          read:           i2cproc_bus_read,
>  };
> ---
>
> This might allow unprivileged users to exploit the
> issue.
>
> Please take note that this potential security hole
> only affects those using the i2c driver -- if this
> driver (it can be installed as either a module or
> built into the kernel) is not installed on your
> system, you're not vulnerable.  The issue is present
> in all 2.4 kernels, including the latest release.
>
>
>
> Solution
> #########
>
> The following sanity check can be added to the
> beginning of the i2cproc_bus_read() in the i2c-core.c
> file:
>
> ---
> if(count < 0)
>         return -EINVAL;
> ---
>
> Then rebuild the kernel, and the issue should be
> resolved.
>
> A possible workaround would be to perhaps disable the
> module or remove the driver if it's not needed on your
> system.
>
>
>
> Disclaimer
> ###########
>
> The information contained within this advisory was
> believed to be accurate at the time of it's
> publishing.  However, it might be inaccurate at times,
> so don't consider any information contained within
> definitely correct.
>
> Direct flames to /dev/null.  Don't bothering wasting
> your time and mine with any crap about any disclosure
> policies I may or may not have followed -- I'm not
> interested, so I'll just ignore you if you don't
> phrase things nicely.
>
>
>
> Thank you for your time.
> Shaun.
>
>
>       
>       
>               
> ___________________________________________________________ALL-NEW Yahoo! Messenger - sooooo many all-new ways to 
> express yourself http://uk.messenger.yahoo.com