[PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[Dmitry Torokhov]

When copy_to/from_user fails in the middle of transfer we should not
report to the user that read/write partially succeeded but rather
report -EFAULT right away, so that application will know that it got
its buffers all wrong.

If application messed up its buffers we can't trust the data fetched
from userspace and successfully written to the device or if data read
from the device and transferred to userspace ended up where application
expected it to end.

If serio_write() fails we still going to report partial writes if failure
happens in the middle of the transfer.

This is basically a revert of 7a0a27d2ce38aee19a31fee8c12095f586eed393
and 4fa0771138d0b56fe59ab8ab3b1ce9e594484362.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/serio/serio_raw.c |   33 ++++++++++++++++++---------------
 1 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
index 3e24362..59df2e7 100644
--- a/drivers/input/serio/serio_raw.c
+++ b/drivers/input/serio/serio_raw.c
@@ -165,9 +165,9 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
 	struct serio_raw *serio_raw = client->serio_raw;
 	char uninitialized_var(c);
 	ssize_t read = 0;
-	int error = 0;
+	int error;
 
-	do {
+	for (;;) {
 		if (serio_raw->dead)
 			return -ENODEV;
 
@@ -179,24 +179,24 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
 			break;
 
 		while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
-			if (put_user(c, buffer++)) {
-				error = -EFAULT;
-				goto out;
-			}
+			if (put_user(c, buffer++))
+				return -EFAULT;
 			read++;
 		}
 
 		if (read)
 			break;
 
-		if (!(file->f_flags & O_NONBLOCK))
+		if (!(file->f_flags & O_NONBLOCK)) {
 			error = wait_event_interruptible(serio_raw->wait,
 					serio_raw->head != serio_raw->tail ||
 					serio_raw->dead);
-	} while (!error);
+			if (error)
+				return error;
+		}
+	}
 
-out:
-	return read ?: error;
+	return read;
 }
 
 static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
@@ -204,8 +204,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
 {
 	struct serio_raw_client *client = file->private_data;
 	struct serio_raw *serio_raw = client->serio_raw;
-	ssize_t written = 0;
-	int retval;
+	int retval = 0;
 	unsigned char c;
 
 	retval = mutex_lock_interruptible(&serio_raw_mutex);
@@ -225,16 +224,20 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
 			retval = -EFAULT;
 			goto out;
 		}
+
 		if (serio_write(serio_raw->serio, c)) {
-			retval = -EIO;
+			/* Either signal error or partial write */
+			if (retval == 0)
+				retval = -EIO;
 			goto out;
 		}
-		written++;
+
+		retval++;
 	}
 
 out:
 	mutex_unlock(&serio_raw_mutex);
-	return written ?: retval;
+	return retval;
 }
 
 static unsigned int serio_raw_poll(struct file *file, poll_table *wait)
-- 
1.7.7.6


-- 
Dmitry

Re: [PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[Alan Cox]

On Sun, 29 Apr 2012 23:44:51 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> When copy_to/from_user fails in the middle of transfer we should not
> report to the user that read/write partially succeeded but rather
> report -EFAULT right away, so that application will know that it got
> its buffers all wrong.

Actually POSIX/SuS is quite explicit that if a write or read partially
succeeds you return the partial result. The error will be returned only
if nothing was written or read, or in some cases can be buffered by the
code for the next read/write attempt (eg for sockets).

In the wait_event_interruptible() case it's even more important as a
signal handler and syscall restart needs to do the right thing. Looking
at the patch it looks like a signal interrupting loses you data with this
change appplied. The -EFAULT one is a corner case, the signal handlers
causing data loss one is not.

EFAULT matters for certain crazy software like persistent storage managers
which use -EFAULT and segfault/bus error catching to implement full
persistent storage. Those people may well be somewhat out of their tree.

> -		if (!(file->f_flags & O_NONBLOCK))
> +		if (!(file->f_flags & O_NONBLOCK)) {
>  			error = wait_event_interruptible(serio_raw->wait,
>  					serio_raw->head != serio_raw->tail ||
>  					serio_raw->dead);
> -	} while (!error);
> +			if (error)
> +				return error;

And we lose data as far as I can see, so a timer event will cause the app
to malfunction.

Alan

Re: [PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[Dmitry Torokhov]

Hi Alan,

On Mon, Apr 30, 2012 at 11:12:23AM +0100, Alan Cox wrote:
> On Sun, 29 Apr 2012 23:44:51 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > When copy_to/from_user fails in the middle of transfer we should not
> > report to the user that read/write partially succeeded but rather
> > report -EFAULT right away, so that application will know that it got
> > its buffers all wrong.
> 
> Actually POSIX/SuS is quite explicit that if a write or read partially
> succeeds you return the partial result. The error will be returned only
> if nothing was written or read, or in some cases can be buffered by the
> code for the next read/write attempt (eg for sockets).

Could you please point me to that particular part of the spec? My
reading of

http://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html

leads me to conclusion that we are only required to report short reads
when interrupted (or don't have enough data) and that severe errors may
be reported at any time:

"Upon successful completion, where nbyte is greater than 0, read() shall
mark for update the last data access timestamp of the file, and shall
return the number of bytes read. This number shall never be greater than
nbyte. The value returned may be less than nbyte if the number of bytes
left in the file is less than nbyte, if the read() request was
interrupted by a signal, or if the file is a pipe or FIFO or special
file and has fewer than nbyte bytes immediately available for reading.
For example, a read() from a file associated with a terminal may return
one typed line of data.

If a read() is interrupted by a signal before it reads any data, it
shall return -1 with errno set to [EINTR].

If a read() is interrupted by a signal after it has successfully read
some data, it shall return the number of bytes read."

> 
> In the wait_event_interruptible() case it's even more important as a
> signal handler and syscall restart needs to do the right thing. Looking
> at the patch it looks like a signal interrupting loses you data with this
> change appplied.

No, it should not as we do not wait for more data once some data is
read.

> The -EFAULT one is a corner case, the signal handlers
> causing data loss one is not.
> 
> EFAULT matters for certain crazy software like persistent storage managers
> which use -EFAULT and segfault/bus error catching to implement full
> persistent storage. Those people may well be somewhat out of their tree.

I do not think persistent storage applicable to character devices, in
particular PS/2 port.

> 
> > -		if (!(file->f_flags & O_NONBLOCK))
> > +		if (!(file->f_flags & O_NONBLOCK)) {
> >  			error = wait_event_interruptible(serio_raw->wait,
> >  					serio_raw->head != serio_raw->tail ||
> >  					serio_raw->dead);
> > -	} while (!error);
> > +			if (error)
> > +				return error;
> 
> And we lose data as far as I can see, so a timer event will cause the app
> to malfunction.

No, we'd only go into wait_event_interruptible() if we haven't read
anything yet so we won't lose any data here.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[Alan Cox]

> If a read() is interrupted by a signal after it has successfully read
> some data, it shall return the number of bytes read."

Fair point - I guess technically EFAULT is not a signal. I still think
its a better behaviour to not lose bytes but objection withdrawn.

> No, we'd only go into wait_event_interruptible() if we haven't read
> anything yet so we won't lose any data here.

Ok

Alan

Re: [PATCH] Input: serio_raw - signal EFAULT even if read/write partially succeeds

[Dmitry Torokhov]

On Mon, Apr 30, 2012 at 10:26:03PM +0100, Alan Cox wrote:
> > If a read() is interrupted by a signal after it has successfully read
> > some data, it shall return the number of bytes read."
> 
> Fair point - I guess technically EFAULT is not a signal. I still think
> its a better behaviour to not lose bytes but objection withdrawn.

I generally agree (with other errors that are not due to faulty
application implementation). But I think signalling EFAULT is more
beneficial than partial read, because it will let application know
earlier that it uses incorrcet buffers. If we were to return patrtial
success apllication would simply retry the read and would never even
know that there is a problem.

Another is practical consideration in serio_raw - we take bytes out of
the queue and then attempt to put_user(). If it faults and we decide to
return partial success we need to put the byte we just fecthed back into
the queue. Or we can see if memory can be accessed before fetching the
byte. In both cases we make the driver more complex for benefit of
faulty applications.

Thanks.

-- 
Dmitry