[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1087429550.7235.49.camel@boxen>
Date: Thu, 17 Jun 2004 01:45:50 +0200
From: Alexander Nyberg <alexn@...ia.com>
To: Shaun Colley <shaunige@...oo.co.uk>
Cc: bugtraq@...urityfocus.com
Subject: Re: Linux Kernel i2c Integer Overflow Vulnerability
> --- 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
Powered by blists - more mailing lists