Re: [PATCH] Missing return value check on do_write_mem

[BlaisorBlade]

Alle 22:46, martedì 9 marzo 2004, Andrew Morton ha scritto:
> BlaisorBlade <blaisorblade_spam@xxxxxxxx> wrote:
> > In drivers/char/mem.c do_write_mem can return -EFAULT but write_kmem
> > forgets this and goes blindly.

First: do not forget this first fix.

> > Also, do_write_mem takes two unused params and is static - so I've
> > removed those.

The "file" parameter is anyway unused - so it can be removed; for the ppos 
parameter your reasoning holds. I'm posting the patch for these two things 
against 2.6.4 (now I do not remove realp so I avoid the race you describe).

Thanks for answering me - you never lose a patch!

> >  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() .

Well, you are right (not found any locking). I've understood that lseek() is 
known to be racy, and that the bug in my patch is just the range check that 
does not work any more; but anyway the marked line in the 2.6.3/2.6.4 kernel 
is racy, too, and I'm unsure if that is allowed; i.e. man 2 write claims even 
that "POSIX requires  that  a
       read()  which  can  be  proved  to  occur  after a write() has returned
       returns the new data. Note that not all file systems  are  POSIX  con-
       forming." which would imply that write is not racy. Or not?

static ssize_t do_write_mem(struct file * file, void *p, unsigned long realp,
                            const char * buf, size_t count, loff_t *ppos)
{
        ssize_t written;

        written = 0;
#if defined(__sparc__) || (defined(__mc68000__) && defined(CONFIG_MMU))
//[...]
#endif
        if (copy_from_user(p, buf, count))
                return -EFAULT;
        written += count;
        *ppos += written;				/*If a second thread lseek()'d between when we 
read realp and this line, we have a race*/
        return written;
}



-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
--- ./drivers/char/mem.c.fix	2004-03-13 17:56:09.000000000 +0100
+++ ./drivers/char/mem.c	2004-03-13 20:36:35.000000000 +0100
@@ -102,7 +102,7 @@
 }
 #endif
 
-static ssize_t do_write_mem(struct file * file, void *p, unsigned long realp,
+static ssize_t do_write_mem(void *p, unsigned long realp,
 			    const char * buf, size_t count, loff_t *ppos)
 {
 	ssize_t written;
@@ -171,7 +171,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), p, buf, count, ppos);
 }
 
 static int mmap_mem(struct file * file, struct vm_area_struct * vma)
@@ -282,7 +282,9 @@
 		if (count > (unsigned long) high_memory - p)
 			wrote = (unsigned long) high_memory - p;
 
-		wrote = do_write_mem(file, (void*)p, p, buf, wrote, ppos);
+		wrote = do_write_mem((void*)p, p, buf, wrote, ppos);
+		if (wrote < 0)
+			return wrote;
 
 		p += wrote;
 		buf += wrote;