On Thu, Jan 27, 2011 at 01:43:17PM +0100, Peter Korsgaard wrote: > >>>>> "Henrik" == Henrik Rydberg <rydberg@xxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> Signed-off-by: Peter Korsgaard <jacmet@xxxxxxxxxx> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx> --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
