Re: [PATCH] hid: Logitech G13 driver 0.0.4

On Mon, 25 Jan 2010, Rick L. Vinyard, Jr. wrote:

quoted

Am Mittwoch, 20. Januar 2010 21:47:22 schrieb Rick L. Vinyard Jr.:

quoted

+       if (copy_from_user(dst, buf, count))
+               err = -EFAULT;
+
+       if (!err)
+               *ppos += count;
+
+       g13_fb_update(par);
+
+       return (err) ? err : count;

Do you really want to go on if you get -EFAULT?

Since the hecubafb driver (which I based this portion of the g13 driver
on) uses the same approach I tried to justify it myself when I first saw
it.

I don't know if this was the intent of the hecubafb author, but this is
the way I saw it.

By this point the copy_from_user() has failed. If it resulted in a partial
copy to dst then continuing on to an update can't hurt, and would reduce
display jitter if a re-write occurs from userspace. If a re-write doesn't
occur the virtual framebuffer is hosed anyways as dst is is the underlying
framebuffer.

Given that, the worst-case consequence seems to be an unnecessary update
to the device display.

Well, it's quite questionable (and I'd say unexpected) behavior to go on 
even if userspace passes wild pointers to kernel.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.