Date: Tue, 9 Mar 2004 13:46:48 -0800
From: Andrew Morton <akpm@osdl.org>
To: BlaisorBlade <blaisorblade_spam@yahoo.it>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Missing return value check on do_write_mem
Message-Id: <20040309134648.61e3cb9f.akpm@osdl.org>
In-Reply-To: <200403081246.33897.blaisorblade_spam@yahoo.it>
References: <200403081246.33897.blaisorblade_spam@yahoo.it>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2208
Lines: 66

BlaisorBlade <blaisorblade_spam@yahoo.it> wrote:
>
> In drivers/char/mem.c do_write_mem can return -EFAULT but write_kmem forgets 
> this and goes blindly.
> 
> Note: /dev/kmem can be written to only by root, so this *cannot* have security 
> implications.
> 
> Also, do_write_mem takes two unused params and is static - so I've removed 
> those. I actually double-checked this - however please test compilation on 
> Sparc/m68k, since there are some #ifdef.
> 

It's a small thing, but:

> --- ./drivers/char/mem.c.fix	2004-02-20 16:27:21.000000000 +0100
> +++ ./drivers/char/mem.c	2004-03-08 12:17:23.000000000 +0100
> @@ -96,13 +96,14 @@
>  }
>  #endif
>  
> -static ssize_t do_write_mem(struct file * file, void *p, unsigned long realp,
> -			    const char * buf, size_t count, loff_t *ppos)
> +static ssize_t do_write_mem(void *p, const char * buf, size_t count,
> +			    loff_t *ppos)
>  {
> -	ssize_t written;
> +	ssize_t written = 0;
>  
> -	written = 0;
>  #if defined(__sparc__) || (defined(__mc68000__) && defined(CONFIG_MMU))
> +	unsigned long realp = *ppos;
> +

A thread which shares this fd can alter the value at *ppos at any time via
lseek() .

>  	/* we don't have page 0 mapped on sparc and m68k.. */
>  	if (realp < PAGE_SIZE) {
>  		unsigned long sz = PAGE_SIZE-realp;
> @@ -165,7 +166,7 @@
>  
>  	if (!valid_phys_addr_range(p, &count))
>  		return -EFAULT;
> -	return do_write_mem(file, __va(p), p, buf, count, ppos);
> +	return do_write_mem(__va(p), buf, count, ppos);
>  }
>  
>  static int mmap_mem(struct file * file, struct vm_area_struct * vma)
> @@ -276,7 +277,9 @@
>  		if (count > (unsigned long) high_memory - p)
>  			wrote = (unsigned long) high_memory - p;

Here we have applied a range check.

> -		wrote = do_write_mem(file, (void*)p, p, buf, wrote, ppos);
> +		wrote = do_write_mem((void*)p, buf, wrote, ppos);

But here we go off and use *ppos, which may now have a different value from
that which we just range-checked.

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