Re: [PATCH] hid: Logitech G13 driver 0.0.4

[Rick L. Vinyard, Jr.]

Jiri Kosina wrote:
> On Mon, 25 Jan 2010, Rick L. Vinyard, Jr. wrote:
>
>> > Am Mittwoch, 20. Januar 2010 21:47:22 schrieb Rick L. Vinyard Jr.:
>> >> +       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.
>

I agree. It was something that stuck out as an oddity, so I did look into
it more and it seemed like it was the standard behavior for several
drivers such as hecubafb, arcfb, xen-fbfront, metronomefb, broadsheetfb,
et. al.

I don't have a problem changing it. In fact, it appears that this code is
largely replicated from fb_sys_write() in hecubafb, et. al.

The only differences appear to be:

fb_sys_write:
   total_size = info->screen_size;

   if (total_size == 0)
      total_size = info->fix.smem_len;

...

   if (info->fbops->fb_sync)
      info->fbops->fb_sync(info);

hecubafb:
   total_size = info->fix.smem_len;

But, in the drivers screen_size and fb_sync are NULL anyway, so I'm not
sure why these drivers are explicitly modifying the write().

The approach taken by xen-fbfront seems to be even better in that it
relies on fb_sys_write():

static ssize_t xenfb_write(struct fb_info *p, const char __user *buf,
			size_t count, loff_t *ppos)
{
	struct xenfb_info *info = p->par;
	ssize_t res;

	res = fb_sys_write(p, buf, count, ppos);
	xenfb_refresh(info, 0, 0, info->page->width, info->page->height);
	return res;
}

But, if EFAULT is an issue I should modify the g13 version slightly to
account for the fact that fb_sys_write() could return EFAULT or EPERM
without modifying the buffer.

static ssize_t g13_fb_write(struct fb_info *info, const char __user *buf,
			    size_t count, loff_t *ppos)
{
	struct g13_data *par = info->par;
	ssize_t res;

	res = fb_sys_write(p, buf, count, ppos);
	if ( res != -EFAULT && res != -EPERM )
                g13_fb_update(par);
        return res;
}

The reason why I didn't put it as the following:
	if ( res >= 0 )
                g13_fb_update(par);

is because it appears there are two conditions that fb_sys_write() will
continue on to the copy_from_user() but still return an error code. In
these cases -EFBIG and -ENOSPEC are returned but it is possible the buffer
has still been modified.

Again, like I said, I don't have a problem changing it. If it's wrong,
it's wrong no matter how many other drivers may take the same approach.

My intuition tells me it's wrong, but at the same time I'd like to know if
there's a reason behind the other drivers also ignoring the EFAULT. Or,
perhaps it simply started in one driver and everyone else patterned after
it just like I did the g13 driver.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/