[PATCHv2] evdev: fix evdev_write return value on partial writes

[PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

As was recently brought up on the busybox list
(http://lists.busybox.net/pipermail/busybox/2011-January/074565.html),
evdev_write doesn't properly check the count argument, which will
lead to a return value > count on partial writes if the remaining bytes
are accessible - Causing userspace confusion.

Fix it by only handling each full input_event structure and return -EINVAL
if less than 1 struct was written, similar to how it is done in evdev_read.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 Changes since v1:
 - Return -EINVAL on writes of length < sizeof(struct input_event), similar
   to how it's done in evdev_read.

 drivers/input/evdev.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c8471a2..1ee7d0f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 	struct input_event event;
 	int retval;
 
+	if (count < input_event_size())
+		return -EINVAL;
+
 	retval = mutex_lock_interruptible(&evdev->mutex);
 	if (retval)
 		return retval;
@@ -330,7 +333,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 		goto out;
 	}
 
-	while (retval < count) {
+	while ((retval + input_event_size()) <= count) {
 
 		if (input_event_from_user(buffer + retval, &event)) {
 			retval = -EFAULT;
-- 
1.7.2.3

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

Hi Peter,

> As was recently brought up on the busybox list
> (http://lists.busybox.net/pipermail/busybox/2011-January/074565.html),
> evdev_write doesn't properly check the count argument, which will
> lead to a return value > count on partial writes if the remaining bytes
> are accessible - Causing userspace confusion.
> 
> Fix it by only handling each full input_event structure and return -EINVAL
> if less than 1 struct was written, similar to how it is done in evdev_read.
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

Why not add the Reported-by here yourself?

> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  	struct input_event event;
>  	int retval;
>  
> +	if (count < input_event_size())
> +		return -EINVAL;
> +

This assumes that write will only ever be called with sufficient
data. It is not an error to write (and report) less data than
specified, so perhaps the above will yield unpleasant surprises.

>  	retval = mutex_lock_interruptible(&evdev->mutex);
>  	if (retval)
>  		return retval;
> @@ -330,7 +333,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  		goto out;
>  	}
>  
> -	while (retval < count) {
> +	while ((retval + input_event_size()) <= count) {

Too many parenthesis.

>  
>  		if (input_event_from_user(buffer + retval, &event)) {
>  			retval = -EFAULT;

Thanks,
Henrik

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

>>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:

Hi Henrik,

 >> Fix it by only handling each full input_event structure and return -EINVAL
 >> if less than 1 struct was written, similar to how it is done in evdev_read.
 >> 
 >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

 Henrik> Why not add the Reported-by here yourself?

Because I sent this mail before seing Baruch's reply.

I can send a v3 with it, but I wanted to wait a bit to see if there was
any more feedback.

 >> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 >> struct input_event event;
 >> int retval;
 >> 
 >> +	if (count < input_event_size())
 >> +		return -EINVAL;
 >> +

 Henrik> This assumes that write will only ever be called with sufficient
 Henrik> data. It is not an error to write (and report) less data than
 Henrik> specified, so perhaps the above will yield unpleasant surprises.

Well, like this it's consistent with evdev_read(). We can only consume
complete input_event structures, so we can either return 0 (no events
consumed) or -EINVAL (invalid data).

Before the patch we returned sizeof input_event (if data after write
buffer is accessible) or -EINVAL otherwise.

The v1 patch returned 0, but that causes problems with userspace, as it
often does:

    wlile (len) {
        n = write(fd, buf, len);
        if (n <= 0) break;
        len -= n;
        buf += n;
    }

Which will then loop forever.

Returning -EINVAL on something that can never work seems like the sanest
option.

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Baruch Siach]

Hi Peter,

On Thu, Jan 27, 2011 at 12:21:30PM +0100, Peter Korsgaard wrote:
> >>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:
>  >> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const 
>  >> char __user *buffer,
>  >> struct input_event event;
>  >> int retval;
>  >> 
>  >> +	if (count < input_event_size())
>  >> +		return -EINVAL;
>  >> +
> 
>  Henrik> This assumes that write will only ever be called with sufficient
>  Henrik> data. It is not an error to write (and report) less data than
>  Henrik> specified, so perhaps the above will yield unpleasant surprises.
> 
> Well, like this it's consistent with evdev_read(). We can only consume
> complete input_event structures, so we can either return 0 (no events
> consumed) or -EINVAL (invalid data).
> 
> Before the patch we returned sizeof input_event (if data after write
> buffer is accessible) or -EINVAL otherwise.
> 
> The v1 patch returned 0, but that causes problems with userspace, as it
> often does:
> 
>     wlile (len) {
>         n = write(fd, buf, len);
>         if (n <= 0) break;
>         len -= n;
>         buf += n;
>     }
> 
> Which will then loop forever.

Well this code won't loop forever, since n == 0. Only if you do:

    if (n < 0) break;

That is.

baruch

> Returning -EINVAL on something that can never work seems like the sanest
> option.

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

>>>>> "Baruch" == Baruch Siach <baruch@tkos.co.il> writes:

 >> wlile (len) {
 >> n = write(fd, buf, len);
 >> if (n <= 0) break;
 >> len -= n;
 >> buf += n;
 >> }
 >> 
 >> Which will then loop forever.

 Baruch> Well this code won't loop forever, since n == 0. Only if you do:

 Baruch>     if (n < 0) break;

 Baruch> That is.

Naturally, -ENOCOFFE.

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

>  >> Fix it by only handling each full input_event structure and return -EINVAL
>  >> if less than 1 struct was written, similar to how it is done in evdev_read.
>  >> 
>  >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> 
>  Henrik> Why not add the Reported-by here yourself?
> 
> Because I sent this mail before seing Baruch's reply.
> 
> I can send a v3 with it, but I wanted to wait a bit to see if there was
> any more feedback.

Great, no problem.

>  >> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  >> struct input_event event;
>  >> int retval;
>  >> 
>  >> +	if (count < input_event_size())
>  >> +		return -EINVAL;
>  >> +
> 
>  Henrik> This assumes that write will only ever be called with sufficient
>  Henrik> data. It is not an error to write (and report) less data than
>  Henrik> specified, so perhaps the above will yield unpleasant surprises.
> 
> Well, like this it's consistent with evdev_read(). We can only consume
> complete input_event structures, so we can either return 0 (no events
> consumed) or -EINVAL (invalid data).

But read and write are not completely symmetric.

> Before the patch we returned sizeof input_event (if data after write
> buffer is accessible) or -EINVAL otherwise.
> 
> The v1 patch returned 0, but that causes problems with userspace, as it
> often does:
> 
>     wlile (len) {
>         n = write(fd, buf, len);
>         if (n <= 0) break;
>         len -= n;
>         buf += n;
>     }
> 
> Which will then loop forever.

I won't argue against this case (with < 0) being frequent, but one
should really check "n < len" to be safe. Hopefully Dmitry has some
more input.

Either way, thanks for finding this problem.

Henrik

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

>>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:

Hi,

 Henrik> I won't argue against this case (with < 0) being frequent, but one
 Henrik> should really check "n < len" to be safe. Hopefully Dmitry has some
 Henrik> more input.

No, the point is that write (and read) can consume less data than
requested, without it being an error. Robust userspace code should
adjust buffer address / size and redo the work until all data is
transferred or an error occurs.

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

On Thu, Jan 27, 2011 at 01:04:49PM +0100, Peter Korsgaard wrote:
> >>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:
> 
> Hi,
> 
>  Henrik> I won't argue against this case (with < 0) being frequent, but one
>  Henrik> should really check "n < len" to be safe. Hopefully Dmitry has some
>  Henrik> more input.
> 
> No, the point is that write (and read) can consume less data than
> requested, without it being an error. Robust userspace code should
> adjust buffer address / size and redo the work until all data is
> transferred or an error occurs.

Shouldn't the error be on (!len || len % smallest_acceptable_chunk),
then? Which makes me wonder about regressions - perhaps accumulating
partial writes in evdev is more safe from that perspective.

Thanks,
Henrik

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

>>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:

 Henrik> I won't argue against this case (with < 0) being frequent, but one
 Henrik> should really check "n < len" to be safe. Hopefully Dmitry has some
 Henrik> more input.
 >> 
 >> No, the point is that write (and read) can consume less data than
 >> requested, without it being an error. Robust userspace code should
 >> adjust buffer address / size and redo the work until all data is
 >> transferred or an error occurs.

 Henrik> Shouldn't the error be on (!len || len % smallest_acceptable_chunk),
 Henrik> then? Which makes me wonder about regressions - perhaps accumulating
 Henrik> partial writes in evdev is more safe from that perspective.

No, writing more than 1 complete struct should just consume the full
structs and return the number of bytes consumed, similar to all other
cases in the kernel where we return a length < count.

No sane userspace will write anything else than a multiple of
sizeof(input_event) though.

I doubt this will introduce any regressions (but you never know). The
only situation I can see is if userspace would fill out a proper struct
input_dev but use a wrong (too small) length in the write call. We used
to accept these, but with the patch here it will -EINVAL.

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 01:43:17PM +0100, Peter Korsgaard wrote:
> >>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:
> 
>  Henrik> I won't argue against this case (with < 0) being frequent, but one
>  Henrik> should really check "n < len" to be safe. Hopefully Dmitry has some
>  Henrik> more input.
>  >> 
>  >> No, the point is that write (and read) can consume less data than
>  >> requested, without it being an error. Robust userspace code should
>  >> adjust buffer address / size and redo the work until all data is
>  >> transferred or an error occurs.
> 
>  Henrik> Shouldn't the error be on (!len || len % smallest_acceptable_chunk),
>  Henrik> then? Which makes me wonder about regressions - perhaps accumulating
>  Henrik> partial writes in evdev is more safe from that perspective.
> 
> No, writing more than 1 complete struct should just consume the full
> structs and return the number of bytes consumed, similar to all other
> cases in the kernel where we return a length < count.
> 
> No sane userspace will write anything else than a multiple of
> sizeof(input_event) though.
> 
> I doubt this will introduce any regressions (but you never know). The
> only situation I can see is if userspace would fill out a proper struct
> input_dev but use a wrong (too small) length in the write call. We used
> to accept these, but with the patch here it will -EINVAL.

I think that returning -EINVAL is good idea, so I am going to apply it.
I also think we can change this loop to do-while kind since we already
checked there enough space for an event.

-- 
Dmitry


Input: evdev - fix evdev_write return value on partial writes

From: Peter Korsgaard <jacmet@sunsite.dk>

As was recently brought up on the busybox list
(http://lists.busybox.net/pipermail/busybox/2011-January/074565.html),
evdev_write doesn't properly check the count argument, which will
lead to a return value > count on partial writes if the remaining bytes
are accessible - causing userspace confusion.

Fix it by only handling each full input_event structure and return -EINVAL
if less than 1 struct was written, similar to how it is done in evdev_read.

Reported-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/evdev.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)


diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c8471a2..7f42d3a 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 	struct input_event event;
 	int retval;
 
+	if (count < input_event_size())
+		return -EINVAL;
+
 	retval = mutex_lock_interruptible(&evdev->mutex);
 	if (retval)
 		return retval;
@@ -330,17 +333,16 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 		goto out;
 	}
 
-	while (retval < count) {
-
+	do {
 		if (input_event_from_user(buffer + retval, &event)) {
 			retval = -EFAULT;
 			goto out;
 		}
+		retval += input_event_size();
 
 		input_inject_event(&evdev->handle,
 				   event.type, event.code, event.value);
-		retval += input_event_size();
-	}
+	} while (retval + input_event_size() <= count);
 
  out:
 	mutex_unlock(&evdev->mutex);

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

> > I doubt this will introduce any regressions (but you never know). The
> > only situation I can see is if userspace would fill out a proper struct
> > input_dev but use a wrong (too small) length in the write call. We used
> > to accept these, but with the patch here it will -EINVAL.
> 
> I think that returning -EINVAL is good idea, so I am going to apply it.
> I also think we can change this loop to do-while kind since we already
> checked there enough space for an event.

The code below will return -EINVAL even if some parts of the buffer
was successfully read, though.

> 
> -- 
> Dmitry
> 
> 
> Input: evdev - fix evdev_write return value on partial writes
> 
> From: Peter Korsgaard <jacmet@sunsite.dk>
> 
> As was recently brought up on the busybox list
> (http://lists.busybox.net/pipermail/busybox/2011-January/074565.html),
> evdev_write doesn't properly check the count argument, which will
> lead to a return value > count on partial writes if the remaining bytes
> are accessible - causing userspace confusion.
> 
> Fix it by only handling each full input_event structure and return -EINVAL
> if less than 1 struct was written, similar to how it is done in evdev_read.
> 
> Reported-by: Baruch Siach <baruch@tkos.co.il>
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  drivers/input/evdev.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c8471a2..7f42d3a 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  	struct input_event event;
>  	int retval;

Alternatively,

size_t num_written = 0;
int ret = 0;

>  
> +	if (count < input_event_size())
> +		return -EINVAL;
> +
>  	retval = mutex_lock_interruptible(&evdev->mutex);
>  	if (retval)
>  		return retval;
> @@ -330,17 +333,16 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  		goto out;
>  	}
>  
> -	while (retval < count) {

while (num_written + input_event_size() <= count) {

> -
> +	do {
>  		if (input_event_from_user(buffer + retval, &event)) {
>  			retval = -EFAULT;

ret = -EFAULT;

>  			goto out;
>  		}
> +		retval += input_event_size();

num_written += input_event_size();

>  
>  		input_inject_event(&evdev->handle,
>  				   event.type, event.code, event.value);
> -		retval += input_event_size();
> -	}
> +	} while (retval + input_event_size() <= count);
>  
>   out:
>  	mutex_unlock(&evdev->mutex);

return ret ? ret : num_written;

Thanks,
Henrik

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Peter Korsgaard]

>>>>> "Henrik" == Henrik Rydberg <rydberg@euromail.se> writes:

Hi,

 >> I think that returning -EINVAL is good idea, so I am going to apply it.
 >> I also think we can change this loop to do-while kind since we already
 >> checked there enough space for an event.

 Henrik> The code below will return -EINVAL even if some parts of the buffer
 Henrik> was successfully read, though.

You mean written? I don't see that. The only place we return -EINVAL is
at the initial check.

 >> --- a/drivers/input/evdev.c
 >> +++ b/drivers/input/evdev.c
 >> @@ -321,6 +321,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 >> struct input_event event;
 >> int retval;

 Henrik> Alternatively,

 Henrik> size_t num_written = 0;
 Henrik> int ret = 0;

Any reason for this bigger change? It's imho pretty clear as is. I don't
see any functional change from your version.

-- 
Bye, Peter Korsgaard

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

>  Henrik> The code below will return -EINVAL even if some parts of the buffer
>  Henrik> was successfully read, though.
> 
> You mean written? I don't see that. The only place we return -EINVAL is
> at the initial check.

Right, it should have been written, and -EFAULT, referring to the code
in the loop.

Henrik

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Dmitry Torokhov]

On Fri, Feb 04, 2011 at 12:23:14PM +0100, Henrik Rydberg wrote:
> >  Henrik> The code below will return -EINVAL even if some parts of the buffer
> >  Henrik> was successfully read, though.
> > 
> > You mean written? I don't see that. The only place we return -EINVAL is
> > at the initial check.
> 
> Right, it should have been written, and -EFAULT, referring to the code
> in the loop.
> 

If we faulted somewhere midway the only sane thing it to treat the whole
request as suspect and return -EFAULT. We really do not know what
happened to the other data (it could have gone past the buffer user
intended but haven't triggered the fault yet. We can't fully recover
either since we aleady retrievend the event that caused fault.

-- 
Dmitry

Re: [PATCHv2] evdev: fix evdev_write return value on partial writes

[Henrik Rydberg]

On Fri, Feb 04, 2011 at 09:15:19AM -0800, Dmitry Torokhov wrote:
> On Fri, Feb 04, 2011 at 12:23:14PM +0100, Henrik Rydberg wrote:
> > >  Henrik> The code below will return -EINVAL even if some parts of the buffer
> > >  Henrik> was successfully read, though.
> > > 
> > > You mean written? I don't see that. The only place we return -EINVAL is
> > > at the initial check.
> > 
> > Right, it should have been written, and -EFAULT, referring to the code
> > in the loop.
> > 
> 
> If we faulted somewhere midway the only sane thing it to treat the whole
> request as suspect and return -EFAULT. We really do not know what
> happened to the other data (it could have gone past the buffer user
> intended but haven't triggered the fault yet. We can't fully recover
> either since we aleady retrievend the event that caused fault.

Alright. I think all ambiguities have now been exhausted, so ACK from
me as well. :-)

Thanks,
Henrik