Re: [PATCH/2.4]: do_write_mem() return value check

Re: [PATCH/2.4]: do_write_mem() return value check

[Marcelo Tosatti]

On Wed, Mar 24, 2004 at 07:59:01PM +0100, BlaisorBlade wrote:
> From: Andrew Morton, and me (I did a first fix for 2.6 and sent to him, he 
> checked everything and committed it and I changed the trivial bits for 2.4).
> 
> - remove unused `file *' arg from do_write_mem()
> 
> - Add checking for copy_from_user() failures in do_write_mem()
> 
> (Note: /dev/kmem can be written to only by root, so this *cannot* have 
> security implications)
> 
> - Return correct value from kmem writes() when a fault is encountered.  A
>   write()-style syscall's return values are:
> 
>    0 when nothing was written and there was no error (someone tried to
>    write zero bytes)
> 
>    >0: the number of bytes copied, whether or not there was an error. 
>    Userspace detects errors by noting that the write() return value is less
>    than was requested.
> 
>    <0: there was an error and no bytes were copied
> 
> TODO: Do the same changes for read_mem() and read_kmem(). The code is more 
> messy so I must create do_read_mem() to avoid clumsy counting; I will post the 
> patch first for 2.6.

Hi Paolo, 

This is nice -- although I see it as a 2.6 cleanup only.

Do you actually have any practical problem caused by the current broken 
"always return -EFAULT" ? 

Otherwise leave it for 2.6.

Re: [PATCH/2.4]: do_write_mem() return value check

[BlaisorBlade]

Alle 04:33, venerdì 26 marzo 2004, Marcelo Tosatti ha scritto:
> On Wed, Mar 24, 2004 at 07:59:01PM +0100, BlaisorBlade wrote:
> > From: Andrew Morton, and me (I did a first fix for 2.6 and sent to him,
> > he checked everything and committed it and I changed the trivial bits for
> > 2.4).

> Do you actually have any practical problem caused by the current broken
> "always return -EFAULT" ?

It is not only that, there can be also data corruption:

- The  "always return -EFAULT" one is a little bug, so you can discard the 
first and third hunk which fix that; anyway, it was Andrew Morton who noticed 
that, so I've no idea (I agree with you but I'm a kernel newbie). Note I 
posted a 2nd version of the patch which fixes a bug in the third hunk (see at 
the end).

- BUT in the hunk below (the 2nd in the patch), without the patch, the var 
"wrote" can be == -EFAULT and WE CAN DO
		p += -EFAULT, 
                buf += -EFAULT;

so that we can read, afterwards, some bytes of junk from buf - EFAULT and put 
them into p - EFAULT, *corrupting memory* !!!!

> > - Add checking for copy_from_user() failures in do_write_mem()

        if (p < (unsigned long) high_memory) {
+               ssize_t written;
+
                wrote = count;
                if (count > (unsigned long) high_memory - p)
                        wrote = (unsigned long) high_memory - p;

-               wrote = do_write_mem(file, (void*)p, p, buf, wrote, ppos);
+               written = do_write_mem((void*)p, p, buf, wrote, ppos);
+               if (written != wrote)
+                       return written;
+               wrote = written;

                p += wrote;
                buf += -EFAULT;


Fix in the second version of the patch, for the 3rd hunk:
- Fix this line:
+                              unwritten = copy_from_user(kbuf, buf, len);
+                              if (unwritten != len) {
					[... error handling]
to this:
+                              unwritten = copy_from_user(kbuf, buf, len);
+                              if (unwritten != 0) {
					[...error handling]
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729