Re: [Security] [PATCH] bug in led_proc_write()

Re: [Security] [PATCH] bug in led_proc_write()

[Helge Deller]

* Andrew Morton <akpm@linux-foundation.org>:
> On Sat, 10 Nov 2007 23:50:29 +0100 Ilja <ilja@netric.org> wrote:
> > When reading some of the parisc driver code I stumbled on a bug in
> > led_proc_write() in drivers/parisc/led.c
> > the code looks like:
> > 
> > 182 static int led_proc_write(struct file *file, const char *buf, 
> > 183         unsigned long count, void *data)
> > 184 {
> > 185         char *cur, lbuf[count + 1];
> > 186         int d;
> > 187
> > 188         if (!capable(CAP_SYS_ADMIN))
> > 189                 return -EACCES;
> > ....
> > 235 }
> > 
> > the problem being that the stack is limited and count is not (except for the
> > MAX_INT check done in sys_write() I guess). this could lead to stack
> > corruption (when for example calling capable()). 
> 
> yes, and the bug's still there.  It allows a writer to /proc/pdc/led(?)
> to cause the kernel to consume an unbounded amount of stack. 

Ilja, Andrew,

Thanks for the bug report.
Below is a patch to fix this issue.

Kyle, please apply to the parisc git tree.

Helge

-----------
[PARISC] led.c - fix potential stack overflow in led_proc_write()

avoid potential stack overflow by correctly checking count parameter

Signed-off-by: Helge Deller <deller@gmx.de>


diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 188bc84..d02be78 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -176,16 +176,18 @@ static ssize_t led_proc_write(struct file *file, const char *buf,
 	size_t count, loff_t *pos)
 {
 	void *data = PDE(file->f_path.dentry->d_inode)->data;
-	char *cur, lbuf[count + 1];
+	char *cur, lbuf[32];
 	int d;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	memset(lbuf, 0, count + 1);
+	if (count >= sizeof(lbuf))
+		count = sizeof(lbuf)-1;
 
 	if (copy_from_user(lbuf, buf, count))
 		return -EFAULT;
+	lbuf[count] = 0;
 
 	cur = lbuf;
 

Re: [Security] [PATCH] bug in led_proc_write()

[Kyle McMartin]

On Mon, Aug 02, 2010 at 10:46:41PM +0200, Helge Deller wrote:
> Kyle, please apply to the parisc git tree.
> 

Please send this straight to Linus with my
Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>

I don't want to accidentally mis-place it again this summer.

--Kyle

> Helge
> 
> -----------
> [PARISC] led.c - fix potential stack overflow in led_proc_write()
> 
> avoid potential stack overflow by correctly checking count parameter
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> 
> diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
> index 188bc84..d02be78 100644
> --- a/drivers/parisc/led.c
> +++ b/drivers/parisc/led.c
> @@ -176,16 +176,18 @@ static ssize_t led_proc_write(struct file *file, const char *buf,
>  	size_t count, loff_t *pos)
>  {
>  	void *data = PDE(file->f_path.dentry->d_inode)->data;
> -	char *cur, lbuf[count + 1];
> +	char *cur, lbuf[32];
>  	int d;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
>  
> -	memset(lbuf, 0, count + 1);
> +	if (count >= sizeof(lbuf))
> +		count = sizeof(lbuf)-1;
>  
>  	if (copy_from_user(lbuf, buf, count))
>  		return -EFAULT;
> +	lbuf[count] = 0;
>  
>  	cur = lbuf;
>  
> 
> 
>