Re: fbmem.c : FBIOGETCMAP

Re: fbmem.c : FBIOGETCMAP

[James Simmons]

> I can run the fb color map tests now if you had a
> chance to fix the code.
> 
> I am just running fbtest from the sourceforge fb cvs.

I'm CC the fbdev list because the fb color map code is a mess. I like to 
know what the best approach would be to clean it up.

FIrst the main bug. In fb_copy_cmap we should be testing to see if 
copy_from[to]_user fails. We don't. The other issue is for fb_copy_cmap
We handle memcpy, copy_from_user, and copy)to_user. Now in fb_set_cmap
we use get_user also without any checks. 

So we could have fb_copy_cmap keep handling all these issues and remove 
get_user from fb_set_cmap. We would just pass in fbcmap we got from 
fb_copy_cmap and pass into fb_set_cmap. The other approach is to 
remove copy_from_user in fb_copy_cmap and place it int fb_set_cmap. 

What do you think is the best approach to this?



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fbmem.c : FBIOGETCMAP

[Antonino Daplas]

On Wed, 2003-01-08 at 06:37, James Simmons wrote:
> 
> > I can run the fb color map tests now if you had a
> > chance to fix the code.
> > 
> > I am just running fbtest from the sourceforge fb cvs.
> 
> I'm CC the fbdev list because the fb color map code is a mess. I like to 
> know what the best approach would be to clean it up.
> 
> FIrst the main bug. In fb_copy_cmap we should be testing to see if 
> copy_from[to]_user fails. We don't. The other issue is for fb_copy_cmap
> We handle memcpy, copy_from_user, and copy)to_user. Now in fb_set_cmap
> we use get_user also without any checks. 
> 
> So we could have fb_copy_cmap keep handling all these issues and remove 
> get_user from fb_set_cmap. We would just pass in fbcmap we got from 
> fb_copy_cmap and pass into fb_set_cmap. The other approach is to 
> remove copy_from_user in fb_copy_cmap and place it int fb_set_cmap. 
> 
> What do you think is the best approach to this?
> 
> 
I don't know, it looks okay to me except for the following:

1.  In case FBIOPUTCMAP, fb_set_cmap() must pass 1 (to imply get info
from user space).

2.  In case FBIOGETCMAP, fb_copy_cmap() must pass 2 (to imply copy cmap
from kernel space to user space)

3.  no checks when doing the user access functions (should have no
effect really, except for the extra processing).

I think fb_set_cmap() is a bit misleading.  It mostly implies set the hardware 
DAC based on the color information in fb_cmap.  fb_cmap is either in kernel 
space or user space.  There is no need to copy the actual cmap to kernel 
space and we just let the user app keep a copy of its own.

So fb_copy_cmap() is not necessary when doing an fb_set_cmap() because
you have to allocate memory for it, which has to be deallocated after
doing an fb_set_cmap().  We cannot use info->cmap because it might be of
different length, and it will clobber the console cmap.

The copy_from_user() of fb_copy_cmap() may not be needed unless we
have support for it.  If this is the case, then we have to find a way of
saving info->cmap first then allocate memory for the new cmap.  Then
deallocate the new cmap and restore the default cmap to info->cmap
afterwards.  You will need additional code for this. In any case, info->cmap
is driver-private anyway, and user apps have no business replacing
info->cmap with its own.

I think the main use of those 2 ioctls is this:

1. save kernel cmap by doing FBIOGETCMAP,
2. set hardware DAC based on user app's cmap using FBIOPUTCMAP,
3. upon exit of the app, restore hardware DAC by doing FBIOPUTCMAP using
the cmap taken from FBIOGETCMAP.

Tony 




-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

fbmem.c : FBIOGETCMAP

[Jon Smirl]

I'm using 2.53 and I ran into this when running
fbtest. It looks like a return went missing.

	case FBIOGETCMAP:
		if (copy_from_user(&cmap, (void *) arg,
sizeof(cmap)))
			return -EFAULT;
		fb_copy_cmap(&info->cmap, &cmap, 0);
	case FBIOPAN_DISPLAY:

In 2.4 it looked like this:

	case FBIOGETCMAP:
		if (copy_from_user(&cmap, (void *) arg,
sizeof(cmap)))
			return -EFAULT;
		return (fb->fb_get_cmap(&cmap, 0,
PROC_CONSOLE(info), info));
	case FBIOPAN_DISPLAY:


=====
Jon Smirl
jonsmirl@yahoo.com

__________________________________________________
Do you Yahoo!?
Yahoo! Mail Plus - Powerful. Affordable. Sign up now.
http://mailplus.yahoo.com


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fbmem.c : FBIOGETCMAP

[James Simmons]

> I'm using 2.53 and I ran into this when running
> fbtest. It looks like a return went missing.
> 
> 	case FBIOGETCMAP:
> 		if (copy_from_user(&cmap, (void *) arg,
> sizeof(cmap)))
> 			return -EFAULT;
> 		fb_copy_cmap(&info->cmap, &cmap, 0);
> 	case FBIOPAN_DISPLAY:
> 
> In 2.4 it looked like this:
> 
> 	case FBIOGETCMAP:
> 		if (copy_from_user(&cmap, (void *) arg,
> sizeof(cmap)))
> 			return -EFAULT;
> 		return (fb->fb_get_cmap(&cmap, 0,
> PROC_CONSOLE(info), info));
> 	case FBIOPAN_DISPLAY:

In the latest code there is a return 0 after fb_copy_cmap. So it is fixed 
with the latest tree.



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fbmem.c : FBIOGETCMAP

[Jon Smirl]

It needs a copy_to_user too to work. Probably doesn't
need the copy_from_user.

Is the right fix?

	case FBIOGETCMAP:
		fb_copy_cmap(&info->cmap, &cmap, 0);
		if (copy_to_user((void *) arg, &cmap, sizeof(cmap)))
			return -EFAULT;
		return 0;


=====
Jon Smirl
jonsmirl@yahoo.com

__________________________________________________
Do you Yahoo!?
Yahoo! Mail Plus - Powerful. Affordable. Sign up now.
http://mailplus.yahoo.com


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fbmem.c : FBIOGETCMAP

[James Simmons]

> It needs a copy_to_user too to work. Probably doesn't
> need the copy_from_user.
> 
> Is the right fix?
> 
> 	case FBIOGETCMAP:
> 		fb_copy_cmap(&info->cmap, &cmap, 0);
> 		if (copy_to_user((void *) arg, &cmap, sizeof(cmap)))
> 			return -EFAULT;
> 		return 0;

Oops. I see the problem. It should be 

  fb_copy_cmap(&info->cmap, &cmap, 2);

This tells it to use copy_to_user. I see a few mistakes here. Fixing...

There are several bugs here. I have to cleanup it up. FOr example in 
fb_copy_cmap there is test for a EFAULT for any copy_* function. This is 
just broken. It has been broken for a long time. Will fix. Will you test 
my patch as soon as it is ready?




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf